Repository navigation
perf: re-render state readers only when a field they read changes - #7583
FarhanAliRaza wants to merge 2 commits into
Conversation
Each state var hook now names its field, the compiler merges the hooks of a component into one useStateContext call per state with the union of fields, and useStateContext keeps the previous snapshot while those fields are unchanged. A hook without a field list still reads the whole substate.
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. |
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
View guided diff | Re-trigger cubic
[High impact] The latest changes appear safe to merge; the previous build-breaking issue is fixed. SummaryComponents subscribe to the state fields they read, so unrelated field changes can skip renders.
Reviews (2) · Last reviewed commit: "fix: give _var_set_state only a whole-su..." · Reviewed by Greptile |
Its expression is not a field name, so a field hook built from it could hold a ']' the hook merge cannot parse, leaving two declarations of the state.
Merging this PR will not alter performance
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e5cd02fb9
ℹ️ 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".
A/B on real apps: main
|
| Action | main | PR |
|---|---|---|
| events: sync +1 / args +5 / async +10 | 15 each | 3 each |
| events: generator (progress updates) | 92 | 51 |
| events: clear | 15 | 12 |
| data: filter "lamp" | 104 | 17 |
| data: next page | 370 | 282 |
| data: reset | 374 | 287 |
| counter +1 | 3 | 3 |
| forms submit | 236 | 236 |
The counter and forms pages do not change, because their states have only one or two shown fields.
Shop app renders
| Action | main | PR |
|---|---|---|
| change 1 of 20 shown fields | 20 | 1 |
| change a field the echo buttons send but do not show | 4 | 3 |
| change a field only the output shows | 4 | 1 |
On both apps, every final value on the page is the same as on main, and there are no page errors.
React Doctor 0.9.18 (offline, on the compiled .web sources)
| App | main | PR | different findings |
|---|---|---|---|
| shop | 2 | 2 | none |
| playground | 234 | 234 | none |
There is no finding on context-registry.js. The 234 playground findings are already on main: 230 exhaustive-deps on generated useCallback dependency lists that include addEvents and ReflexEvent without need, and 4 prefer-dynamic-import.
|
Possible hook-ordering issue in Compared main Here is the compiler reproducer. Save as from dataclasses import dataclass
from typing import Callable
import reflex as rx
from reflex_base.constants.compiler import MemoizationMode
from reflex_base.plugins import CompileContext, CompilerHooks
from reflex_base.registry import RegistrationContext
from reflex.compiler.plugins import default_page_plugins
from reflex.compiler.compiler import compile_memo_components
@dataclass
class Page:
route: str
component: Callable
title: str = ""
description: str = ""
image: str = ""
meta: tuple = ()
on_load: object = None
_source_module: str | None = None
with RegistrationContext():
class S(rx.State):
a: int = 1
b: int = 2
class ParentHook(rx.el.Div):
_memoization_mode = MemoizationMode(recursive=False)
def add_hooks(self):
return [rx.Var(f"const parentRead = {S.a}", _var_type=int)]
def page():
local = rx._x.client_state(
"childValue", default=S.b, global_ref=False
)
return ParentHook.create(rx.el.div(local, local.value))
ctx = CompileContext(
pages=[Page(route="/p", component=page)],
hooks=CompilerHooks(plugins=default_page_plugins()),
)
with ctx:
ctx.compile()
modules, _ = compile_memo_components(
memos=tuple(ctx.auto_memo_components.values())
)
for _, source in modules:
if "parentRead" in source:
print(source)On the PR, the relevant output is ordered like this (state identifier shortened): const [childValue, setChildvalue] = useState(s.b_rx_state_)
const s = useStateContext("...", ["a_rx_state_", "b_rx_state_"])Main declares The Validation scope: full default Python compiler pipeline plus the generated modules in real React/react-test-renderer 19.2.0, with imports remapped for standalone execution (Emotion |
Follow-up to #7441, which put all substates in one shared store.
Problem
A component that reads one field of a state re-renders when any field of that state changes. A state with 20 fields shown in 20 places re-renders all 20 places when one field changes.
Change
VarData.from_state(state, field)emitsuseStateContext("S", ["field_rx_state_"]). Without a field, the hook has no list, as before._sort_hooksmerges the hooks of a component into oneuseStateContextcall per state, with the union of the fields. A hook without a list reads the whole substate and wins over the field lists.useStateContext(name, fields)keeps the previous snapshot while the listed fields are unchanged, souseSyncExternalStoreskips the render. The store does not change.Var._var_set_stateadds the whole-substate hook, because its JS expression can read any key.Event handlers in the component read the same object. Their arguments are vars of the component, so their fields are in the list and their closures stay current.
Measured
Own small shop app (8 states,
/bigpage with a 20-field state), prod build, React commit hook counting the function components that render per section. main =5ae5dcd528.last_echo(the echo button does not read it)incFinal values are the same on both. No page errors.
Field coverage check on compiled output: for each compiled component, every
stateVar.fieldread is in that component's field list.examples/playground: 216 state hooks, all with a list, no missing fieldHot reload in dev: adding a field, a text and an event to a mounted component shows the new default value, and both the new and the old fields update on click.
Known limit
A hand-written
Varthat reads a state field in raw JS, with noVarData.from_statefor that field, gets no subscription of its own. Before this PR it was correct only when another var of the same state was in the same component. With this PR, that component re-renders only for the fields in its list, so the hand-written read can show a stale value.