Only propagate newly-dirty vars in _mark_dirty_computed_vars#6743
Only propagate newly-dirty vars in _mark_dirty_computed_vars#6743Alek99 wants to merge 5 commits into
Conversation
Every setattr and proxied mutation triggered a full dirty-propagation pass: an expiry scan over all computed vars plus a dependency re-walk of the entire accumulated dirty_vars set. - Precompute per class which computed vars have an update interval, so the expiry scan is skipped entirely for the common case of none. - Track a per-instance "propagated frontier" so each propagation only processes vars whose dependency closure has not been walked yet. Recomputing a cached var re-materializes its cache, which a later mutation of its dependencies must invalidate again, so recomputes bump a generation counter that resets the frontier. The frontier is transient: cleared by _clean and excluded from pickling. 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 makes computed-var dirty propagation do less repeated work. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "Write propagation generation via __dict_..." | Re-trigger Greptile |
Merging this PR will improve performance by 7.01%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_process_event |
7.6 ms | 7.1 ms | +7.01% |
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-10095 (0c44576) with main (32cc257)
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. ↩
| current_gen = reflex_base_vars_base._computed_var_recompute_generation | ||
| if propagated is None: | ||
| propagated = set() | ||
| object.__setattr__(self, "_propagated_dirty_vars", propagated) |
There was a problem hiding this comment.
if we already have the instance_dict, why not just shove the value in there?
| self.dirty_vars = set() | ||
| self.dirty_substates = set() | ||
| # Discard the propagation frontier along with the dirty vars it tracked. | ||
| self.__dict__.pop("_propagated_dirty_vars", None) |
There was a problem hiding this comment.
why are we treating these new fields like this and not defining them as normal fields in a BaseState, like all the other fields
Match review feedback on the sibling PR: declare _propagated_dirty_vars and _propagated_generation as real (non-var) fields and reserve their names, so user vars cannot silently collide with the framework's tracking machinery. Still transient: excluded from pickling and recreated on unpickle. 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
Background task handlers run state methods with self bound to a StateProxy; wrapt's C ObjectProxy (used on Python <= 3.12) rejects object.__setattr__ on the proxy, while __dict__ resolves to the wrapped state's dict on both proxies and plain states. Fixes test_background_task_* failures on 3.10-3.12. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Linear: ENG-10095
Description
Every
setattrand every proxiedappend/__setitem__triggered a full dirty-propagation pass: aneeds_updatescan over all computed vars (interval expiry) plus a dependency re-walk of the entire accumulateddirty_varsset, not just the newly dirtied var._interval_computed_vars, refreshed whenevercomputed_varschanges), so the expiry scan is skipped entirely for the overwhelmingly common case of none.Correctness: re-walking the dirty set is only observably needed after a cached computed var is recomputed mid-cycle (the recompute re-materializes a cache that a later mutation of its dependencies must invalidate again — including across states). Recomputes therefore bump a process-wide generation counter, and a generation mismatch resets the frontier, forcing the same full re-walk as today. The frontier is cleared by
_cleanand excluded from pickling, so restored/copied states always start with a full walk.Benchmarks (GitHub Actions runner, run, 2 passes each; state with 2 cached computed vars)
cProfile (2000 proxied appends): total function calls drop 216k -> 78k;
_mark_dirty_computed_varscumulative time drops 0.102s -> 0.019s and_expired_computed_varsdisappears from the profile. The benchmark script also spot-checks that computed vars still recompute correctly after the mutation loop.Type of change
Changes To Core Features:
test_computed_var_recompute_after_mid_cycle_read: mutate -> read (recompute) -> mutate again -> the cache is invalidated again (the exact staleness hazard a naive frontier would introduce).test_computed_var_recompute_after_mid_cycle_read_across_states: same hazard across parent/child states.test_interval_computed_vars_precomputed: per-class interval var precomputation.Note: this touches the same
ComputedVar.__get__region as #6738; whichever merges second needs a trivial rebase.🤖 Generated with Claude Code
https://claude.ai/code/session_01G8cXh3TUbNtbjm2ERqE62X
Generated by Claude Code