fix(codegen): make emitted IR run-to-run deterministic (#7622) - #7625
Conversation
Compiling one source twice with the same perry binary produced different LLVM IR. Two emission sites read their order straight out of a std::collections hash map, whose RandomState is seeded per process: * codegen/artifacts.rs - the `hir.closure_display_names` walk. Each entry mints a rodata constant via `add_string_constant` (whose `@.str.N` counter numbers in first-use order) and emits one `js_register_function_name` call, so both permuted every run. #7038 fixed the identical defect in the `closure_source_text` loop directly below it and left this one standing. * lower_call/property_get/dynamic_dispatch.rs - the `ctx.class_ids` walk that builds the interface dispatch tower. Each surviving entry is one icmp-guarded case block, so the map order WAS the arm order: the same three call sites named different `perry_method_*` callees run to run. Both reproduce 5/5 on the issue's own test files; both are byte-stable after the fix, and a 41-program `test_gap_*` sweep is 0/41 nondeterministic. Two mechanical siblings are sorted without a fixture and labelled as such in the test module: the virtual-override tower over the same map (emits zero arms across all 41 sampled programs), and method_registry.rs's `class_table` walk, whose insert/or_insert_with tie-breaks only diverge when two `&Class` contend for one registry key. The `max_explicit_arity` scan there also stops taking the first name that carries a class id - ids are not unique over `class_ids` keys - and takes the max instead, which is what the variable already means.
📝 WalkthroughWalkthroughThe code generator now sorts closure registrations, method registry entries, and dispatch class roots. Virtual dispatch computes maximum override arity across matching class names. Regression tests verify stable ordering and identical LLVM IR across compilations. ChangesDeterministic code generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Codegen as Code generator
participant Dispatch as Dynamic dispatch lowering
participant IR as LLVM IR emitter
Codegen->>Codegen: Sort closure and method registry entries
Dispatch->>Dispatch: Sort class roots and compute maximum override arity
Codegen->>IR: Emit registrations and dispatch arms
IR-->>Codegen: Produce deterministic LLVM IR
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/codegen/emission_order_tests.rs`:
- Around line 402-408: Update the emission-order fixture around arms and its
class-ID assignments so class names receive IDs in reverse lexical order.
Replace the self-derived sorted comparison with an explicit expected class-name
sequence representing ascending class IDs, ensuring a name-only sort would fail
while validating the dispatch-tower emission order.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f3edf346-77ac-4d66-9382-46199cdfab9a
📒 Files selected for processing (6)
changelog.d/7625-codegen-determinism.mdcrates/perry-codegen/src/codegen/artifacts.rscrates/perry-codegen/src/codegen/emission_order_tests.rscrates/perry-codegen/src/codegen/method_registry.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs
CodeRabbit review: the fixture gave class C00..C15 ids 1..16, so class-id order and name order coincided and a name-keyed sort would have satisfied the assertion just as well. Ids now run opposite to names, and the expected arm sequence is spelled out rather than derived by sorting the observed list against itself.
Audit before merge — verified, merged as v0.5.1356Reproduced the bug and the fix on my own probe (3 classes with a shared
And main's diff is exactly shape 2 as described — Sabotage re-verified: commenting out the Gates re-run: codegen 694/0, runtime 1886/0, all six lint scripts + file-size + Why this one mattered more than its diff sizeEvery Layer-1 slice audit — mine and the agents' — rests on "emitted IR Three findings I want on the record:
Also noted: the "+13% compile time" the agent first saw did not survive CPU |
Closes #7622.
Compiling one source twice with the same
perrybinary produced different LLVM IR. Both shapes the issue names arestd::collectionshash-map iteration order reaching the emitter — Rust's default hasher is seeded perRandomState, so the order is a fresh permutation every process.Variance taxonomy
I compiled the issue's two named sources six times each with one baseline binary and diffed pairwise. 5 of 5 runs differed from run 1 for both. The entire diff is two shapes; nothing else in the
.llmoves.@.str.Nnumbering of display-name constants, and the order of the matchingjs_register_function_namecalls in__perry_init_strings_*perry_method_*symbol eachicmp-guarded case block names, and the class ids in the matchingicmp eq i32chainNothing else was found: no register renumbering, no block reordering, no metadata churn, no
Instant/address-derived value, and no rayon-completion effect —ctx.native_modulesis aBTreeMap<PathBuf, HirModule>(compile/types.rs:557), so #7303's suspected mechanism is not what is happening. Both shapes areHashMap, as #7622 suspected.Shape 2's diff is diagnostic on its own: the case blocks keep their register numbers and their arguments and only the callee symbol and the compared class id move — arms emitted in a permuted order, not different arms.
Root causes
1.
crates/perry-codegen/src/codegen/artifacts.rs:1836—for (func_id, display) in &hir.closure_display_names(aHashMap<FuncId, String>). Each entry mints a rodata constant throughadd_string_constant, whose@.str.Ncounter numbers in first-use order, and emits onejs_register_function_namecall. So the map's order set both.This is the same defect #7038 fixed 36 lines below, in the
closure_source_textloop, and left standing here. Its own comment explains exactly why it matters.2.
crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs:241—for (start_cls, &start_cid) in ctx.class_ids.iter()(aHashMap<String, u32>) buildsimplementors, which is walked by index to emit one case block per implementing class. Map order was arm order.Two mechanical siblings found by a systematic sweep of all four compiler crates rather than by following the diff, and fixed in the same commit:
3.
dynamic_dispatch.rs:708— the virtual-override tower (vdispatch.*) reads the same map the same way.4.
codegen/method_registry.rs:42—for c in class_table.values(), whose body writes into the(class, method) -> symbolregistry bothinsert(last writer wins) andentry().or_insert_with(first writer wins).class_tablemixes local classes, their alias keys and imported stubs, so two distinct&Classcan contend for one key, and that registry is what every call site consults to pick its callee.The fix, and its ordering guarantee
Sorting at the point of iteration, not serializing the compiler. Rayon parallel module codegen is untouched — it was never the cause.
artifacts.rsFuncId(total: filtered to unique, materialized ids)dynamic_dispatch.rsinterface tower(class_id, class_name)(total:class_nameis the map key, so unique)dynamic_dispatch.rsvirtual towermethod_registry.rsEvery key is total, so each is a single fixed permutation, not merely "more stable".
One behaviour change, deliberate.
max_explicit_arity's scan (dynamic_dispatch.rs:794) stopped at the first name carrying a class id, and class ids are not unique overclass_idskeys — a class-expression self-binding alias (var X = class _X) and an imported class registered under both its own and its local-alias name each map two names to one id. So it was a hash-order tie-break, and the loser's arity decides how manyTAG_UNDEFINEDpadding args every emitted call in the tower carries. It now takes the max over all names sharing the id, which is what the variable already means and is the safe direction: under-padding is the #235 garbage-argument bug, over-padding just lets a default-param desugaring fire.Is the object cache implicated? Yes — say it plainly
compute_object_cache_key(compile/object_cache.rs:253) is a function ofCompileOptions, the post-transform HIR fingerprint, the perry version, a hash of the perry binary, and the codegen env vars. All deterministic. The emitted IR was not. So identical inputs produced an identical key over different.obytes: a cache hit and a cold rebuild could legitimately hold different code.For the shapes actually observed that difference is semantically neutral — the name registry is keyed on distinct function pointers, and the tower's arms are keyed on distinct class ids, so first-match picks the same arm whatever the order. But it is not neutral by construction:
seen_pairsdedups on(class_id, fname), which admits two arms sharing one class id with different symbols, and there the emission order is the behaviour. Same formethod_registry's two tie-breaks. So this was a latent correctness hazard on the cache path, not only an audit-fidelity one, and it is now closed.Evidence (all local — CI backlog is deep, so this is the evidence)
test_gap_object_create_method_this5/5 runs differ;test_gap_class_expr_dynamic_parent_ctor5/5 differ.test_gap_*compiled 3x each with the fixed binary: 0 nondeterministic / 41 compared. The 42nd,test_gap_http_overloads_3226plus, failed to link during that batch — the host was at 99% disk; on a clean re-run it links under both arms and aborts identically at runtime, see below.test_gap_*compiled with BOTH binaries and the produced executables run and compared: 123 byte-identical, 3 flagged, and all 3 investigated to a non-cause.test_gap_http_overloads_3226plusaborts identically on both arms (exit 134, panic atcrates/perry-ext-http/src/server/server.rs:911); the entire stdout difference is the thread id inside the panic message.test_gap_proto_write_local_shadows_classandtest_gap_typeof_instanceofwere SIGKILLed (137) on the fixed arm during the concurrent batch, on a host that was at 99% disk and load ~20. Re-run serially on an idle host, 3x per arm: both arms exit 0 with byte-identical output, 3/3 each. Not reproducible.artifacts.rssort alone turns exactly the two shape-1 tests red and leaves shape-2 green; reverting thedynamic_dispatch.rssort alone does the mirror image. Verified both directions.-Osthreshold so LLVM does not bury the signal), best-of-7: ratio 1.008 (median ratio 0.983). Within-arm spread is 3.56-4.25s, an order of magnitude larger than the gap.test_gap_*programs, total CPU, best-of-3: ratio 0.958.(class_id, &name)pairs per dynamic-dispatch site, in a loop that already clones aStringper class per site.cargo test -p perry-codegen --lib: 695 passed.cargo test -p perry-runtime --lib --no-fail-fast: 1885 passed, 1 failed —promise::keyed_table::tests::settling_many_keys_is_not_quadratic, a timing test that passes on rerun and lives in a crate this diff does not touch.lintjob step list from.github/workflows/test.yml, run individually: all green exceptPublic benchmark evidence freshness, which fails onmainfor reasons this diff cannot reach (it touches onlycrates/perry-codegen/) and which the workflow's own inline comment records as failing on everymainrun since 2026-07-29.cargo clippy -p perry --bins: exit 0.#7303 is the same defect and is also closed
#7303 reports shape 1 exactly —
js_register_function_nameorder flipping, with the@.str.Nnumbering following it — and attributes it to rayon closure-codegen completion order. It is thehir.closure_display_namesHashMap. On its own two repro files, with the same binaries used above:test_gap_1840_class_iterator_for_of_spreadtest_gap_yield_star_iterableThat 2-in-5 is #7303's own "~1-in-3 runs", and it is also where its reasoning went wrong: it ruled out
RandomStatebecause "that would flip near-every run". With only two registration entries a hash order agrees with itself about half the time, so ~1-in-3 is exactly what a two-elementHashMaplooks like. The frequency argument needs the entry count, and both of those files have two.Tests
Four
--libunit tests incrates/perry-codegen/src/codegen/emission_order_tests.rs, so they run on every PR that touches perry-codegen rather than only in the tag-gated integration tier.They are built not to be vacuous, both ways:
Modulefresh for every compile. The offending maps live in the HIR, so compiling one long-livedModuletwice would re-iterate the very sameRandomStateand pass unconditionally. WithN = 16entries, a chance-ordered agreement is a 1-in-16! event.What is deliberately not tested, and why. The virtual-override tower and the
method_registrywalk are sorted with no test.vdispatchblocks appear in zero of the 41 sampled programs — every receiver-typed call measured is claimed earlier bymethod_override.rs'smethod_directshape guard — and I could not construct a fixture that reaches it. A green test over a fixture emitting no arms asserts nothing, which is worse than no test. They are sorted anyway because they are mechanical siblings of the defects that are covered, and #7622 exists precisely because #7038 fixed one such loop and left its neighbour. The test module says so in prose.Out of scope
The same sweep found three hash-order reads that do not reach emitted IR and are left alone:
dialect/mod.rs:1556(which undefined register an error message names),compile/host_config.rs:841(which offending package abail!reports), andcommands/fix_applier.rs:93/128(the ordering of theperry fixreport). Worth a follow-up; not this PR, whose diff should stay auditable.