Speed up memo-name hashing, move it into the memo module, fix four tag collisions - #6947
Open
masenf wants to merge 6 commits into
Open
Speed up memo-name hashing, move it into the memo module, fix four tag collisions#6947masenf wants to merge 6 commits into
masenf wants to merge 6 commits into
Conversation
The hash feeds every value one `hasher.update()` call at a time and walks the full `isinstance` ladder per node, so a single component hash costs tens of thousands of C calls. On a foreach/cond-heavy page, `_get_component_hash` is ~50% of compile wall time. Encode into a `bytearray` flushed to the hasher in 64KB chunks instead of per node, dispatch on the exact type before falling back to the `isinstance` ladder for subclasses, cache each dataclass type's field layout with pre-encoded names, and cache the encoded form of short strings and of `ImportVar` instances (a frozen dataclass of `str`/`bool`/`None` fields, so its generated equality means exactly "same encoding", and it accounts for most of what a component hash consumes: 5664 visits across just 12 distinct values on one benchmark page). The byte stream is unchanged, so every digest is identical to before — verified against a copy of the previous implementation over all values hashed while compiling four benchmark pages. 2.3-2.6x faster on the large pages, 1.8-1.9x on the small ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr
Merging this PR will improve performance by 5.97%
Performance Changes
Tip Curious why performance improved? Comment Comparing 
Footnotes
|
Contributor
Greptile SummaryThe PR moves component hashing into the memoization module, adds buffered deterministic encoding and bounded encoding caches, expands memo identity to cover compile artifacts, and clears hashing caches after every compile.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/components/memo.py | Centralizes component hashing and memo-tag generation with buffered encoding, bounded caches, and broader compile-artifact coverage. |
| packages/reflex-base/src/reflex_base/components/component.py | Removes the previous component-local deterministic hashing and memo-tag implementation. |
| reflex/app.py | Clears memo-naming encoding caches from an outer compile lifecycle finally block. |
| tests/units/components/test_memo.py | Adds coverage for deterministic encoding, cache behavior, artifact-sensitive memo names, and class/module identity. |
| tests/units/test_app.py | Verifies cache cleanup after both successful and failed compilation. |
Reviews (6): Last reviewed commit: "fix(compiler): release naming caches fro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr
The deterministic hash exists for exactly one purpose: giving an
auto-memoized component a stable, non-colliding export name. It lived in
`component.py` as `Component._get_component_hash` and
`Component._compute_memo_tag`, but nothing outside `memo.py` ever called
either, and neither is a property of a component the way `render()` or
`_get_imports()` is.
Move the encoder and both entry points into `memo.py` as
`component_hash(component, *, recursive=...)` and `memo_tag(component)`,
next to the `create_passthrough_component_memo` call site, and drop the two
methods from `Component`. The `shallow` flag becomes `recursive`, named for
what it means at the call site: a snapshot memo body carries its whole
subtree, a passthrough body carries a `{children}` hole. Also drops the
unused `_hash_str` helper.
The own-node artifact set was missing `add_custom_code`: `_get_custom_code`
was hashed but the classmethod extension point was not, while the recursive
side picked it up through `_get_all_custom_code`. Two passthrough bodies
that rendered identically and differed only in the module-level code they
emit therefore shared one memo module, and one of the two code blocks was
dropped. Fed explicitly now, with a regression test.
Compile wall time is unchanged; this is a structural change plus the
collision fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The encoding caches that speed up memo naming were module globals with no teardown. The two value caches are capped, but the dataclass field-layout cache is keyed by type and was uncapped -- and a dataclass defined inside a function body is a fresh class object on every call, so hashing one pinned a class per compile for the life of the process. Confirmed reachable: 50 dynamically created dataclasses survived a gc.collect(). Capping that cache would be the wrong fix. It bounds retention without removing it, and once the cap is hit every dataclass encode falls back to `dataclasses.fields()` plus re-encoding field names per instance -- a silent cliff on the hot path, for a cache whose real-world population is two entries (`VarData` and `ImportVar`, stable across repeated compiles). Every component auto-memoization will ever name is named during compilation, so drop all three caches when it finishes, alongside the existing `GLOBAL_CACHE.clear()` in the same post-compile block. Digests are unchanged and compile wall time is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr
…ffer Review of the naming hash turned up two more gaps of the same kind as the `add_custom_code` one: - `_get_dynamic_imports` is emitted into the memo body by `compile_experimental_component_memo` but was never hashed, so two components differing only there shared a module and one of their two import statements was dropped. - `memo_tag` identified a class by `__qualname__` alone, so two modules each defining `class Card` with the same rendered output produced the same tag -- exactly what the qualname prefix exists to prevent. The defining module now reaches the digest rather than the prefix, which keeps the discrimination without stretching every generated module filename by a dotted module path. Both are covered by regression tests that fail without the fix. Also make the encoder's buffer bound real: the flush check ran only after a container's whole loop, so one flat 2 MB dict buffered 2 MB before the first flush. Checking per item holds it at the intended 64 KiB and costs nothing measurable -- the encoder is still 1.7-2.0x the old one and every digest is byte-identical to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
`clear_hash_caches()` was called from `App.__call__`, which only the ASGI path reaches. `reflex export` and `reflex compile` get to a compile through `prerequisites.get_compiled_app` -> `App._compile` and never touch `__call__`, so those paths never released anything. Move the call into `App._compile` -- the single funnel every compile goes through -- inside a `finally`, so a failed compile does not leave the caches behind either. Covered by a test that fails under the old placement, on both the success and the exception path. Also add the root `news/` fragment: this PR now touches `reflex/`, so the changelog check requires one for the main package too. Corrects the reflex-base performance fragment, which claimed digests were unchanged -- true of the encoder rewrite alone, but later commits deliberately folded the defining module and dynamic imports into the hash, so generated memo module names do change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr
masenf
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of change
New Feature Submission
Changes To Core Features
Description
The deterministic hash exists for one purpose: giving an auto-memoized component a stable, non-colliding export name. Started as a profiling question about it, and grew into three related pieces.
1. The encoder is 2.3–2.6x faster on large pages (1.8–1.9x on small ones).
It fed the hasher one
update()per node — ~148k C calls on one benchmark page — and walked the fullisinstanceladder for every value. Now it encodes into abytearrayflushed in 64 KiB chunks, dispatches on the exact type first, caches each dataclass type's field layout with pre-encoded names, and caches the encoded form of short strings and ofImportVarinstances.ImportVaris the whale: 5664 visits across just 12 distinct values on one page, and being a frozen dataclass ofstr/bool/Nonefields, its generated equality means exactly "same encoding".Every digest was verified byte-identical to the previous implementation across all values hashed while compiling four benchmark pages.
2. Moved out of
component.pyintomemo.py.Nothing outside
memo.pycalledComponent._get_component_hashorComponent._compute_memo_tag, and neither is a property of a component the wayrender()is. Both are now module-level functions —component_hash(component, *, recursive=...)andmemo_tag(component)— beside thecreate_passthrough_component_memocall site.shallowbecamerecursive, named for what it means there: a snapshot memo body carries its whole subtree, a passthrough body carries a{children}hole. Dead_hash_strhelper dropped.3. Four memo-name collisions, each dropping compiled output.
The hashed artifact set has to match what
compile_experimental_component_memoactually puts in the memo body. It didn't:add_custom_codenot hashed (only_get_custom_code)_get_dynamic_importsnot hashed at all__qualname__aloneclass Cardwith identical output collided@dataclassdefined in a function body is a fresh class per call; 50 stayed pinned through agc.collect()The module now reaches the digest rather than the tag prefix —
format_state_nameonly maps dots to__, and those names become filenames, so a dotted path in the prefix would stretch every generated memo module name. The caches are released fromApp._compilein afinally: that's the single funnel every compile goes through, sincereflex exportandreflex compilereach it viaget_compiled_appand never touchApp.__call__.Note for reviewers
Generated memo module names change, because items 3 deliberately fold new material into the digest. Nothing outside the compiled output refers to them, and no test pins them.
Tests
In
tests/units/components/test_memo.py(with the code under test) andtests/units/test_app.py. Each of the four collision fixes has a regression test that was mutation-checked — reverting the fix fails the test. One first-draft test passed for the wrong reason (its two probe classes had different qualnames, so class identity alone separated them) and was rewritten around a single class.Also covers encoding injectivity,
ImportVarcache-key correctness, strings past the cache limit, payloads past the flush threshold, and aclean_hash_cachesfixture so cache-state tests are order-independent underpytest-randomly.Not in this PR
component_hashis still ~45% of compile wall time on a foreach/cond-heavy page. The cost is per-node artifact gathering plusrender()over snapshot subtrees, and reusing it needs the page walk to descend into snapshot subtrees with the collector sealed off — the_memoize_structural_childmachinery Implement client state with useClientState hook #6936 is adding. Should build on that rather than race it. (Fusing the five_get_all_*traversals into one was tried and measured ~2% slower: they already share cached per-node results, so the aggregation was never the cost.)GLOBAL_CACHE.clear()has the same ASGI-only gap as the old cache-release site, soreflex exportnever frees the var cache either. Pre-existing and unrelated; belongs in its own commit.https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr
🤖 Generated with Claude Code