Avoid synchronous code within IDOM about reactpy-django HOT 21 CLOSED

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

from reactpy-django.

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.

from reactpy-django.

Already exists #39

from reactpy-django.

@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.

from reactpy-django.

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).

from reactpy-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.

from reactpy-django.

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.

from reactpy-django.

Ultimately, I think we need some quantitative evidence to back up these kinds of changes - it's hard to reason about all the factors that might be contributing to the observed issues.

from reactpy-django.

In the simple test scenario, we're sitting at ~18 component renders per second, which is a fairly bad quantitative metric. Asyncification is likely an equivalent effort to creating a more robust testing suite for WS. And it's likely impossible to single out the specific cause of this, even with profiling and a more robust WS benchmarking suite.

Additionally, blocking an async event loop is never a good idea in general.

I personally am hopeful that the "great Python divide" will eventually get alleviated in a future Python release. But for now, the standard is to avoid mixing sync/async.

from reactpy-django.

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

from reactpy-django.

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):

1. WS accept
2. WS connect
3. Django-IDOM WS configure
4. 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.

from reactpy-django.

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.

from reactpy-django.

Correct. 18 first renders per second.

from reactpy-django.

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.

from reactpy-django.

@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

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

from reactpy-django.

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

from reactpy-django.

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

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

from reactpy-django.

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?

from reactpy-django.

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.

from reactpy-django.

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

from reactpy-django.

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

from reactpy-django.