Reduce MutableProxy per-element overhead and cache per-field proxies - #6740
Conversation
Greptile SummaryThis PR reduces
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| reflex/istate/proxy.py | Reorders recursive wrapping so immutable values return before the dataclasses frame walk. |
| reflex/state.py | Adds per-field mutable proxy caching with reassignment invalidation and pickle-safe restore behavior. |
| packages/reflex-base/src/reflex_base/utils/types.py | Reserves _mutable_proxy_cache so it is treated as an internal state field. |
| tests/units/istate/test_proxy.py | Adds tests for proxy cache reuse, replacement, pickle behavior, and immutable iteration. |
| tests/units/test_state.py | Updates the state proxy identity expectation for cached reads. |
Reviews (6): Last reviewed commit: "Declare _mutable_proxy_cache as a reserv..." | Re-trigger Greptile
Merging this PR will improve performance by ×3.4
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | test_var_access[mutable_list] |
71.1 ms | 9.1 ms | ×7.9 |
| ⚡ | Simulation | test_var_access[mutable_dict] |
88.4 ms | 19.6 ms | ×4.5 |
| ⚡ | Simulation | test_evaluate_page_with_hooks[_stateful_page] |
5.3 ms | 4.7 ms | +11.71% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/reflex-perf-optimizations-01l7a3-eng-10094 (b5e404b) with claude/reflex-perf-optimizations-01l7a3-eng-10093 (1b480cb)2
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. ↩
-
No successful run was found on
claude/reflex-perf-optimizations-01l7a3-eng-10093(1f6a4f4) during the generation of this report, so 56eab96 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
![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: c7b1bd3c9f
ℹ️ 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".
| ): | ||
| # track changes in mutable containers (list, dict, set, etc) | ||
| return MutableProxy(wrapped=value, state=self, field_name=name) | ||
| cache = super().__getattribute__("__dict__").get("_mutable_proxy_cache") |
There was a problem hiding this comment.
similar to the last PR, unclear on why these cache flags are being directly added into the __dict__ and using object.__setattr__ and the like instead of just defining them as reserved fields on the state.
if we define them as real fields on the state, then users will get an exception if they try to reuse the name, instead of internal tracking machinery just getting broken or corrupting user data.
There was a problem hiding this comment.
Good call — converted in d6ab47c: _mutable_proxy_cache is now a declared field(default_factory=dict, is_var=False) like _backend_vars/_was_touched, added to RESERVED_BACKEND_VAR_NAMES so a user var with that name can't silently collide. It stays out of pickles (__getstate__ pop) and is recreated empty in __setstate__. I'll apply the same treatment to the propagation-frontier fields in #6743.
Generated by Claude Code
d6ab47c to
01ec207
Compare
_wrap_recursive walked 5 stack frames (dataclasses-internal check) for every element retrieved through a proxy, even immutable scalars that never get wrapped. Check is_mutable_type first so immutable elements skip the frame walk entirely. Also cache the MutableProxy built for each mutable state var on the instance, keyed by field name, instead of constructing a fresh proxy on every attribute read. The cache is invalidated by identity when the underlying value is reassigned and is excluded from pickling (copy and deepcopy already go through __getstate__). 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
Address review feedback: drop the _mutable_proxy_cache entry when a base or backend var is assigned, so the replaced value is not kept alive by the cached proxy until the next read. Update test_set_base_field_via_setter for the new cached-proxy identity semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Address review feedback: instead of managing the proxy cache as an ad-hoc instance __dict__ entry, declare it as a real (non-var) field like _backend_vars/_was_touched and add it to RESERVED_BACKEND_VAR_NAMES, so user vars cannot silently collide with the framework's tracking machinery. The cache is still excluded from pickling and recreated empty on unpickle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtmC37Y8kJ5S6
1b480cb to
1f6a4f4
Compare
01ec207 to
b5e404b
Compare
|
Folded into #6743, which now spans the full state hot-path set — this PR, #6738, and the dirty-propagation frontier all edit the same functions, so they're easier to review as one final shape. Commits are preserved there per original PR. The feedback from this thread is already in those commits: |
3 participants
Linear: ENG-10094
Description
MutableProxy._wrap_recursivewalked 5 stack frames (_is_called_from_dataclasses_internal) for every element retrieved through a proxy, before the cheapis_mutable_typecheck that usually decides nothing needs wrapping. The checks are reordered so immutable elements (the common case: ints, strings in a proxied list) skip the frame walk entirely. The dataclasses-internal check still runs for mutable values, preservingdataclasses.asdict/astuplebehavior.BaseState._get_attributeconstructed a freshMutableProxyon every read of a mutable state var. The proxy is now cached per instance and field name, invalidated by identity when the underlying value is reassigned. The cache is excluded from pickling (__getstate__), andcopy/deepcopyalready round-trip through__getstate__, so copies never share or carry proxies.One deliberate nuance: when
_wrap_recursivereceives an already-proxied value and is called from dataclasses internals, it now returns the unwrapped value (previously the still-wrapped proxy), matching the documented intent of that path.Benchmarks (GitHub Actions runner, run, 2 passes each)
cProfile (20 iterations + 2000 attr reads):
_is_called_from_dataclasses_internal(20,000 calls, 0.028s cumulative) disappears from the profile; total function calls drop from 168k to 72k.Type of change
Changes To Core Features:
tests/units/istate/test_proxy.py: proxy reused across reads and invalidated on reassignment; cache never serialized (pickle round-trip rebinds proxies to the restored state); iteration yields plain immutables while mutable elements stay wrapped.🤖 Generated with Claude Code
https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Generated by Claude Code