Fast-path framework bookkeeping fields in state attribute access - #6757
Fast-path framework bookkeeping fields in state attribute access#6757Alek99 wants to merge 4 commits into
Conversation
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | test_var_access[non_mutable_scalar] |
58.5 ms | 66 ms | -11.31% |
| ⚡ | Simulation | test_state_tree_delta[depth_10] |
2.8 ms | 1.9 ms | +45.83% |
| ⚡ | Simulation | test_proxy_mapping_mutation |
6.4 ms | 4.4 ms | +44.55% |
| ⚡ | Simulation | test_computed_dependency_fanout[0] |
364.2 µs | 253.7 µs | +43.55% |
| ⚡ | Simulation | test_state_tree_delta[three_by_three] |
2.5 ms | 1.8 ms | +40.43% |
| ⚡ | Simulation | test_state_tree_delta[width_10] |
2.1 ms | 1.5 ms | +37.67% |
| ⚡ | Simulation | test_dirty_computed_var_propagation[100] |
8.3 ms | 6.2 ms | +33.21% |
| ⚡ | Simulation | test_proxy_list_mutation |
7.8 ms | 5.9 ms | +33.17% |
| ⚡ | Simulation | test_dirty_computed_var_propagation[10] |
1,233.9 µs | 955.9 µs | +29.08% |
| ⚡ | Simulation | test_computed_dependency_fanout[2] |
602.6 µs | 481.6 µs | +25.13% |
| ⚡ | Simulation | test_state_delta_scalar_mutation[10] |
659.1 µs | 535 µs | +23.19% |
| ⚡ | Simulation | test_dirty_computed_var_propagation[1] |
535.3 µs | 440.2 µs | +21.61% |
| ⚡ | Simulation | test_state_delta_scalar_mutation[100] |
730.8 µs | 602.9 µs | +21.22% |
| ⚡ | Simulation | test_state_manager_memory_cold_get |
430.1 µs | 358.3 µs | +20.05% |
| ⚡ | Simulation | test_process_event_burst_independent_tokens[100] |
212.5 ms | 181.5 ms | +17.07% |
| ⚡ | Simulation | test_process_event_burst_independent_tokens[10] |
21.7 ms | 18.6 ms | +16.62% |
| ⚡ | Simulation | test_computed_dependency_fanout[10] |
1.5 ms | 1.3 ms | +13.71% |
| ⚡ | Simulation | test_process_event_burst_independent_tokens[1] |
2.7 ms | 2.4 ms | +13.27% |
| ⚡ | Simulation | test_process_event_warm |
2.7 ms | 2.4 ms | +13.14% |
| ⚡ | Simulation | test_process_event_burst_same_token[1] |
2.7 ms | 2.4 ms | +13.13% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/reflex-perf-optimizations-01l7a3-eng-10096 (932c1fd) with agent/event-loop-performance-benchmarks (b938b34)
Footnotes
-
8 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. ↩
Greptile SummaryThis PR speeds up state attribute access and repeated state serialization work. The main changes are:
Confidence Score: 5/5No additional blocking issue qualifies for this follow-up review.
|
| Filename | Overview |
|---|---|
| reflex/state.py | Adds bookkeeping attribute fast paths, cached state metadata, and invalidation hooks. |
| tests/units/test_state.py | Adds tests for per-class caches, computed-variable filtering, and bookkeeping field access. |
| news/6757.performance.md | Documents the state attribute-access and serialization performance improvements. |
Reviews (5): Last reviewed commit: "Trigger CI on rebased branch" | Re-trigger Greptile
![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: 483d2d1253
ℹ️ 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".
| for substate_class in cls.get_substates(): | ||
| substate_class.vars.setdefault(name, var) | ||
| # inherited_vars may alias this class's vars dict. | ||
| substate_class._skip_var_names = None |
There was a problem hiding this comment.
When add_var() is called on a state that already has nested substates, this clears the cache only for immediate children. Grandchildren have their inherited_vars updated indirectly because it aliases the child vars dict, but their _skip_var_names frozenset was already populated during class creation and remains stale, so Grandchild.get_skip_vars() omits the newly inherited dynamic var. Please invalidate descendants as well when propagating a dynamically added var.
Useful? React with 👍 / 👎.
97e9e37 to
1ded8bd
Compare
fbf0b27 to
15bdf67
Compare
1ded8bd to
b8dc9e0
Compare
_get_attribute was 43% of profiled event-handling time; much of it was the framework's own reads of dirty_vars, parent_state, substates, and _backend_vars paying the full inherited-vars/event-handler/proxying chain on every access. - Add those names (and dirty_substates) to the CLASS_VAR_NAMES fast path; they are never state vars, never inherited, and never proxied. - Check base_vars/backend_vars membership before is_mutable_type when deciding whether to proxy, so non-var values skip the type check. - Cache get_skip_vars() per class (invalidated when inherited_vars changes) instead of rebuilding the set on every internal write. - Cache the frontend computed var names used by get_delta per class (invalidated when computed_vars changes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
15bdf67 to
b938b34
Compare
b8dc9e0 to
88919b5
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 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="tests/units/test_state.py">
<violation number="1" location="tests/units/test_state.py:1528">
P3: The new caching tests (test_get_skip_vars_cached_per_class, test_frontend_computed_var_names_cached_per_class) verify the cache populates and is per-class, but never verify invalidation, even though the PR calls out that get_skip_vars must be invalidated on add_var/_update_substate_inherited_vars and frontend computed var names on computed_vars change. A test that calls get_skip_vars, then add_var, and re-checks would guard the invalidation paths that the caching optimization depends on.</violation>
</file>
<file name="reflex/state.py">
<violation number="1" location="reflex/state.py:1301">
P2: When `add_var` adds a field to a state with grandchildren, the grandchildren keep a stale `_skip_var_names` cache because this invalidation covers only direct children. Their aliased `inherited_vars` contains the new field, but `get_skip_vars()` does not, so proxy assignment can take the wrong immutable/non-persisted path. Invalidate skip caches recursively, or reuse `_update_substate_inherited_vars` for this update.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| for substate_class in cls.get_substates(): | ||
| substate_class.vars.setdefault(name, var) | ||
| # inherited_vars may alias this class's vars dict. | ||
| substate_class._skip_var_names = None |
There was a problem hiding this comment.
P2: When add_var adds a field to a state with grandchildren, the grandchildren keep a stale _skip_var_names cache because this invalidation covers only direct children. Their aliased inherited_vars contains the new field, but get_skip_vars() does not, so proxy assignment can take the wrong immutable/non-persisted path. Invalidate skip caches recursively, or reuse _update_substate_inherited_vars for this update.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At reflex/state.py, line 1301:
<comment>When `add_var` adds a field to a state with grandchildren, the grandchildren keep a stale `_skip_var_names` cache because this invalidation covers only direct children. Their aliased `inherited_vars` contains the new field, but `get_skip_vars()` does not, so proxy assignment can take the wrong immutable/non-persisted path. Invalidate skip caches recursively, or reuse `_update_substate_inherited_vars` for this update.</comment>
<file context>
@@ -1262,6 +1297,8 @@ def add_var(cls, name: str, type_: Any, default_value: Any = None):
for substate_class in cls.get_substates():
substate_class.vars.setdefault(name, var)
+ # inherited_vars may alias this class's vars dict.
+ substate_class._skip_var_names = None
# Reinitialize dependency tracking dicts.
</file context>
| child_skip = SkipVarsChildState.get_skip_vars() | ||
| assert child_skip is not parent_skip | ||
| assert "p" in child_skip | ||
| assert "p" not in parent_skip |
There was a problem hiding this comment.
P3: The new caching tests (test_get_skip_vars_cached_per_class, test_frontend_computed_var_names_cached_per_class) verify the cache populates and is per-class, but never verify invalidation, even though the PR calls out that get_skip_vars must be invalidated on add_var/_update_substate_inherited_vars and frontend computed var names on computed_vars change. A test that calls get_skip_vars, then add_var, and re-checks would guard the invalidation paths that the caching optimization depends on.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/units/test_state.py, line 1528:
<comment>The new caching tests (test_get_skip_vars_cached_per_class, test_frontend_computed_var_names_cached_per_class) verify the cache populates and is per-class, but never verify invalidation, even though the PR calls out that get_skip_vars must be invalidated on add_var/_update_substate_inherited_vars and frontend computed var names on computed_vars change. A test that calls get_skip_vars, then add_var, and re-checks would guard the invalidation paths that the caching optimization depends on.</comment>
<file context>
@@ -1510,6 +1510,57 @@ def timed(self) -> int:
+ child_skip = SkipVarsChildState.get_skip_vars()
+ assert child_skip is not parent_skip
+ assert "p" in child_skip
+ assert "p" not in parent_skip
+
+
</file context>
2 participants
Linear: ENG-10096
Description
_get_attributewas ~43% of profiled event-handling time, and much of that was the framework's own reads ofdirty_vars,parent_state,substates, and_backend_varspaying the full inherited-vars/event-handler/proxying chain on every access from_mark_dirty/_clean/get_delta.dirty_substates) to theCLASS_VAR_NAMESfast path. They are never state vars, never inherited, and were never proxied (not inbase_vars/backend_vars), so resolution viaobject.__getattribute__is behavior-identical; assignments already ended inobject.__setattr__with no dirty-marking.base_vars/backend_varsmembership is checked beforeis_mutable_type, sparing non-var values the type check.get_skip_vars()per class as a frozenset, invalidated wheninherited_varschanges (_update_substate_inherited_vars,add_var).get_deltaper class, invalidated whencomputed_varschanges (_evaluate, dynamic route vars).Benchmarks (GitHub Actions runner, run, 2 passes each)
cProfile (5000 bookkeeping reads + 500 event cycles):
_get_attributetottime drops 0.120s -> 0.061s; total function calls drop 256k -> 207k.Type of change
Changes To Core Features:
test_get_skip_vars_cached_per_class: per-class caching with inherited vars included and no cross-class leakage.test_frontend_computed_var_names_cached_per_class: backend computed vars excluded; cached identity.test_framework_bookkeeping_fields_not_proxied: bookkeeping containers stay raw while real state vars still get proxied.🤖 Generated with Claude Code
https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Generated by Claude Code