perf: pipeline redis set_state writes, cache required-state classes#6745
perf: pipeline redis set_state writes, cache required-state classes#6745Alek99 wants to merge 2 commits into
Conversation
set_state recursed over the substate tree spawning a task per substate; every recursive call re-GETed the same lock key and re-PTTLed it, and each touched state got its own unpipelined SET -- roughly 2N+1 round trips per event flush for N states. The tree is now walked once, the lock is verified once, and all touched states are written in a single pipeline: 3 round trips per flush regardless of tree size (12->3 for a root with 3 substates, 39->3 for a 13-state tree). Also: - Pickling runs off the event loop (asyncio.to_thread) for states whose previous payload exceeded 64KiB, instead of stalling the loop. - The recursive required-state-classes computation (pure class-level data) is now lru-cached per registration context and state hierarchy generation; subclass creation and module hot reload bump the generation, so dynamic classes invalidate cleanly. - _was_touched is reset after a successful write (mirroring the disk manager), so a once-touched state held by the oplock cache is no longer re-pickled and re-SET on every subsequent flush. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sns7EuBPYvfGCxRSRZ1bUx
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sns7EuBPYvfGCxRSRZ1bUx
Greptile SummaryThis PR optimizes Redis-backed state reads and writes. The main changes are:
Confidence Score: 4/5The Redis write path needs a fix for large-state serialization before merging.
reflex/istate/manager/redis.py Important Files Changed
Reviews (1): Last reviewed commit: "chore: add news fragment for #6745" | Re-trigger Greptile |
| > _OFFLOAD_SERIALIZE_THRESHOLD | ||
| ): | ||
| # Pickling large states would stall the event loop. | ||
| pickle_state = await asyncio.to_thread(substate._serialize) |
There was a problem hiding this comment.
When a state's previous pickle is larger than the threshold, this sends the live substate object to a worker thread and yields the event loop. If a background task mutates the same instance while _serialize() is running, Redis can receive a stale or partial snapshot and _was_touched is then cleared, so the interleaved change may not be written on the next flush.
There was a problem hiding this comment.
i'm wondering if this to_thread change makes sense right now. pickle is a CPU-bound operation, moving it to a thread doesn't stop it from blocking the event loop, unless there's some kind of special pre-emption going on.
also, it might be better to pickle all of the states in a thread and return all the results at once to avoid overhead.
There was a problem hiding this comment.
You're right on both counts.
GIL issue: pickle.dumps on typical Python state objects is pure-Python and holds the GIL throughout. asyncio.to_thread schedules work on a thread pool, but the GIL means the event loop thread is still effectively blocked while pickling runs — there's no preemption benefit for CPU-bound work. (The only way to truly unblock the event loop for CPU-bound serialization is ProcessPoolExecutor, but that introduces the race condition raised in my earlier comment even more severely.)
Batching: Even if threading helped, spawning a separate to_thread call per large state is worse than one batched call. The overhead of each thread dispatch adds up.
The simplest and most correct fix is to drop the asyncio.to_thread path entirely and serialize all touched states synchronously before building the pipeline:
| pickle_state = await asyncio.to_thread(substate._serialize) | |
| # Walk the substate tree and collect the states that need writing. | |
| touched_states: list[BaseState] = [] | |
| stack = [base_state] | |
| while stack: | |
| substate = stack.pop() | |
| stack.extend(substate.substates.values()) | |
| if substate._get_was_touched(): | |
| touched_states.append(substate) | |
| if not touched_states: | |
| return | |
| # Serialize all touched states synchronously, then pipeline the writes. | |
| serialized: list[tuple[BaseState, bytes]] = [ | |
| (substate, substate._serialize()) for substate in touched_states | |
| ] | |
| pipeline = self.redis.pipeline() | |
| written_states: list[BaseState] = [] | |
| for substate, pickle_state in serialized: |
This avoids the event loop yield (and thus the race), removes the threshold branching complexity, and is strictly equivalent for the GIL-bound case. If true off-loop serialization becomes a future goal, it would need a ProcessPoolExecutor with a copy/snapshot of the state — not a live reference.
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cd59d2f22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Reset the touched flag so an unchanged state is not re-pickled | ||
| # and re-written by subsequent flushes of the same instance. | ||
| for substate in written_states: | ||
| substate._was_touched = False |
There was a problem hiding this comment.
the _was_touched flag was already being reset by virtue of BaseState.__setstate__ popping the flag out of the payload that gets saved in the pickle, so this is just added overhead now.
All Submissions:
Type of change
Changes To Core Features:
StateManagerRedis.set_staterecursed over the substate tree spawning a task per substate; every recursive call re-GETed the same lock key and re-PTTLed it, and each touched state got its own unpipelined SET — roughly 2N+1 redis round trips per event flush for N states.get_statealready pipelines its reads;set_statenow does the same for writes (ENG-10097):asyncio.to_threadinstead of stalling the event loop (safe: the token's lock is held, so nothing mutates the tree mid-pickle — the old code awaited between pickles too)._get_required_state_classes: the recursive class-level computation ran on everyget_state. It's now anlru_cached module function keyed on(target class, registration context, state hierarchy generation);BaseState.__init_subclass__andreload_state_modulebump the generation, so dynamically created classes and hot reload invalidate cleanly (including same-name class redefinition, where registry sizes don't change)._was_touchedreset after write (mirrors the disk manager's existingget_and_reset_touched_state): previously the flag was never cleared, so a once-touched state kept alive by the oplock cache was re-pickled and re-SET on every subsequent flush. Reset only happens after a successful pipeline execute, so failed writes retry.Round trips per event flush (modeled on the exact before/after command flows; "3" = lock GET + PTTL + one pipelined write):
New unit tests (run against both mock and real redis in CI):
test_set_state_verifies_lock_once_and_pipelines_writescounts actualGET/PTTLcommands during a 3-substate modify (asserts exactly one of each);test_set_state_resets_touched_flagasserts a second flush of an unchanged instance creates no write pipeline at all;test_set_state_offloads_large_picklesasserts theto_threadpath engages above the threshold;test_required_state_classes_cache_invalidationasserts a subclass defined after cache warm-up appears in the cached result via the generation bump. Existingmodify_state/oplock/expiration tests cover end-to-end behavior.Linear: ENG-10097
🤖 Generated with Claude Code
https://claude.ai/code/session_01Sns7EuBPYvfGCxRSRZ1bUx
Generated by Claude Code