Explicit state and event id minification - #6100
Conversation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Merging this PR will improve performance by 14.19%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | test_from_event_type[event_spec] |
49.2 µs | 40.5 µs | +21.46% |
| ⚡ | Simulation | test_from_event_type[lambda_event_spec] |
51.1 µs | 42.7 µs | +19.62% |
| ⚡ | Simulation | test_from_event_type[event_handler] |
91.9 µs | 84.9 µs | +8.31% |
| ⚡ | Simulation | test_from_event_type[lambda_event_handler] |
92.8 µs | 85.9 µs | +8.04% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing benedikt-bartscher:explicit-event-id-minification (d5107c1) with main (9f7f82c)
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. ↩
|
TODO: best-effort-mode
a similar thing can be implemented for event handler id's edit: not that easy, as state names are baked into to many things (events, vars etc). we would need to defer init_subclass and possibly parts of @rx.var and @rx.event edit2: i have found a better approach with a minify.json which makes this obsolete |
|
Open for discussion: Should reflex reserve the first 5/10 state id's for internal states? Current reflex uses 0-3, but that might change in the future. edit: with sibling uniqueness we would just need to reserve the state id's on the first rx.State subclass level. edit2: i have found a better approach with a minify.json which makes this obsolete |
|
TODO: think about sibling uniqueness for states. currently using global uniqueness edit: adjusted in 6b4c5fa |
35ad1c6 to
1647c1e
Compare
|
Idea: store ids in a minify.json file
|
When a parent and child state have the same minified name, substate resolution can fail because the leading segment is stripped incorrectly. This change adds a flag to skip stripping only on the initial recursive call, ensuring correct resolution even in name collision scenarios.
…stay reserved per sibling group
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af4219fac8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 16 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/utils/format.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/utils/format.py:489">
P1: When forked registration contexts use different name resolvers, this handler-level cache leaks the formatted event name between apps. Key the cache by registration context/resolver, or remove the instance cache so each context resolves its own event name.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| cached = handler.__dict__.get(_FORMATTED_NAME_CACHE_ATTR) | ||
| if cached is not None: | ||
| return cached | ||
| state, name = get_event_handler_parts(handler) | ||
| if state == "": | ||
| return name | ||
| return f"{state}.{name}" | ||
| full = name if state == "" else f"{state}.{name}" | ||
| object.__setattr__(handler, _FORMATTED_NAME_CACHE_ATTR, full) | ||
| return full |
There was a problem hiding this comment.
P1: When forked registration contexts use different name resolvers, this handler-level cache leaks the formatted event name between apps. Key the cache by registration context/resolver, or remove the instance cache so each context resolves its own event name.
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/utils/format.py, line 489:
<comment>When forked registration contexts use different name resolvers, this handler-level cache leaks the formatted event name between apps. Key the cache by registration context/resolver, or remove the instance cache so each context resolves its own event name.</comment>
<file context>
@@ -444,43 +444,55 @@ def format_props(*single_props, **key_value_props) -> list[str]:
Returns:
The formatted function.
"""
+ cached = handler.__dict__.get(_FORMATTED_NAME_CACHE_ATTR)
+ if cached is not None:
+ return cached
</file context>
| cached = handler.__dict__.get(_FORMATTED_NAME_CACHE_ATTR) | |
| if cached is not None: | |
| return cached | |
| state, name = get_event_handler_parts(handler) | |
| if state == "": | |
| return name | |
| return f"{state}.{name}" | |
| full = name if state == "" else f"{state}.{name}" | |
| object.__setattr__(handler, _FORMATTED_NAME_CACHE_ATTR, full) | |
| return full | |
| state, name = get_event_handler_parts(handler) | |
| return name if state == "" else f"{state}.{name}" |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| for part in parts: | ||
| # Find the child of the previous match whose minified id is ``part``. | ||
| found = next( | ||
| ( | ||
| c | ||
| for c in current.get_substates() | ||
| if path_to_id.get(get_state_full_path(c)) == part |
There was a problem hiding this comment.
Event lookup fails
lookup still walks every remaining segment as a state id, so the documented copied frontend event form remains broken. For a valid minified event name like a.bU, the first segment can resolve to the user state, but the final bU is an event id from config["events"], not a child state id in config["states"]. Because this loop only checks current.get_substates() and path_to_id, it exits with No state found for minified segment 'bU' instead of resolving the event handler. The command needs to treat the final segment as a handler id for the resolved state, or otherwise reject and document state-only paths.
Summary
Add state and event name minification with CLI management tools.
minify.jsonconfig file to map state/event names to short IDs (e.g.,myapp.state.AppState→"a")reflex minifyCLI command group for managing minificationCLI Commands
based on #6098