perf(events): share one chain per handler and trigger across call sites - #7122
perf(events): share one chain per handler and trigger across call sites#7122FarhanAliRaza wants to merge 2 commits into
Conversation
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
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
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}
| from reflex_components_core.base.bare import Bare | ||
| from reflex_components_core.core.cond import Cond | ||
| from reflex_components_core.core.match import Match |
There was a problem hiding this comment.
This changes production code in the root reflex package, but the PR only adds news fragments under packages/reflex-base/news. The repository requires a news fragment for every package whose source is changed. Add a root news/ performance fragment or apply the documented skip-changelog waiver before merging.
Context Used: CLAUDE.md (source)
![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".
| from reflex_components_core.base.bare import Bare | ||
| from reflex_components_core.core.cond import Cond | ||
| from reflex_components_core.core.match import Match |
There was a problem hiding this comment.
This commit changes packaged source under reflex/, but both new fragments are under packages/reflex-base/news/. On a PR without the skip-changelog label, the changelog workflow treats reflex and reflex-base as affected packages and the root-package check will fail because news/ has no fragment for this change. Add a corresponding root news/+<slug>.performance.md fragment.
AGENTS.md reference: AGENTS.md:L94-L100
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="reflex/compiler/plugins/memoize.py">
<violation number="1" location="reflex/compiler/plugins/memoize.py:38">
P2: Add a root `news/+<slug>.performance.md` fragment for the changed `reflex` package, or apply the documented `skip-changelog` waiver; otherwise the changelog check fails.</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/registry.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/registry.py:73">
P2: The new `_memoized_event_triggers` cache grows unboundedly over the lifetime of the process-wide RegistrationContext and is never invalidated. Each compile/recompile memoizes chains under fresh `id(event)` keys and holds strong references to the chain and its memo Var; nothing clears the dict (`fork()` omits it and `clear_hash_caches()` doesn't touch it), so dev-server reloads accumulate stale entries and retained VarData/memo vars across the app lifetime. Add an invalidation point (e.g. clear it in the same place `clear_hash_caches()` runs on compile, or on `fork()`), or bound entries that no longer match the live chain.</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/vars/base.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/vars/base.py:132">
P2: When an ABC-backed `python_types` registration changes after the first lookup, this returns a stale literal class for that concrete type. Avoid caching checks whose `isinstance` result can change, or provide invalidation for ABC/protocol registrations.</violation>
<violation number="2" location="packages/reflex-base/src/reflex_base/vars/base.py:145">
P2: Each new concrete value type is retained permanently by this global cache, including types with no literal handler. Use weak type keys or another bounded/lifecycle-aware cache so dynamically generated classes do not accumulate during a long-lived compile process.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -35,6 +35,9 @@ | |||
| from reflex_base.constants.compiler import MemoizationDisposition | |||
There was a problem hiding this comment.
P2: Add a root news/+<slug>.performance.md fragment for the changed reflex package, or apply the documented skip-changelog waiver; otherwise the changelog check fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At reflex/compiler/plugins/memoize.py, line 38:
<comment>Add a root `news/+<slug>.performance.md` fragment for the changed `reflex` package, or apply the documented `skip-changelog` waiver; otherwise the changelog check fails.</comment>
<file context>
@@ -35,6 +35,9 @@
from reflex_base.constants.compiler import MemoizationDisposition
from reflex_base.plugins import ComponentAndChildren, PageContext
from reflex_base.plugins.base import Plugin
+from reflex_components_core.base.bare import Bare
+from reflex_components_core.core.cond import Cond
+from reflex_components_core.core.match import Match
</file context>
| repr=False, | ||
| ) | ||
| _app: App | None = dataclasses.field(default=None, repr=False) | ||
| _memoized_event_triggers: dict[tuple[str, int], tuple[Any, Var]] = ( |
There was a problem hiding this comment.
P2: The new _memoized_event_triggers cache grows unboundedly over the lifetime of the process-wide RegistrationContext and is never invalidated. Each compile/recompile memoizes chains under fresh id(event) keys and holds strong references to the chain and its memo Var; nothing clears the dict (fork() omits it and clear_hash_caches() doesn't touch it), so dev-server reloads accumulate stale entries and retained VarData/memo vars across the app lifetime. Add an invalidation point (e.g. clear it in the same place clear_hash_caches() runs on compile, or on fork()), or bound entries that no longer match the live chain.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/registry.py, line 73:
<comment>The new `_memoized_event_triggers` cache grows unboundedly over the lifetime of the process-wide RegistrationContext and is never invalidated. Each compile/recompile memoizes chains under fresh `id(event)` keys and holds strong references to the chain and its memo Var; nothing clears the dict (`fork()` omits it and `clear_hash_caches()` doesn't touch it), so dev-server reloads accumulate stale entries and retained VarData/memo vars across the app lifetime. Add an invalidation point (e.g. clear it in the same place `clear_hash_caches()` runs on compile, or on `fork()`), or bound entries that no longer match the live chain.</comment>
<file context>
@@ -69,6 +70,9 @@ class RegistrationContext(BaseContext):
repr=False,
)
_app: App | None = dataclasses.field(default=None, repr=False)
+ _memoized_event_triggers: dict[tuple[str, int], tuple[Any, Var]] = (
+ dataclasses.field(default_factory=dict, repr=False)
+ )
</file context>
| """ | ||
| value_type = type(value) | ||
| try: | ||
| return _literal_var_by_type[value_type] |
There was a problem hiding this comment.
P2: When an ABC-backed python_types registration changes after the first lookup, this returns a stale literal class for that concrete type. Avoid caching checks whose isinstance result can change, or provide invalidation for ABC/protocol registrations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/vars/base.py, line 132:
<comment>When an ABC-backed `python_types` registration changes after the first lookup, this returns a stale literal class for that concrete type. Avoid caching checks whose `isinstance` result can change, or provide invalidation for ABC/protocol registrations.</comment>
<file context>
@@ -114,6 +113,37 @@ class VarSubclassEntry:
+ """
+ value_type = type(value)
+ try:
+ return _literal_var_by_type[value_type]
+ except KeyError:
+ pass
</file context>
| ) | ||
| # A class object's type is its metaclass, which other classes share. | ||
| if not isinstance(value, type): | ||
| _literal_var_by_type[value_type] = literal_subclass |
There was a problem hiding this comment.
P2: Each new concrete value type is retained permanently by this global cache, including types with no literal handler. Use weak type keys or another bounded/lifecycle-aware cache so dynamically generated classes do not accumulate during a long-lived compile process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/vars/base.py, line 145:
<comment>Each new concrete value type is retained permanently by this global cache, including types with no literal handler. Use weak type keys or another bounded/lifecycle-aware cache so dynamically generated classes do not accumulate during a long-lived compile process.</comment>
<file context>
@@ -114,6 +113,37 @@ class VarSubclassEntry:
+ )
+ # A class object's type is its metaclass, which other classes share.
+ if not isinstance(value, type):
+ _literal_var_by_type[value_type] = literal_subclass
+ return literal_subclass
</file context>
1 participant
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 on the handler object (handler.__dict__["_bound_chains"], alongside the existing__parametersslot). A handler bound to one trigger always produces the same chain, so every call site sharing the handler now shares one instance. 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.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
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