Avoid synchronous code within IDOM

Old Behavior

Currently, IDOM relies on synchronous code to properly queue renders. This code blocks the asyncio event loop, causing massive concurrency issues.

See section "Two Worlds of Python" for why this is a problem:
https://arunrocks.com/a-guide-to-asgi-in-django-30-and-its-performance/

New Behavior

Rewrite parts of IDOM core to do everything using asyncio.

Implementation Details

Anything that relies on order of operations should be executed in a FIFO queue.

If synchronous code is required, it should be run in a thread in order to avoid blocking asyncio event loops.

All synchronous functions should be converted to async within IDOM core.

I had previously attempted threading the django-idom dispatcher as a quick fix, but that did not yield worthwhile performance benefits, likely due to context switching.

Code of Conduct

I agree to follow the Code of Conduct.

@rmorshea

@rmorshea Can we transfer this to idom-team/idom as a v2 goal.

Also, I personally consider this one as P0 since it demolishes the performance of any ASGI webserver.

I think we should create a more generic research task within IDOM core to investigate the best ways to improve performance (if there isn't one already). There may be other ways to make things faster (e.g. improving/removing VDOM diffing).

We should also create a task to benchmark the performance of IDOM within django-idom (i.e. see how much time IDOM contributes to a render vs Channels/Django).

reactive-python/reactpy#557 exists for generic performance improvements to rendering performance, but profiling isn't going to tell us the whole story. The issue in this scenario is concurrency, rather than performance.

The issue is visually pronounced enough to notice. For example, using the django-idom tests and opening two separate webpages concurrently, you can see them render serially fairly slowly, and console logs show WS connection accept being delayed during renders.

I'm not really sure how to benchmark multi-client websocket concurrency. I'll need to do some research and probably update the title/description of that issue.

It looks like there are no good benchmarking utilities we can use for our purposes. The websocket benchmarking world seems severely underdeveloped.

There's one repository I found that we might be able heavily customize to suit our needs
https://github.com/healeycodes/websocket-benchmarker

I'm going to close this issue since it outside the scope of Django-IDOM, and I'll migrate all the relevant information to the issue within IDOM Core.

Regardless, I still believe it's a really bad idea to mix sync/async code. In my opinion, at some point we're going to have to fix that and it's only a question of when. The sooner the better since it's most cleanly developed as a breaking change.

What sort of view is being rendered 18/s? In this simple benchmark of IDOM core, trivial views render a couple times per millisecond.

Ref: #23 (comment)

The testing I performed as first paint speed, which involves 100 concurrent attempts at the following sequence, using four unique Django IDOM components (same four in the Django IDOM test app):

WS accept
WS connect
Django-IDOM WS configure
Django-IDOM first paint

We can assume step four may be relatively fast, but my hypothesis is that this blocking of the event loop continuously is effectively blasting out a bunch of "software interrupts", which keeps stalling steps 1-3 from occurring. And the longer it takes for one WS connection to complete steps 1-3, the longer it will take for all of them. They will all compete for CPU time on Python's one GIL thread.

I'm also assuming that if you're testing a pre-connected WS then you're unlikely to detect much performance degradation, since handling things serially doesn't impact much of anything in that scenario. Things are fairly FIFO once your connected.

There's may also be an extra layer of latency added from constantly switching between sync/async, but I'm not certain if an idea similar to context-switching applies here.

As a side note, I've also experienced visibly good performance with re-renders. The only performance issues I'm having is the connect->first paint cycle.

I see, this makes more sense to me. So that's 18 first renders per second? We should measure to be sure, but because the render process is CPU bound I'm not sure making it more concurrent with async/await will improve things. In fact it might make it worse by introducing more context switching between tasks.

Correct. 18 first renders per second.

Right now we're using a Django Async consumer. I'd be interested in finding out what the performance looks like if we use a Django Sync consumer. I tried to implement that for performance testing a while ago but ran into an issue due to dispatch_single_view being async only.

@rmorshea

@rmorshea Running in to issues regarding sync components.

Django's entire ORM (all database queries) are completely broken within components.

For example...

@register.component.app_store()
@component
def app_store(websocket, state, set_state):
    from .models import Category
    from pprint import pprint

    pprint(Category.objects.all())

...will result in an exception django.core.exceptions.SynchronousOnlyOperation: You cannot call this from an async context - use a thread or sync_to_async.

There isn't any real solution for this besides doing all DB queries in an use_effect hook, which is a really awkward design pattern in the majority of scenarios.

The only real solutions to clean this up are either

Support async components
Create sync websocket consumer.
- A cursory look shows that supporting this isn't cleanly doable within IDOM core.

Just to push back a bit, isn't there some benefit to doing DB queries in an effect in that it forces the user to think about the scenario where the DB doesn't respond immediately? Even if components were async, in a scenario where you don't do the query in an effect and the DB responds slowly, the UI would freeze up because renders are inherently sequential.

The awkwardness of effects could be mitigated with a custom use_query hook. For example:

@register.component.app_store()
@component
def app_store(websocket, state, set_state):
    from .models import Category

    categories = use_query(lambda: Category.objects.all())
    
    if categories is None:
        return  # fallback
    else:
        return # actual view

Where use_query looks something like:

def use_query(do_query, default=None):
    do_query = use_callback(do_query)
    result, set_result = use_state(None)
    use_effect(lambda: set_result(do_query()))
    return result

There is a benefit, yes, but there's also a big awkwardness (mostly due to Django's half-baked async support).

