Opened 3 weeks ago
Last modified 3 weeks ago
#37156 closed New feature
Properly implement async Redis methods — at Version 4
| Reported by: | Johannes Maron | Owned by: | |
|---|---|---|---|
| Component: | Core (Cache system) | Version: | 6.0 |
| Severity: | Normal | Keywords: | redis, asyncio, async |
| Cc: | Johannes Maron | Triage Stage: | Unreviewed |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description (last modified by )
The backend API already implements aget, aset, β¦ methods.
However, the native RedisCache backend, doesn't reimplement those methods to use the asyncio API of the Redis SDK, but rather the asgiref fallback.
As discussed before, the extra threading comes with a significant overhead and disrupts things like full-page caching of otherwise async views.
This proposal does not alter Django's public API, but rather optimizes internal implementation, which is why I put this on Trac.
Change History (4)
comment:1 by , 3 weeks ago
comment:2 by , 3 weeks ago
... the extra threading comes with a significant overhead
Sigh. This is essentially a myth, at least these days. Unless you're repeatedly crossing the boundary in a hot-loop β which, don't! β overhead is basically negligible.
Redis-py's async client is no faster than the sync client (indeed it seems slightly slower, but I need to verify that before stating definitively βΒ it's negligible too though). Similar to the way psycopg's async cursors serialise queries, throughput is limited by pool size. And do enable pipelining!
I shall put writing this up on the backlog, but it's not at all clear that the argument to remove (particularly) sync_to_async calls throughout is/remains sound.
...core devs won't want to...
... core devs don't like ...
This phrasing isn't helpful. (TBH it verges on prompting a CoC report, since it's frequent.) The original design decisions were to support both sync and async views (and so code) under both WSGI and ASGI, and (given that) duplicating the entire code tree with pure async versions is not a sustainable strategy. Nothing has changed there. It's not about "want to" or "not liking".
comment:3 by , 3 weeks ago
Ok, I should have withheld any performance claims w/o proof. But since I ran out of my monthly token rations, let's park that discussion.
---
In my humble opinion, I thought it to be reasonable to assume that RedisCache.aget actually calls the async stack of the Redis SDK.
I certainly did and tripped on the fact it wasn't. I am aware that connection management is a complication. But just because something is hard has never stopped us before. <3
comment:4 by , 3 weeks ago
| Description: | modified (diff) |
|---|
Hey. Happy to help get rid of the
async_to_syncstuff in Django π I'll leave a few thoughts below, hope they help.To get a real async implementation, we basically just need to implement
AsyncRedisClient. Django already depends on redis, which ships an async client. But there's a catch. πThe async redis client relies on event-loop-bound connections, which means the code won't work under WSGI. Django supports running async code under WSGI, and core devs won't want to drop that. So the options are:
AsyncRedisCacheso people on ASGI can opt into it. Avoids breaking changes, but core devs don't like adding a new class for this.I looked into the async_to_sync performance degradation, and it looks like creating threads doesn't cause significant degradation on its own. what matters more is how frequently you hit it during the request cycle. If you're seeing something like that, could you share your setup (RPS, sync_to_async calls per request, avg CPU per pod)? That'd be helpful.