improve compile perf - #6804
Conversation
Greptile SummaryThis PR accelerates the deterministic hashing used for compiler auto-memoization by replacing per-node hasher updates with a buffer-based accumulation strategy and a memoized, per-type encoder dispatch table. It also fixes a latent correctness issue where components that inherit from a dataclass (e.g.,
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/components/component.py | Replaces the incremental _update_deterministic_hash approach with buffer-based _encode_deterministic, adds a type-keyed encoder table (_ENCODERS) with per-type memoization, caches dataclasses.fields() results via @functools.cache, and adds an identity-keyed encoding cache (_ENCODED_DATACLASSES) for frozen dataclass instances with all-scalar fields. Also fixes a pre-existing ordering bug where components inheriting from a dataclass were hashed by their (often empty) field list instead of their rendered content. The encoder table overwrite ordering is intentional and correct — _encode_frozen_dataclass downgrades its own entry on the first encounter of a mutable-field type, and the downgrade persists from the second call onwards. Logic is sound with no correctness regressions. |
| tests/units/components/test_component.py | Adds 18 focused tests covering: collision resistance, dict-order independence, subclass normalization, Var data inclusion, cache eviction (oldest-first), size cap, frozen-dataclass identity reuse, mutation tracking for non-cacheable types, MutableProxy-style synthesized classes, component-vs-dataclass branch ordering, and _get_component_hash lifecycle-hook sensitivity. The encoding_caches fixture correctly isolates module-level _ENCODERS and _ENCODED_DATACLASSES state for tests that need it. Test test_deterministic_hash_handles_dataclasses_without_params references _HashFrozenScalars defined later in the file, which is valid in Python (function-body lookup is deferred to call time) but may surprise readers. |
| packages/reflex-base/news/6804.performance.md | Changelog entry accurately describing the 3.7× speedup on large page hashing, the per-type encoder table, identity-keyed frozen-dataclass encoding reuse, and the fix for MarkdownComponentMap-inheriting components. |
Reviews (11): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile
Merging this PR will improve performance by 5.57%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | test_compile_all_artifacts[_stateful_page] |
27 ms | 25.4 ms | +6.35% |
| ⚡ | Simulation | test_compile_page[_stateful_page] |
30.6 ms | 28.9 ms | +5.8% |
| ⚡ | Simulation | test_compile_page_full_context[_stateful_page] |
34.6 ms | 33.1 ms | +4.58% |
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:improve-compile-perf (a642daa) with main (f7c848f)2
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. ↩
-
No successful run was found on
main(45b8ed5) during the generation of this report, so f7c848f was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
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="tests/units/components/test_component.py">
<violation number="1" location="tests/units/components/test_component.py:2475">
P3: This test's name and docstring claim it verifies that the frozen-dataclass encoding cache is reused by identity, but the two equality assertions would pass even if no cache existed at all (equal-but-distinct instances always encode to the same bytes). Either rename/reword it as a plain equality contract test, or actually exercise the cache path, e.g. clear component._ENCODED_DATACLASSES, hash `shared` once, then assert id(shared) is present in the cache and that a second hash of the same object short-circuits.</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/components/component.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/components/component.py:688">
P2: The new `_ENCODED_DATACLASSES` cache pins up to 8192 frozen dataclass instances and copies of their encoded bytes in a module-global for the whole process lifetime, released only by a full `clear()` once the cap is hit. Because `_deterministic_hash` runs for every component hash during a compile, these scalar-only frozen instances (and their otherwise-transient byte encodings) no longer get garbage-collected, adding persistent memory that was not retained before. The full-clear-on-capacity eviction also drops the entire cache at once and forces re-encoding of the whole working set right at the boundary, so the cache both holds memory and thrashes. Consider evicting/limiting per-entry (e.g. only cache the bytes keyed by object identity while bounding total retained bytes, or evict entries gradually instead of a wholesale clear).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
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
…t it did (reflex-dev#6948) * ENG-11237 feat(hosting-cli): report why a deploy failed, not just that it did The watch loop decided everything by substring against a bare status string, and a build failure printed two warnings: the raw status, and an unconditional pointer at `reflex cloud apps build-logs`. A generic failure printed the status alone and nothing else. That pointer was unconditional because there was nothing to condition it on. The server classifies every failure as the app's, the platform's, or transient, but that classification never reached a client -- so a failure in the build pipeline arrived dressed as a build failure and sent people looking for a bug in an app that did not have one. The failure arms now fetch GET /deployments/{id}/failure and print the recorded reason, the guidance for that fault, and the end of the build log when the code is one the log explains. The excerpt goes through console.print(markup=False): it is raw build output, and rich would read its paths and version specifiers as markup. Every way of not getting an answer is one case -- a server predating the endpoint 404s, an older self-hosted one may not route it, the network may be down -- and all three fall back to exactly what the arm printed before, so a new CLI against an older server is unchanged. * Strip terminal controls from the excerpt, and file the news fragment per package Two fixes from review. The excerpt is raw build output -- the user's own dependencies and build scripts -- and it is now printed without anyone asking, on any failed deploy, where before it took an explicit `reflex cloud apps build-logs`. markup=False stops rich reading the text as its own markup and does nothing about escape sequences, so OSC 52 could write the reader's clipboard, OSC 8 could render one destination and link to another, and CSI could erase the lines above it and leave "build succeeded" on screen. Colour is not worth carrying for output shown unsolicited. The changelog job runs towncrier per affected package, so a fragment for a change under packages/reflex-hosting-cli/src has to live in that package's own news directory, not the repository root's. * Widen the escape class, and let a malformed answer fall back like any other Two review findings, both narrow and both real. The two-character escape class covered ESC + 0x40-0x5F, so a sequence whose final byte falls outside it -- `\x1b7` (DECSC), `\x1bc` (a full terminal reset) -- had its ESC removed by the bare-control catch-all and printed the final byte as a stray character. Inert, since the ESC is what drives the terminal, but it is garbage in an excerpt whose whole job is to be read. The general ECMA-48 shape covers them. `response.json()` raises UnicodeDecodeError on a 2xx body in an encoding httpx cannot decode, and that is a ValueError rather than a JSONDecodeError, so it escaped the fallback and would have ended the watch over a malformed answer to a request whose contract is that not getting one costs nothing. The excerpt's type is checked for the same reason: the CLI ships apart from the control plane and talks to self-hosted ones. * Report a build log the server could not read, rather than passing over it The failure endpoint now separates an unreadable log from a build that stored none. Collapsing the two tells somebody their build produced no log when the store was simply down, so the two get different answers here. * Assert the no-log path offers no build log, not just no outage message The test claimed the reason stands alone and only checked the outage wording. Offering the command is what separates this path from the unreadable one, so that is what has to be absent. Verified by mutation: forcing the offer fails this test and nothing else. * Fall back however the failure body is malformed RecursionError is a RuntimeError, so a deeply nested document escaped the ValueError catch and aborted the deploy watch -- over an answer this function is contracted to treat as no answer at all. Parametrized with the UnicodeDecodeError case, since they are one rule.
|
seems i haev some overlap here with #6947 |
yeah, maybe we can try to get best of both worlds? |
…g collisions (#6947) * perf(compiler): speed up _update_deterministic_hash ~2.5x 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 * docs: add changelog fragment for the component hash speedup Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr * refactor(compiler): move memo-name hashing into the memo module 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 * fix(compiler): release memo-naming caches once compilation is done 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 * fix(compiler): close two more memo-name collisions, bound the hash buffer 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 * fix(compiler): release naming caches from the compile lifecycle `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 * chore: update pyi_hashes.json for the memo module change Moving the naming hash into `memo.py` changed the source that `reflex/experimental/memo.pyi` is generated from, so its recorded hash went stale and the pre-commit check failed. I ran `make_pyi.py` after the first commit but not after the move. `pre-commit run --all-files` now passes all seven hooks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X8v1xEMwog6ZhP7hZYibFr * perf(compiler): resolve hash encoders per type, fix a fifth memo collision Incorporates the ideas from #6804 into the memo-name hashing rework. The encoder no longer walks an isinstance ladder for every value whose exact type has no inline fast path. It resolves an encoder once per type and reaches it through a memoized table, so vars, components, enums and dataclasses each pay the ladder once per compile instead of once per value. That also collapses the two parallel ladders the previous version carried -- an exact-type one and an isinstance one -- into a single encoder per type. Fixes a fifth naming collision, found by #6804: the dataclass branch sat ahead of the component branch, so a component that also inherits a dataclass encoded as that mixin's field list. Every component built on MarkdownComponentMap does -- rx.text, rx.heading and friends -- and the mixin declares no fields, so all of them encoded to the same nine bytes. Reachable through app-wrap components, which the hash feeds in as components rather than as rendered dicts. The ImportVar encoding cache is no longer hard-coded to ImportVar: any frozen dataclass declaring only str/bool/None fields is cached by value. Numbers are excluded because equality has to imply an identical encoding for a value-keyed cache to be sound, and True == 1 == 1.0 while all three encode differently. Reading a class's annotations is the expensive part, so that per-type verdict outlives a compile in a WeakKeyDictionary, which still lets a dataclass defined in a function body be collected. Cached encodings are bounded in size as well as in count. Verified byte-identical: every memo tag generated while compiling three benchmark pages is unchanged, and an A/B of the two encoders on the values those compiles hash gives matching digests at 1.00-1.02x the speed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * docs: tighten the changelog fragments Fragments are for downstream users; the narrative is a click away on the PR. The bugfix one becomes a bulleted list of what the memo name now accounts for instead of a paragraph per collision. Verified the list renders correctly through the release tooling's own towncrier invocation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * Delete news/6947.performance.md * refactor: move the deterministic hash into its own module The hash is not a property of memoization -- it digests components, vars and rendered data under a self-delimiting encoding, and auto-memoization is just its only caller today. Moved to reflex_base/utils/deterministic_hash.py with the tests alongside it. The private _deterministic_hash/_update_deterministic_hash pair becomes one public variadic deterministic_hash(*values), which is what the two call sites wanted: component_hash now reads as the render plus the artifacts that identify a memo body, and _update_component_artifacts_hash becomes _component_artifacts, a generator that yields them instead of threading a hasher and buffer through. One shared buffer still covers the whole digest. Nothing else under utils imports components at runtime, so the two isinstance checks that need Var and BaseComponent import them inside _resolve_hash_encoder -- once per type, so it never shows up in a profile -- and the module now imports standalone without pulling in the component system. Digests are unchanged: memo tags across three benchmark pages are byte-identical to before the move, and an A/B of the encoder before and after runs at parity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * docs: restore the reflex changelog fragment The PR touches reflex/app.py, so the root package needs a fragment too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * docs: note why the shared cached get_type_hints is not used here Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * perf(compiler): hash import library names, not the ImportVars under them Walking the ImportVar lists was the largest single item in the memo-name encoding, and almost all of it was redundant. An import only reaches a memo body through the local name it binds, and every name a body references is already in its render, hooks or custom code: Reflex aliases each tag to a globally unique binding (Trigger -> RadixAccordionTrigger), and validate_imports rejects one name bound from two libraries. So bodies that render alike reference the same names, and the library names are what pin where each name comes from. Encoding drops 11-19% depending on the page (13.06 -> 11.55 ms on a memoization-heavy one). Distinct tag counts across three benchmark pages are unchanged -- 61, 9 and 2 -- so no page gains a collision from the narrower digest. What this deliberately stops distinguishing, documented on the function: two bodies binding the same name from the same library to a different export (X as N vs Y as N) or in a different form (default vs named). Both need one library to export two things a component aliases to one name. Generated memo module names change again, for the same reason as the rest of this branch: nothing outside the compiled output refers to them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * fix(compiler): encode dataclass and enum identity; trim comments Two injectivity holes in the encoder, both raised in review and both confirmed: - Two dataclasses with the same field names and values encoded identically, so Alpha(a="x") and Beta(a="x") shared a digest. The defining class now goes into the cached layout header, which is built once per type. - enum members encoded as str(value), which for an IntEnum is just its integer, so Level.ONE and 1 shared a digest. Enums now get their own tag and encode their qualified member name. Both were present before this branch; the encoder claims injectivity, so they belong with the rest of the collision fixes. The parametrized case that claimed to cover distinct dataclass types compared two instances of one type and could not have caught the first; it now uses two types. The synthesized-dataclass test asserted a runtime-built class hashed equal to the class it copied its fields from, which only held while class identity was absent from the digest. Also trims the narrative from comments and docstrings across the branch, leaving what the code needs and moving the reasoning to the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * test: cover values larger than the hash flush buffer Nothing exercised a single leaf bigger than _HASH_BUFFER_FLUSH_SIZE. A leaf is appended whole, so the buffer holds all of it before the first flush can run: a LiteralStringVar of 3x the flush size peaks at 196,621 B against a 65,536 B threshold. Behaviour is correct, it just had no test. Adds that case, plus a parametrized check that the digest is identical for flush sizes from 1 byte to 1 GiB, which pins the invariant that chunking only moves bytes into the hasher. The second one fails if a flush stops clearing the buffer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * fix(compiler): hash ImportVars again; don't crash on an unhashable field Two review findings, both reproduced. Hashing only import library names dropped ImportVar payloads from the digest. Icon._get_imports builds a per-instance package_path and alias, and a tagless ImportVar defaults to render=True so compile_imports emits it as a side-effect import ("lucide-react/light.css"). Two bodies differing only there rendered identically, shared a memo tag, and one body's import never reached the compiled module. Restores the imports dict; the narrowing was measured again at ~4% of encoding, not the 23% an earlier harness suggested. _encode_hash_cached_dataclass keyed the value cache on instances whose hashability comes from their declared field types, so a frozen dataclass declaring `name: str` while holding a list raised TypeError where the previous hasher returned a digest. It now falls back to encoding the fields directly. Also corrects the module docstring: numbers carry a type tag but no length prefix, so the previous wording overstated the encoding. Re-measured against main: encoding 1.31x on a memoization-heavy page and 1.17x on _stateful_page, with the whole component_hash at parity. Fragment updated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c * test: isolate the annotation-mismatch test from the shared caches Its well-typed _KeyedProbe instances take the cached-dataclass path, so it left entries in _hash_dataclass_encodings for whatever ran next. Every other cache-touching test in the file already takes clean_hash_caches; this one now does too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1fNEq67sbXeXDZGjLX16c --------- Co-authored-by: Claude <noreply@anthropic.com>
|
rode in with #6947 |
No description provided.