Reduce MutableProxy per-element overhead and cache per-field proxies#6740
Reduce MutableProxy per-element overhead and cache per-field proxies#6740Alek99 wants to merge 4 commits into
Conversation
_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
Greptile SummaryThis PR reduces overhead in mutable state proxy handling. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "Declare _mutable_proxy_cache as a reserv..." | Re-trigger Greptile |
Merging this PR will improve performance by ×3.2
Performance Changes
Tip Curious why this is faster? Comment Comparing Footnotes
|
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".
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
| ): | ||
| # 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
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
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