perf(compile): read only set props and cache literal Var dispatch - #7121
perf(compile): read only set props and cache literal Var dispatch#7121FarhanAliRaza wants to merge 1 commit into
Conversation
Component render, Var collection, and the prop-component scan walked every declared prop through the field descriptor to find the few that are set. Iterate the instance dict plus class-level defaults instead. Cache the literal Var class per exact value type, short-circuit app-wrap dedupe on identity, skip the generic tag protocol for plain tags, and hoist the memoize plugin's component imports. Docs site dry compile (511 pages): 47 s to 40 s. Claude-Session: https://claude.ai/code/session_01PmizE1eQhtYZyVs1RK2ke3
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 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="packages/reflex-base/src/reflex_base/components/component.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/components/component.py:1480">
P2: When a child renderer returns a `dict` subclass or another value handled specially by `render_prop`, the plain-`Tag` fast path bypasses that protocol and changes the rendered children. Apply `render_prop` to `children` here, matching `Tag.__iter__` and preserving the no-output-change contract.</violation>
</file>
<file name="tests/units/reflex_base/vars/test_base.py">
<violation number="1" location="tests/units/reflex_base/vars/test_base.py:337">
P3: This test permanently mutates module-level global state that outlives the test: `serializers.serializer` writes into SERIALIZERS/SERIALIZER_TYPES, and defining LiteralCoordinateVar registers it in `_var_literal_subclasses` and `_literal_var_by_type`. The types are test-local so nothing collides today, but the module-level caches retain the test classes forever and the test becomes order-dependent as soon as any other test uses a `Coordinate`-typed value or the literal registry. Unregister the serializer and remove the literal subclass (or add an autouse cleanup) so the test leaves global state as it found it.</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:131">
P2: When a literal-supported value has an unhashable metaclass, `type(value)` is unhashable and this lookup raises `TypeError` before literal dispatch runs. Catch `TypeError` during lookup and skip caching unhashable types.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (name := render_prop(tag.name)) is not None: | ||
| rendered_dict["name"] = name | ||
| rendered_dict["props"] = tag.format_props() | ||
| rendered_dict["children"] = children |
There was a problem hiding this comment.
P2: When a child renderer returns a dict subclass or another value handled specially by render_prop, the plain-Tag fast path bypasses that protocol and changes the rendered children. Apply render_prop to children here, matching Tag.__iter__ and preserving the no-output-change contract.
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/components/component.py, line 1480:
<comment>When a child renderer returns a `dict` subclass or another value handled specially by `render_prop`, the plain-`Tag` fast path bypasses that protocol and changes the rendered children. Apply `render_prop` to `children` here, matching `Tag.__iter__` and preserving the no-output-change contract.</comment>
<file context>
@@ -1438,11 +1471,15 @@ def render(self) -> dict:
+ if (name := render_prop(tag.name)) is not None:
+ rendered_dict["name"] = name
+ rendered_dict["props"] = tag.format_props()
+ rendered_dict["children"] = children
+ else:
+ rendered_dict = dict(tag.set(children=children))
</file context>
| rendered_dict["children"] = children | |
| rendered_dict["children"] = render_prop(children) |
| try: | ||
| return _literal_var_by_type[value_type] | ||
| except KeyError: | ||
| pass | ||
| literal_subclass = next( | ||
| ( | ||
| literal | ||
| for literal, var_subclass in reversed(_var_literal_subclasses) | ||
| if isinstance(value, var_subclass.python_types) | ||
| ), | ||
| None, | ||
| ) | ||
| # A class object's type is its metaclass, which other classes share. | ||
| if not isinstance(value, type): | ||
| _literal_var_by_type[value_type] = literal_subclass | ||
| return literal_subclass |
There was a problem hiding this comment.
P2: When a literal-supported value has an unhashable metaclass, type(value) is unhashable and this lookup raises TypeError before literal dispatch runs. Catch TypeError during lookup and skip caching unhashable types.
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 131:
<comment>When a literal-supported value has an unhashable metaclass, `type(value)` is unhashable and this lookup raises `TypeError` before literal dispatch runs. Catch `TypeError` during lookup and skip caching unhashable types.</comment>
<file context>
@@ -114,6 +113,37 @@ class VarSubclassEntry:
+ The matching literal class, or None if no registered class claims it.
+ """
+ value_type = type(value)
+ try:
+ return _literal_var_by_type[value_type]
+ except KeyError:
</file context>
| try: | |
| return _literal_var_by_type[value_type] | |
| except KeyError: | |
| pass | |
| literal_subclass = next( | |
| ( | |
| literal | |
| for literal, var_subclass in reversed(_var_literal_subclasses) | |
| if isinstance(value, var_subclass.python_types) | |
| ), | |
| None, | |
| ) | |
| # A class object's type is its metaclass, which other classes share. | |
| if not isinstance(value, type): | |
| _literal_var_by_type[value_type] = literal_subclass | |
| return literal_subclass | |
| try: | |
| return _literal_var_by_type[value_type] | |
| except (KeyError, TypeError): | |
| pass | |
| literal_subclass = next( | |
| ( | |
| literal | |
| for literal, var_subclass in reversed(_var_literal_subclasses) | |
| if isinstance(value, var_subclass.python_types) | |
| ), | |
| None, | |
| ) | |
| # A class object's type is its metaclass, which other classes share. | |
| if not isinstance(value, type): | |
| try: | |
| _literal_var_by_type[value_type] = literal_subclass | |
| except TypeError: | |
| pass | |
| return literal_subclass |
|
|
||
| from reflex_base.utils import serializers | ||
|
|
||
| @serializers.serializer |
There was a problem hiding this comment.
P3: This test permanently mutates module-level global state that outlives the test: serializers.serializer writes into SERIALIZERS/SERIALIZER_TYPES, and defining LiteralCoordinateVar registers it in _var_literal_subclasses and _literal_var_by_type. The types are test-local so nothing collides today, but the module-level caches retain the test classes forever and the test becomes order-dependent as soon as any other test uses a Coordinate-typed value or the literal registry. Unregister the serializer and remove the literal subclass (or add an autouse cleanup) so the test leaves global state as it found it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/units/reflex_base/vars/test_base.py, line 337:
<comment>This test permanently mutates module-level global state that outlives the test: `serializers.serializer` writes into SERIALIZERS/SERIALIZER_TYPES, and defining LiteralCoordinateVar registers it in `_var_literal_subclasses` and `_literal_var_by_type`. The types are test-local so nothing collides today, but the module-level caches retain the test classes forever and the test becomes order-dependent as soon as any other test uses a `Coordinate`-typed value or the literal registry. Unregister the serializer and remove the literal subclass (or add an autouse cleanup) so the test leaves global state as it found it.</comment>
<file context>
@@ -246,3 +257,114 @@ def __hash__(cls) -> int:
+
+ from reflex_base.utils import serializers
+
+ @serializers.serializer
+ def serialize_coordinate(value: Coordinate) -> str:
+ """Serialize a coordinate.
</file context>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from reflex_components_core.base.bare import Bare | ||
| from reflex_components_core.core.cond import Cond | ||
| from reflex_components_core.core.match import Match |
There was a problem hiding this comment.
This changes source in the root reflex package, but the only added news fragment belongs to reflex-base. The repository requires a news fragment for every package whose source is touched, so root-package changelog handling must be added or explicitly waived before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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: 870787ab22
ℹ️ 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".
| from reflex_components_core.base.bare import Bare | ||
| from reflex_components_core.core.cond import Cond | ||
| from reflex_components_core.core.match import Match |
There was a problem hiding this comment.
This modifies the main reflex package by changing reflex/compiler/plugins/memoize.py, but the commit adds a fragment only under packages/reflex-base/news/. Add a corresponding root news/ performance fragment; otherwise the repository's per-package changelog requirement is not satisfied.
AGENTS.md reference: AGENTS.md:L96-L100
Useful? React with 👍 / 👎.
| if prop in values: | ||
| yield prop, values[prop] |
There was a problem hiding this comment.
When a custom Component overrides an inherited JavaScript field with a property or data descriptor whose setter stores the raw value under the same name in __dict__, this branch returns that raw value and bypasses the descriptor getter. The previous unconditional getattr(self, prop) returned the getter's transformed value, so rendering, Var collection, and component-in-prop discovery can now all use the wrong value; retain descriptor-aware access for such class attributes while keeping the direct-dict fast path for ordinary fields.
AGENTS.md reference: AGENTS.md:L43-L43
Useful? React with 👍 / 👎.
1 participant
Summary
First of three stacked compile-performance PRs. This one is pure hot-path work with no change to generated output.
Component._iter_set_props:_render,_get_vars, and the prop-component scan walked every declared prop (34 to 70 per Radix component) through the field descriptor to find the few that are set. They now iterate the instance dict plus class-level defaults, in declaration order. Literal defaults,@propertyoverrides, and factory defaults still surface._literal_var_for: caches the literal Var class per exact value type instead of copying and walking the twelve-entry isinstance ladder on everyLiteralVar.create. Cleared when a literal subclass registers.insert_app_wrapsshort-circuits on identity before the field-by-field component comparison.Component.renderskips the generic tag protocol for plainTaginstances, andrender_proppasses strings and dicts straight through.cached_propertykeys use a plainobject()instead ofuuid4.Measurements
Docs site (
docs/app, 511 routes),App._compile(dry_run=True)in a fresh process, warm runs:cProfile of the same compile: field-descriptor reads fell from 14.9M to 3.1M calls;
_render-6.2 s,_get_vars-3.1 s,_create_literal_var-5.1 s,Tag.add_props-2.5 s (profiled time).Benchmark pages (
tests/benchmarks/fixtures.py, full-context compile): complicated page 29.3 ms to 25.0 ms.Stack
Each later PR's diff includes the earlier ones; merge in order.
Test plan
render_proppassthrough, literal dispatch invalidation on late registration,cached_propertyidentity, pickle, and release.uv run pytest tests/units/components tests/units/reflex_base tests/units/compiler tests/units/test_event.pygreen apart from failures that reproduce on cleanmainin this environment.https://claude.ai/code/session_01PmizE1eQhtYZyVs1RK2ke3