Conversation
Avoid issue where broader-than-needed SNAPSHOT memoization was occuring when these components should have been memoized separately.
Greptile SummaryThis PR fixes over-broad SNAPSHOT memoization by removing The logic change is correct and the tests are meaningfully updated. Two minor points worth noting: Confidence Score: 4/5Safe to merge — the core logic is correct and the critical invariants (Foreach is SNAPSHOT, parent is PASSTHROUGH) are verified by the updated unit tests. No P0 or P1 findings. Two P2 observations: a missing wrapper-count assertion in the integration test (which means the old broken behaviour would still pass that test), and a private symbol being used across a package boundary without all declaration. Neither blocks correctness. tests/units/compiler/test_memoize_plugin.py — the foreach integration test could be strengthened with a wrapper-count assertion to guard the regression directly. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[enter_component / leave_component] --> B{get_memoization_strategy}
B --> |SNAPSHOT| C{is_snapshot_boundary?}
B --> |PASSTHROUGH| D{_should_memoize?}
C --> |True: recursive=False boundary| E[enter_component: wrap + suppress descendants]
C --> |False: Foreach structural child| F[enter_component: wrap Foreach as SNAPSHOT]
D --> |True: stateful props/triggers| G[leave_component: build passthrough wrapper]
D --> |False: no reactive data| H[leave_component: no-op, skip]
subgraph OLD ["BEFORE"]
direction TB
OLD1["Parent of Foreach → SNAPSHOT\n(whole subtree in one memo)"]
end
subgraph NEW ["AFTER this PR"]
direction TB
NEW1["Parent of Foreach → PASSTHROUGH"]
NEW2["Foreach itself → SNAPSHOT"]
end
style OLD fill:#ffcccc,stroke:#cc0000
style NEW fill:#ccffcc,stroke:#007700
|
Merging this PR will degrade performance by 46.34%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_compile_page_full_context[_stateful_page] |
15.9 ms | 27.5 ms | -42.03% |
| ❌ | test_compile_page_single_pass[_stateful_page] |
13.3 ms | 24.7 ms | -46.34% |
| ❌ | test_compile_single_pass_all_artifacts[_stateful_page] |
13 ms | 23 ms | -43.44% |
Comparing masenf/do-not-memo-foreach-parent (ea54160) with main (a84e29b)
Footnotes
-
2 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. ↩
FarhanAliRaza
left a comment
FarhanAliRaza
left a comment
There was a problem hiding this comment.
We might want to add a full test.
def test_foreach_parent_does_not_absorb_sibling_into_snapshot() -> None:
"""Foreach owns its own snapshot — its parent stays passthrough.
Regression for the foreach-parent memoization fix: previously a parent
component holding a Foreach was promoted to SNAPSHOT, and any sibling
reactive content under that parent was absorbed into a single wide memo
body. The parent should now render on the page side, with the Foreach
and any reactive sibling each getting their own independent wrapper.
"""
from reflex_components_core.core.foreach import Foreach
ctx, _page_ctx = _compile_single_page(
lambda: rx.box(
Bare.create(SpecialFormMemoState.items.length()), # reactive sibling
rx.foreach(
SpecialFormMemoState.items,
lambda item: rx.text(item),
),
)
)
wrapped_types = {
type(defn.component)
for defn in ctx.auto_memo_components.values()
if isinstance(defn, ExperimentalMemoComponentDefinition)
}
# Parent rx.box is NOT memoized — it renders directly on the page side.
from reflex_components_core.radix.themes.layout.box import Box
assert Box not in wrapped_types
# Foreach owns its own SNAPSHOT memo.
assert Foreach in wrapped_types
foreach_defn = next(
defn
for defn in ctx.auto_memo_components.values()
if isinstance(defn, ExperimentalMemoComponentDefinition)
and isinstance(defn.component, Foreach)
)
assert get_memoization_strategy(foreach_defn.component) is MemoizationStrategy.SNAPSHOT
# Sibling Bare gets its own PASSTHROUGH memo, distinct from the foreach
# wrapper — it is not absorbed into the foreach-parent snapshot.
bare_defn = next(
defn
for defn in ctx.auto_memo_components.values()
if isinstance(defn, ExperimentalMemoComponentDefinition)
and isinstance(defn.component, Bare)
)
assert get_memoization_strategy(bare_defn.component) is MemoizationStrategy.PASSTHROUGH
assert bare_defn is not foreach_defn 2 participants
Avoid issue where broader-than-needed SNAPSHOT memoization was occuring when these components should have been memoized separately.
This is likely a benchmark regression, since before the entire stateful_page was getting wrapped up in a single memo component, so most of the compiler tree walk was getting skipped.