Here's what kind of code is needed to get a query working currently:

@component
def app_store(websocket, state, set_state):
    categories, set_categories = hooks.use_state({})

    @hooks.use_effect
    async def _get_categories():
        if categories:
            return
        print("Categories state is empty. Refreshing...")
        set_categories(await get_categories())


from channels.db import database_sync_to_async

@database_sync_to_async
def get_categories() -> dict[Category, list[Subcategory]]:
    from .models import Category

    query = Subcategory.objects.select_related("category").order_by("name").all()
    categories = {}
    for subcategory in query:
        categories.setdefault(subcategory.category, []).append(subcategory)
    return categories

Basically, the query needs to be wrapped within an effect. Which itself contains a wrapped call to the query.

So to make a more complete list of what options exist

Wait for Django to develop async ORM support
use_query hook
Support async components
Create sync websocket consumer.

To be conservative about this, I think it'd be best to go with a use_query, or perhaps more generally (since you'd use this for writing to the DB too) a use_database hook. I think we could even provide some automatic niceties (e.g. applying the database_sync_to_async decorator automatically for the user.

Question though, why are you importing Category in the function?

Category can be imported at top level. Importing late for demonstration purposes.

On a tangent, we still should tag up soon. There's another potential bug with the key-child relationships I need to pass by you.

I can help with the implementation of use_database since there are some subtleties around object identity that need to be considered. The bit I'll need advice from you on is how to make sure the query executes before leaving the effect. It seems like DB queries are always lazy. Maybe a simple bool(query_set) is sufficient to force it, but there's probably some subtleties I'm not aware of there.

Yeah I'm also struggling with the lazy aspect of things. I'm trying to determine a solution today.

Can you write up an issue for this use_database hook where we can collab on this?

Already exists #39

Navigation Menu

Repository navigation

Avoid synchronous code within IDOM #31

Old Behavior

New Behavior

Implementation Details

Code of Conduct

3 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Avoid synchronous code within IDOM #31

Description

Old Behavior

New Behavior

Implementation Details

Code of Conduct

Activity

Archmonger commented on Jan 31, 2022

rmorshea commented on Jan 31, 2022

Archmonger commented on Jan 31, 2022

Archmonger commented on Jan 31, 2022

3 remaining items

rmorshea commented on Jan 31, 2022

Archmonger commented on Jan 31, 2022

rmorshea commented on Jan 31, 2022

Archmonger commented on Jan 31, 2022

Archmonger commented on Jan 31, 2022

Archmonger commented on Feb 4, 2022

rmorshea commented on Feb 4, 2022

Archmonger commented on Feb 4, 2022

rmorshea commented on Feb 4, 2022

Archmonger commented on Feb 4, 2022

Archmonger commented on Feb 4, 2022

rmorshea commented on Feb 4, 2022

Archmonger commented on Feb 4, 2022

rmorshea commented on Feb 4, 2022

Archmonger commented on Feb 4, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions