perf: deduplicate imports as they merge, not once at the end - #7012
Conversation
Greptile SummaryThis PR deduplicates import metadata during merging and preserves deterministic first-seen ordering during collapse.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the eligible review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/utils/imports.py | Adds deterministic normalization, merge-time deduplication, and order-preserving collapse behavior. |
| tests/units/utils/test_imports.py | Adds regression tests covering deduplication, first-seen ordering, unordered batches, and sort-key uniqueness. |
| packages/reflex-base/news/7012.bugfix.md | Documents deterministic generated output across unchanged compilations. |
| packages/reflex-base/news/7012.performance.md | Documents the compilation performance improvement from reducing duplicate import metadata. |
Reviews (7): Last reviewed commit: "perf: deduplicate imports as they merge,..." | Re-trigger Greptile
Merging this PR will degrade performance by 35.95%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | test_get_all_imports[_complicated_page] |
2.9 ms | 4.6 ms | -37.91% |
| ❌ | Simulation | test_get_all_imports[_stateful_page] |
542.1 µs | 820.5 µs | -33.93% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing khaleel/dedupe-import-merge (845b06e) with main (2145622)
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. ↩
4dbb4b9 to
24fb9a6
Compare
|
Acting on the CodSpeed report rather than acknowledging it: the regression was real, and the pushed version cuts it roughly in half.
What changed since the first push: a library reached through only one child cannot have gained a duplicate there, so the first batch is taken as-is and hashes nothing, and only from the second batch onward does a library earn a lookup set. Locally on Two further ideas measured and dropped: inlining the accumulator made no difference (0.124s vs 0.125s), and caching 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
ffc6bd9 to
b66065f
Compare
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
b66065f to
0f564da
Compare
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
`_get_all_imports` merges every descendant's imports on the way up the tree, and the merge only concatenated. A tag shared across a subtree was therefore carried once per node rather than once, so the list a node reports grows with the size of its subtree instead of with the number of distinct imports it has. Nothing read those duplicates, since `collapse_imports` removed them at the end, but naming a memoized component hashes the node's whole recursive import dict, so the cost landed there. On an 80-route app one `Button` hashed 2,263,441 values covering 133 distinct objects, and hashing was 65% of the compile. Back to back on the same machine, `reflex compile` on that app drops from 263s to 48s. A library's first batch has nothing accumulated to collide with, so it is deduplicated against itself and no further, and a batch of one entry skips even that. Only from the second batch does a library earn a lookup set. That matters because deduplicating unconditionally costs more than it saves on shallow trees, where there is little duplication to remove. `collapse_imports` moves off `set` for a second reason. Its iteration order varies with PYTHONHASHSEED, so two compiles of identical source emitted their imports in different orders, and every content hash derived from them differed. That made compiler output irreproducible: on the same app, two consecutive runs rewrote 113 of 157 generated files with new memo names. Preserving first-seen order fixes that, and both compiles now agree byte for byte.
0f564da to
845b06e
Compare
What
merge_importsandmerge_parsed_importsconcatenated without deduplicating, andcollapse_importsdeduplicated through aset. This makes the merges dedupe as they go, and makes the collapse preserve first-seen order.Why it is slow
Component._get_all_importsmerges every descendant's imports on the way up the tree. Because the merge only concatenated, a tag shared across a subtree was carried once per node rather than once, so each node's import list grew with the size of its subtree instead of with the number of distinct imports it actually has.Nothing consumed those duplicates, since
collapse_importsdropped them at the end, once per page. But_get_component_hash, which names every auto-memoized component, hashes the node's whole recursive import dict, so the cost landed there instead.Measured on an app with 80 routes and 42,268 components (7,036 memo wrappers):
_update_deterministic_hashcallsButton: values hashedButton: distinct objects behind themThat Button has no children. The 2.26M came from 323,296
ImportVarvisits over 21 distinct import vars.Result
reflex compileon that app, run back to back on the same machine:Reproducibility
collapse_importsusedlist(set(import_vars)), whose iteration order overImportVarvaries withPYTHONHASHSEED. Two compiles of identical source therefore emitted imports in different orders, and since import order feeds_get_component_hash, every memo name derived from it changed too.On the same app, two consecutive runs of unmodified reflex rewrote 113 of 157 generated files with different memo names. With this change both runs agree byte for byte.
That is worth having on its own: stable output means a bundler and its downstream caches see unchanged modules when nothing changed.
Deduplicating a library's first batch against itself, added in review below, turned out to be load-bearing for this too. Those repeats varied per process, so they moved the memo hash even once import order was stable. On the app above, the two fixes together take it from 113 differing files to 0, byte-identical between
PYTHONHASHSEED=1andPYTHONHASHSEED=999983.On the CodSpeed regression
test_get_all_importsis genuinely slower, and the first version of this PR was worse than what is here now. Worth being precise about what that benchmark measures.It times
_get_all_imports()alone. Deduplicating is pure cost inside that call; the payoff is a much smaller dict, and it is collected by whoever consumes the result. On_complicated_page, the result goes from 277 entries to 19 (14.6x), and on_stateful_pagefrom 61 to 10. The benchmark pays for that reduction and never uses it, so it can only show the cost side. The whole-compile benchmarks are unaffected, and the app above spends 65% of its compile hashing exactly what shrank.I still narrowed it rather than leaving it. A library's first batch has nothing accumulated to collide with, so it is deduplicated against itself and no further, and a batch of one entry skips even that; only from the second batch does a library earn a lookup set. Locally, on
_complicated_pageover 300 calls: base 0.101s, dedupe-everywhere 0.255s, this version 0.170s.The batch-of-one guard is doing real work there. Deduplicating every first batch unconditionally costs 36% (0.165s to 0.225s in one run of the same comparison), because the average first batch is 1.4 entries and it allocates a dict a million times for batches that cannot duplicate. The guard gets the same correctness for 9%.
I also tried and dropped two further ideas: inlining the accumulator to avoid a call per library made no measurable difference (0.124s vs 0.125s), and caching
ImportVar.__hash__bought ~10% on the micro but needed a typing workaround to keep the cache out offields(), which is where_update_deterministic_hashwould have picked it up. Neither seemed worth the complexity, but say the word if you would rather have them.Tests
Four regression tests in
tests/units/utils/test_imports.py, each confirmed to fail against the current implementation before the fix:test_merge_imports_deduplicatestest_merge_parsed_imports_deduplicatestest_merge_imports_keeps_first_seen_ordertest_collapse_imports_is_order_preservingThe existing
test_merge_importscompares withset(...)on both sides, so it could not have caught either problem.Full unit suite: 8007 passed, 18 skipped.
pre-commitclean.One unrelated flake to note:
tests/units/test_state.py::test_state_manager_lock_expireand its_contendsibling fail intermittently under full-suite load, on a wall-clock redis lock expiry. Across alternating full-suite runs they failed with this change and also without it, and always pass whentest_state.pyruns on its own.Notes
defaultdict(list), so no caller sees a changed return type.collapse_importsalready removed these duplicates before anything was emitted. This only stops them being built and hashed in the first place._get_all_importsis still recomputed from scratch at every node rather than cached, which is a separate quadratic. Left for its own PR, since components are mutated during compile and a cache needs an invalidation story.