perf(events): share one chain per handler and trigger across call sites - #7122
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9103569b3
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Component render, Var collection, and the prop-component scan walked every declared prop through the field descriptor to find the few that are set. Iterate the instance dict plus class-level defaults instead. Cache the literal Var class per exact value type, short-circuit app-wrap dedupe on identity, skip the generic tag protocol for plain tags, and hoist the memoize plugin's component imports. Docs site dry compile (511 pages): 47 s to 40 s. Claude-Session: https://claude.ai/code/session_01PmizE1eQhtYZyVs1RK2ke3
755f84f to
aa338fc
Compare
…d the root news fragment
EventChain.create rebuilt an identical chain for every component that bound the same handler to the same trigger, and the memoize pass then rendered each chain again to name its useCallback wrapper. Intern the chain on the handler keyed by args spec and trigger, and key the wrapper cache by chain identity so repeated call sites reuse the wrapper without rendering. Claude-Session: https://claude.ai/code/session_01PmizE1eQhtYZyVs1RK2ke3
Deep-copying a component walked into the handler's chain cache and copied every chain bound to it. The cache now lives on the RegistrationContext, keyed by handler, args spec and trigger, so handlers carry no state and a forked context starts with its own chains.
Those handlers are fresh copies at every call site, so a cached entry can never be hit again and would only retain the copy. Also add the root news fragment.
aa338fc to
bf0d021
Compare
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_from_event_type[event_handler] |
183.9 µs | 191.1 µs | -3.77% |
| ⚡ | test_compile_page_full_context[_stateful_page] |
69.6 ms | 65.6 ms | +6.04% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing FarhanAliRaza:farhan/event-chain-interning (e014e06) with main (59df268)2
Footnotes
-
9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(0d68f46) during the generation of this report, so 59df268 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
…interning # Conflicts: # packages/reflex-base/news/+compile-prop-hot-paths.performance.md # packages/reflex-base/src/reflex_base/components/component.py # reflex/compiler/plugins/memoize.py # tests/units/components/test_component.py # tests/units/components/test_tag.py
2 participants
Summary
Second of three stacked compile-performance PRs. Stacked on #7121; this diff includes it. Merge #7121 first.
EventChain.createinterns one chain per handler, args spec, and trigger key inRegistrationContext._bound_event_chains, keyed by(id(handler), id(args_spec), key)with the handler and spec held in the value so the ids stay valid. A handler bound to one trigger always produces the same chain, so every call site sharing the handler now shares one instance. Handlers carry no state, so deep-copying a component tree copies only the chains it contains, and a forked context starts with its own chains. Handlers withevent_chain_kwargs, handlers carrying event actions (which are distinctdataclasses.replacecopies), lists, lambdas, andEventSpecvalues are unaffected.get_memoized_event_triggerskeys its wrapper cache by trigger name and chain identity, holding the chain so its id cannot be recycled. The memo name stays content-hashed, so generated output is unchanged.Why
The memoize pass rendered a fresh
LiteralEventChainVarfor every component to name itsuseCallbackwrapper: ~240 µs per chain (LiteralVar.create+str+_get_all_var_data). With a hundred buttons bound to one handler that was the single largest item in the profile.Measurements
Benchmark page with 100 rows bound to one handler (
_repeated_stateful_page, full-context compile): 87.8 ms to 60.7 ms together with #7121, of which most is this change.Docs site dry compile (511 routes, few handlers): 40.0 s (#7121) to 39.3 to 39.6 s.
EventChain.createcalls fell from 11.1k to 4.8k.Micro-benchmarks, one handler bound to 8 triggers across 200 buttons, best of 5:
EventChain.create, repeated call sitecopy.deepcopyof the treeRegistrationContext(this PR)The context-owned cache costs one context-variable lookup per hit. The handler-owned revision let
copy.deepcopywalk into the handler and duplicate every chain bound to it, which scaled with the number of triggers per handler; the context-owned map removes that walk.Design note for review
Call sites that bind the same handler to the same trigger now share one
EventChainobject. Chains are frozen dataclasses and nothing in the framework mutateseventsorevent_actionsin place (all action helpers return replaced copies), but this is a visible change in object identity.Test plan
test_event_chain_cache_lives_on_the_registration_context: a forked context yields its own chain, the parent's chain comes back after the fork exits, and the handler retains no reference to any chain.tests/units/test_event.py,tests/units/reflex_base,tests/units/components, memoize plugin tests green apart from failures that reproduce on cleanmainhere.https://claude.ai/code/session_01PmizE1eQhtYZyVs1RK2ke3