perf(state): skip unchanged scalar writes and resolve stale computed vars lazily - #7036
perf(state): skip unchanged scalar writes and resolve stale computed vars lazily#7036FarhanAliRaza wants to merge 4 commits into
Conversation
Benchmarks isolate the per-write cascade cost: many writes with and without dependents, unchanged writes, fan-out and chain shapes, and the clean get_delta floor. Three new unit tests fail on purpose and define the target behavior: an unchanged scalar write must not dirty the var or its dependents, and a computed var that recomputes to an equal value must not cascade. Two more pin the semantics that must survive the change: reads between writes see fresh values, and a getter runs once per read. Claude-Session: https://claude.ai/code/session_01U3KUBonGWkaQm2D7sWKQ5T
__setattr__ now returns early when a scalar base or backend var is assigned the value it already holds, so nothing is marked dirty and nothing is sent. Objects are never compared, so reassigning a mutated object still marks it dirty. Dependent computed vars are marked stale instead of having their cache deleted. A stale var is resolved on read or in get_delta: it checks its recorded dependencies, resolves stale computed dependencies first, and recomputes only when one of them is in dirty_vars. A recomputed value equal to the cached one keeps the cache and stays out of dirty_vars, which stops the cascade at that point. Marking stops at a var that is already stale, so repeated writes in one handler no longer re-walk the dependency graph. Unresolved stale vars drop their cache in _clean. Async computed vars keep the old delete-and-resend behavior because they cannot be resolved on a sync read. is_hydrated is always sent because the client resets it locally on navigation and waits for it to come back. get_delta uses class-level sets for frontend computed vars and for computed vars with an update_interval instead of scanning every computed var on each call. Benchmarks (tests/benchmarks/test_state_dirty.py, median): 200 unchanged writes, 50 dependents 35.8 ms -> 0.33 ms 200 changed writes, 50 dependents 35.9 ms -> 3.0 ms 200 writes, no dependents 2.56 ms -> 2.08 ms chain write + get_delta 1.05 ms -> 0.66 ms Claude-Session: https://claude.ai/code/session_01U3KUBonGWkaQm2D7sWKQ5T
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. |
Merging this PR will improve performance by 4.52%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | test_var_access[non_mutable_scalar] |
61.4 ms | 58.7 ms | +4.52% |
| 🆕 | Simulation | test_chain_write_and_delta |
N/A | 5.2 ms | N/A |
| 🆕 | Simulation | test_fanout_write_and_delta |
N/A | 6.9 ms | N/A |
| 🆕 | Simulation | test_get_delta_clean[ChainState] |
N/A | 109.4 µs | N/A |
| 🆕 | Simulation | test_get_delta_clean[FanOutState] |
N/A | 109.8 µs | N/A |
| 🆕 | Simulation | test_many_unchanged_writes |
N/A | 23.4 ms | N/A |
| 🆕 | Simulation | test_many_writes_no_dependents |
N/A | 17.9 ms | N/A |
| 🆕 | Simulation | test_many_writes_with_fanout |
N/A | 23.4 ms | N/A |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing FarhanAliRaza:dirty-tracking-cutoff (fc47df9) with main (174f2c5)
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 SummaryThe PR optimizes state dirty tracking by skipping unchanged scalar assignments and resolving invalidated cached computed variables lazily.
Confidence Score: 4/5The PR does not appear safe to merge until failed computed-variable recomputation preserves invalidation state instead of allowing an outdated cache to become current.
Files Needing Attention: packages/reflex-base/src/reflex_base/vars/base.py
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/vars/base.py | Adds scalar equality cutoffs and lazy cached-computed-variable recomputation. |
| reflex/state.py | Reworks dependency invalidation, delta generation, cleanup, and scalar assignment dirty tracking around stale computed variables. |
| packages/reflex-base/src/reflex_base/utils/types.py | Reserves the new per-instance stale-computed-variable bookkeeping field. |
| tests/units/test_state.py | Updates dirty-tracking expectations and adds regression coverage for lazy resolution and unchanged values. |
| tests/benchmarks/test_state_dirty.py | Adds benchmarks for repeated assignments and computed-variable dependency topologies. |
Reviews (3): Last reviewed commit: "Drop stale computed var marks when persi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| instance: The state instance holding the stale cache. | ||
| """ | ||
| name = self._name | ||
| instance._stale_computed_vars.discard(name) |
There was a problem hiding this comment.
When a stale computed getter raises, _resolve_stale has already removed its stale marker while leaving the old cache intact, so subsequent reads treat the outdated cached value as current and later deltas can silently omit the dependency change.
Knowledge Base Used:
![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: ef8666bacd
ℹ️ 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".
| forward_deps.append((defining_state_cls.get_full_name(), dvar)) | ||
| cls._computed_var_deps[cvar_name] = tuple(forward_deps) |
There was a problem hiding this comment.
When a cached synchronous computed var calls add_dependency() after class initialization, that method updates _var_dependencies but not this new _computed_var_deps snapshot. A write to the added dependency therefore marks the computed var stale, but _resolve_stale() sees no forward dependencies and returns without recomputing it, leaving the client with the old computed value. Update the forward map when adding a dependency or rebuild it before relying on it.
Useful? React with 👍 / 👎.
| for name in [cvar for cvar in stale if cvar in frontend_computed_vars]: | ||
| computed_vars[name]._resolve_stale(self) |
There was a problem hiding this comment.
When a dependent computed var appears before its dependency in this set snapshot, resolving the dependent recursively resolves and removes the dependency, but the stale snapshot still invokes that dependency again later. Because its base dependency remains dirty, the getter is recomputed twice; this defeats the optimization and can leave non-idempotent or side-effecting computed chains inconsistent, with the dependent based on the first result and the dependency cache holding the second. Check that each snapshot entry is still stale before resolving it.
AGENTS.md reference: AGENTS.md:L43-L47
Useful? React with 👍 / 👎.
| Returns: | ||
| True when both are scalars of the same type and compare equal. | ||
| """ | ||
| return type(new) in _CUTOFF_TYPES and type(old) is type(new) and old == new |
There was a problem hiding this comment.
For float state or computed values, 0.0 == -0.0 even though the sign is observable through serialization, math.copysign, and JavaScript numeric operations. Assigning one over the other now returns early, and a computed var changing only between these values keeps its old cache and stops invalidation, so backend and frontend state can remain incorrect. Exclude opposite-signed zeroes from the unchanged cutoff.
AGENTS.md reference: AGENTS.md:L43-L43
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.
2 issues found across 7 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/state.py">
<violation number="1" location="reflex/state.py:951">
P1: When a cached synchronous `ComputedVar` receives a dependency through `add_dependency()` after class initialization, this reverse map never includes it. The stale resolver retains the old cache and omits the computed value from the delta; update `_computed_var_deps` during dynamic dependency registration.</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:2654">
P2: If a stale computed getter raises, this method has already removed its stale marker, so the cached old value is returned on the next read without retrying. Preserve the stale marker until dependency resolution and recomputation succeed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| cls.get_full_name() | ||
| ) | ||
| forward_deps.append((defining_state_cls.get_full_name(), dvar)) | ||
| cls._computed_var_deps[cvar_name] = tuple(forward_deps) |
There was a problem hiding this comment.
P1: When a cached synchronous ComputedVar receives a dependency through add_dependency() after class initialization, this reverse map never includes it. The stale resolver retains the old cache and omits the computed value from the delta; update _computed_var_deps during dynamic dependency registration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At reflex/state.py, line 951:
<comment>When a cached synchronous `ComputedVar` receives a dependency through `add_dependency()` after class initialization, this reverse map never includes it. The stale resolver retains the old cache and omits the computed value from the delta; update `_computed_var_deps` during dynamic dependency registration.</comment>
<file context>
@@ -916,6 +947,8 @@ def _init_var_dependency_dicts(cls):
cls.get_full_name()
)
+ forward_deps.append((defining_state_cls.get_full_name(), dvar))
+ cls._computed_var_deps[cvar_name] = tuple(forward_deps)
# ComputedVar with cache=False always need to be recomputed
</file context>
| instance: The state instance holding the stale cache. | ||
| """ | ||
| name = self._name | ||
| instance._stale_computed_vars.discard(name) |
There was a problem hiding this comment.
P2: If a stale computed getter raises, this method has already removed its stale marker, so the cached old value is returned on the next read without retrying. Preserve the stale marker until dependency resolution and recomputation succeed.
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 2654:
<comment>If a stale computed getter raises, this method has already removed its stale marker, so the cached old value is returned on the next read without retrying. Preserve the stale marker until dependency resolution and recomputation succeed.</comment>
<file context>
@@ -2594,20 +2616,62 @@ def __get__(self, instance: BaseState | None, owner: type):
+ instance: The state instance holding the stale cache.
+ """
+ name = self._name
+ instance._stale_computed_vars.discard(name)
+ cache_attr = self._cache_attr
+ changed = not hasattr(instance, cache_attr) or self.needs_update(instance)
</file context>
|
this is definitely a behavior change that will affect some apps i've seen in the wild that use self-assignment to manually trigger a cascade of dependent computed vars (which might have dependencies that cannot be tracked by the state system). Obviously the solution for those cases is to just explicitly mark the affected vars dirty, but the API for doing that is not currently very nice. |
Assigning a scalar var the value it already holds is an established way to force a computed var to recompute when it reads something the dependency tracker cannot see. Skipping that write silently broke the idiom, so the write is marked dirty again, and the is_hydrated helper that only existed to bypass the cutoff is gone. The recomputed-value cutoff on computed vars stays: a forced recompute still runs, and a changed result still ships. Claude-Session: https://claude.ai/code/session_01U3KUBonGWkaQm2D7sWKQ5T
|
@masenf droped that. Other changes are worth a look i think. |
A stale mark is only meaningful together with the marks it cascaded to, and those may live in substates that redis persists separately or not at all. On socket connect the root is written with its dynamic route vars already stale, so the next event's cascade stops early and dependents in freshly loaded substates keep their old cached values. Persist the invalidation instead: drop the stale caches at pickle time so a reload recomputes them, and let the persisted dirty_vars re-mark the dependents. Claude-Session: https://claude.ai/code/session_01F1U5DCxRAQ1Xys5GywyNgd
2 participants
Problem
Every
__setattr__on a state var runs the full computed-var invalidation cascade immediately, restarting from the wholedirty_varsset. N assignments in one handler cost N graph walks. There is no equality check, soself.x = self.xdirtiesx, invalidates every dependent computed var, and ships everything. A computed var that recomputes to an equal value still cascades to its dependents and is re-sent.Change
__setattr__. Assigning anint,float,str,bool, orNoneequal to the stored value returns before anything is marked dirty. Objects are never compared, so reassigning a mutated object still marks it dirty._stale_computed_varsset. Marking stops at a var that is already stale, so repeated writes in one handler do not re-walk the graph.get_delta: it resolves stale computed dependencies first, recomputes only when a dependency is indirty_vars, and keeps the cache when the new value is equal. That stops the cascade at that point. Unresolved stale vars drop their cache in_clean, so the next event recomputes them as before.get_delta.is_hydratedis always sent. The client resets it locally on navigation and flushes queued events only when a delta carries it back, so those writes bypass the cutoff.Names already in
dirty_varsare still sent unconditionally, so callers that add a computed var name to force delivery keep working.Benchmarks
New file
tests/benchmarks/test_state_dirty.py. Medians on one machine:Behavior changes a reviewer should check
reset()dirties only vars whose value actually changed. Substates already at defaults no longer appear indirty_substates.cache=Falsevar is still recomputed every delta but only sent when its value changes.Tests
https://claude.ai/code/session_01U3KUBonGWkaQm2D7sWKQ5T