engine: delete the fragment-compiler address round trip (row C) - #991
Merged
Conversation
`codegen::full_source_len` recovers the source variable's full element count -- the genuine-Vensim out-of-range bound for VECTOR ELM MAP, where an offset landing outside the source's storage yields `:NA:`. It had arms for `StaticSubscript`, `Var` and `TempArray`, but none for the dynamic `Expr::Subscript`, so a dynamically-subscripted source fell to the catch-all and reported an extent of one element. The consequence was silent: `y[D] = VECTOR ELM MAP(src[i], offsets[D])` with a variable `i` produced NaN for every element whenever `i` selected anything but `src`'s first element, and for any non-zero offset at all, poisoning every downstream variable. A literal subscript takes the `StaticSubscript` arm and was already correct, which is why no existing test covered it. Both construction sites of `Expr::Subscript` pass the variable's base offset and its full declared dimensions, so the new arm resolves the same extent the static arm does, and its `bounds` product is the same number the offset-map lookup returns rather than a fallback default. The two backends cannot diverge on this shape: wasmgen rejects a dynamically-subscripted ELM MAP source outright, so the regression test is VM-only by construction rather than by omission.
Three places let HashMap iteration order reach a salsa-cached value, so two identical compiles of the same fragment could compare unequal. None of them produced a wrong number, which is exactly why they survived: the damage is that salsa stops backdating (every downstream consumer re-executes) and the compiled artifact is not reproducible run to run. This is the GH #595 class of defect. `PerVarBytecodes::temp_sizes` was flattened straight out of a HashMap at all three fragment emission sites -- `db::assemble` and both hand-copied tails in `db/ltm/compile.rs`. `FragmentMerger::absorb` folds those entries into a resize-and-max over a dense vector, which is order-independent, so the disorder was invisible downstream and only ever cost cache hits. All three now share one `temp_sizes_by_id` helper ordered by temp id. `Compiler::new` laid out `graphical_functions` -- and assigned every `Lookup`/`LookupArray` `base_gf` operand -- by iterating `module.tables`, so a fragment referencing two or more table-bearing variables emitted hash-seed-dependent bytecode. Sorting the idents fixes the block order without disturbing anything downstream: each variable's tables stay contiguous from a monotonically advancing cursor, so `gf_blocks_of_fragment`'s disjoint-or-nested precondition holds, `FragmentMerger::absorb_gf` keys blocks by content rather than position, and both the VM and wasmgen index strictly relative to `base_gf`. This deterministically changes the graphical-function ordering emitted for models that have such a fragment (`test/metasd/theil-statistics/Theil_2011.mdl` is the one in-repo case); numerics are unaffected. `LoweredLtmVariable::dep_idents` was a HashSet whose iteration order assigned consecutive mini-layout offsets. Symbolization inverts those offsets, so the emitted fragment was identical either way -- but the two LTM dependency walks were the only ones not already using a BTreeSet, and intermediate state should be a function of the inputs. Its incidental value is evidence for the round-trip deletion this branch is building toward: reversing the walk leaves every characterization golden byte-identical, which is what "a fragment is independent of the private offsets it is lowered at" means in practice. The new tests compile the same fragment on repeated fresh databases and assert byte equality. Each carries a precondition assertion that the fixture really does express an ordering (more than one temp, more than one graphical function), so a fixture that stops exercising the hazard fails loudly instead of passing vacuously.
GH #964 deletes the address round trip in the per-variable fragment compiler: a variable is lowered at private mini-offsets, a stand-in one-variable `compiler::Module` is built to compile it, and every concrete offset is then reverse-mapped back to a name. Before removing that machinery we need a gate finer than the existing corpus, which pins whole- model numerics and so cannot see a fragment change shape, stop being emitted, or lose its salsa cache entry. Twelve fixtures pin the symbolic fragment of every variable and phase -- the opcode stream with its `SymVarRef` operands, the literal pool, the graphical functions, module declarations, static views, temp sizes and dimension lists -- across the shapes the deletion has to preserve: scalar and stock phases, the three arrayed equation forms including an EXCEPT default, static and dynamic subscripts, a runtime array view, array reducers and the array-producing builtins that hoist into temps, scalar and per-element graphical functions, a lookup-only table holder, an explicit sub-model instance at two different input sets, stdlib module expansion with its implicit helpers, PREVIOUS/INIT in both their direct and helper-rewritten forms, a resolved recurrence SCC and its combined fragment, and LTM synthetic scores in both exhaustive and discovery mode. A golden alone would be blind in two directions, so each fixture asserts two more things that are never regenerated. A hand-written exhaustive variable-to-phases map with a required justification means a fragment that silently stops being emitted fails on the phase that vanished rather than as a downstream `NotSimulatable`, and a fragment that appears from nowhere fails too; it is checked before the model runs, so attribution is precise. Hand-computed VM spot checks, asserted non-empty, mean a fixture cannot contribute zero runtime coverage. The salsa half uses `#[cfg(test)]` execution counters rather than pointer equality, because a fragment is designed to compare equal across a layout edit and salsa backdates a re-executed query whose value compares equal -- so the memo address cannot distinguish "reused" from "recomputed". Each test has a control step: re-syncing an identical project must re-execute nothing. What that measured is worth recording, because it is not what #964's acceptance criteria assume: adding, deleting or renaming any variable currently re-executes every fragment in the model, through two coarse edges (the whole `ModelDepGraphResult` read for three runlist bits, and the whole `SourceModel::variables` map read during lowering). Narrowing those is the next commit's work. Until then the execution counts are saturated, so the tests additionally assert fragment VALUE equality across each individual edit -- which is what would actually catch a newly introduced layout dependency. Per-edit rather than end-state, because an end-state-only assertion passes even with a deliberate layout leak injected: the base and post-rename projects hold the same variable count.
Per-variable fragment emission had three hand-copied implementations of "compile one variable's one phase and symbolize it": the shared tail in `db::assemble`, and two byte-identical inline copies in `db/ltm/compile.rs` that diverged only in which map they passed as `module_refs`. The previous commit had to fix the same temp-ordering defect in all three, which is what a mirror family costs. There is now one implementation, reached by all five call sites. Each copy also built a stand-in one-variable `compiler::Module` by struct literal, cloning five fields per phase: the nested offsets map, the tables map, the converted dimensions, the dimensions context, and module_refs. The dimensions-context deep clone is the part GH #655 attributes the measured per-fragment overhead to. Emission now takes a `ModuleCtx<'a>` holding only what codegen reads, all by reference, and the phase-invariant half is built once per variable rather than once per phase. `Module::compile()` builds the same borrowing context from its own owned fields, so there is still exactly one codegen shared by the fragment path and the test-only monolithic path -- no trait, no second container, no duplicated walk. `module_refs` turned out to be read by nothing at all. It was the only consumer of `build_caller_module_refs`, which ran a second complete dependency walk per variable -- building stub variables, computing sub-model layouts, resolving every dependency name -- purely to populate a field codegen never looked at. Both are deleted, along with `runlist_order` (also unread) and `n_temps` (always equal to `temp_sizes.len()`, so one fewer invariant that can disagree). With production no longer constructing one, `compiler::Module` is now `#[cfg(test)]`. The compiler reported it as never-constructed on the lib target, which is GH #964's "no production `compiler::Module` literal remains" criterion proved mechanically; gating it turns that from a claim someone re-audits into a compile error. The whole-model path remains as the run-invariance differential oracle, which still passes. Measured on this machine: `bytecode_compile/clearn` 428.19 ms -> 410.81 ms, `bytecode_compile/wrld3` 8.61 ms -> 8.11 ms, roughly 4-6% of whole-model compile time. That is real and reproducible but much smaller than the plan's framing implies -- the borrows are the smaller half of this row's payoff.
`Opcode::PushVarView` and the un-fused `Opcode::AssignNext` were carried by the VM, the symbolizer, the resolver, the fragment merger, the wasm backend and the bytecode metadata, and neither can be produced by the compiler. `PushVarView`'s only non-test constructor is the symbolic resolve direction, which fires only on a symbolic `PushVarView`, which only ever came from symbolizing a concrete one -- a closed dead cycle. `AssignNext` was always consumed by the peephole that fuses it with the preceding `Op2`, because a stock update is built as `Op2(Add, ..)` and constant folding performs no algebraic rewrites, so nothing can collapse that operand. Establishing that empirically took a panicking probe over the whole workspace plus the C-LEARN structural run, with a self-test proving the probe site was live. That is evidence, not a guarantee, so the reachability claim is no longer prose: the fusion is now an explicit emit-time step that returns false when the last opcode is not an `Op2`, and codegen turns that into a typed error. Prose cannot fail; a check can. The failure is also raised earlier, at the lowering chokepoint every path funnels through, so a stock update in a shape codegen cannot emit produces a per-variable `NotSimulatable` naming the stock rather than an assembly-level batch failure listing names with no reason. Both directions are tested, including the `MAX`-wrapped shape that implementing `non_negative` (#545) would produce and the `Neq` shape, which is the one binary operator codegen does not leave as a trailing `Op2`. This is a capability deletion, not an encoding cleanup: a `MAX`- or `Neq`-shaped stock update would have compiled and run correctly before, and is now a compile error. #545 must re-add the opcode or restructure the update; the new guard is written so that whoever does gets a loud, attributable failure naming the shape codegen needs. Also sweeps the comments the deletion invalidated. Several were the correctness argument for a match arm this commit removes, and one pair had drifted into disagreeing with each other about the rule they claim to share. The dated wasm design plan is deliberately left alone -- it is a record of what was decided in May, not a live contract.
GH #964's acceptance criteria assume layout-only project edits reuse unchanged cached fragments. The characterization gate added earlier on this branch measured that they do not: adding, deleting or renaming any variable re-executed every fragment in the model. Three reads were responsible, each pulling in a whole-model value to answer a per-variable question. `compile_var_fragment` read the entire dependency graph to learn three bits -- whether this variable is in the initials, flows and stocks runlists. Lowering read the whole `SourceModel::variables` map to look up dependencies by name. And the dependency walk read the whole implicit-variable map to answer two questions about one name. Each is now a projection or a per-name query, so a reader depends on the entries it actually consults; the queries still re-execute when the model's variable set changes, but their values backdate, which is what firewalls the readers. An unrelated add now re-executes one fragment instead of all of them, a delete re-executes none, and a rename re-executes one. `incremental_add` on a hundred-variable chain goes from 1.31 ms to 807 us. That 38% is the larger half of this row's payoff -- the borrows in the previous commit were worth 4-6% of whole-model compile time, so the plan's claim that the win rides on deleting the clones is measurably the wrong way round. The narrowing stops short of a variable that instantiates a module. That case stays saturated for two reasons worth naming: a first stdlib call splices the template into the project's model map, and the module-ident context is an interned handle whose id changes when the set grows, so every variable's parse gets a new cache key and cannot backdate at all. Narrowing that means keying each parse on only the module idents that variable references, which is circular with parsing. Both outcomes are pinned by a test rather than described in a comment, so the limitation goes red when someone fixes the cache key rather than silently persisting. The tightened counts are now the gate the next stage is measured against, alongside the fragment value-equality assertions, which are kept because they are independent of the counting apparatus: a rewrite may legitimately change which queries run, but it is never free to change what a fragment compiles to.
The per-variable fragment compiler reached its layout-independent result by
the long way round: lowering assigned each variable and its dependencies
arbitrary offsets in a private per-fragment layout, codegen baked those
integers into concrete opcodes, and a symbolize pass mapped every one of
them back to a name so assembly could resolve it against the real layout.
Steps one and two existed only so step three could undo them.
Lowering now produces names. `compiler::VarRef { name, element_offset }`
replaces the raw offset in five `Expr` variants, codegen emits
`SymbolicOpcode` / `SymbolicStaticView` / `SymbolicModuleDecl` directly, and
`resolve_module` assigns addresses exactly once, at assembly. There is no
symbolization pass, no reverse offset map, and no per-fragment layout;
`SymVarRef` is an alias for the type lowering emits rather than a second
representation to convert into.
The test-only monolithic `Module::compile` keeps working by resolving the
emitted symbolic module against its own layout through that same
`resolve_module`, so there is still exactly one codegen and one direction of
travel. Three reverse lookups codegen only needed because lowering had
thrown the name away become direct: a VECTOR ELM MAP source's extent, a
lookup table's identity, and a module instance's base slot. The peephole
optimizer and the bytecode builder move to the symbolic domain along with
the emission they serve, which keeps `resolve_bytecode` a strict 1:1
mapping -- what the run-invariant flow prefix and the SCC per-element
segmentation both rest on.
Five things here are not behavior-neutral, each a deliberate decision:
- The VM's stack-safety check moves from a per-fragment `assert!`/abort in
the deleted `ByteCodeBuilder::finish` to a per-phase structured `Err` in
`resolve_bytecode`, now the single place concrete bytecode is born. On
well-formed fragments the two agree, since every fragment starts and ends
at depth zero; the per-phase form additionally catches a fragment that
fails to balance, which the per-fragment form could not see, because
`concatenate_fragments` strips trailing `Ret`s and appends.
- Both of `resolve_bytecode`'s failure modes are now reported rather than
one reported and one aborting. A `stack_effect` metadata bug -- a compiler
bug, not anything a caller can cause or fix -- used to panic, which in a
`panic = abort` release build takes down a libsimlin host. The cost is
that it becomes swallowable by a caller that discards compile results;
the VM still never receives the bytecode, since the `Err` prevents the
module from being built at all.
- `VariableMetadata.offset` becomes `Option<usize>`, `None` for the model
being compiled, so consulting its layout during lowering is a compile
error rather than a silent slot 0. In the one scenario that could reach
it -- a self-instantiating model, which the module-graph cycle gate
rejects first -- the old code produced a plausible wrong slot.
- Sixteen `SymbolicOpcode` variants become compiler-provably dead: codegen
is the only producer, so the dead-code lint proves what previously needed
an empirical probe. They are the superseded halves of incremental
view-stack construction and broadcast iteration. Retiring them along with
their `Opcode` twins, VM arms and wasm arms is a change to the VM
instruction set and the wasm parity harness, sequenced separately.
- `compiler::Expr` is no longer keyed to a layout, so `model.rs`'s
prohibition on it ever deriving `salsa::Update` is retired rather than
reworded. Nothing needs the derive yet; the point is that the soundness
argument against it no longer applies.
All 12 characterization goldens are byte-identical with no regeneration, and
the execution-count assertions are unchanged. That second half is now the
load-bearing one: a fragment can no longer carry an offset of its own model,
and consulting one does not compile, so the type subsumes most of what the
value-equality assertions were watching for. The only change to
`fragment_char_tests.rs` is prose.
Compile time: bytecode_compile/clearn -3.3%, full_pipeline/clearn -3.2%,
wrld3 -3.6% and -2.4%; incremental is unmoved (add -1.3%, equation-edit
+0.9%, the latter because a lowered reference grew from 8 to 16 bytes). The
gain is deleted per-fragment allocation rather than an algorithmic change,
and it is the smallest of this row's three wins. The case for it is one
emitter, one direction of travel, and a layer of scaffolding gone.
`FragmentMerger::absorb_non_gf` narrowed four resource bases with a bare `as u16` and `renumber_opcode` added them with unchecked `+`. Past 65,536 merged literals, module declarations, static views or dimension lists, an id wraps to a real, in-range resource: the program compiles, runs, and returns different numbers, with no diagnostic anywhere. This is the third member of a family whose other two members caused real problems -- graphical-function ids (#582) and temp ids (#583) -- and it was the only one unguarded, while `renumber_opcode`'s rustdoc listed the two guarded ones as though the list were exhaustive. Reachability was measured rather than assumed: instrumenting the merge and compiling the largest model in the repository under LTM discovery mode produces 11,363 merged literals, 5,121 static views and 399 module declarations -- roughly 17% of the id space, so 5.8x headroom. Latent, not live. Fixed anyway, because the failure mode is silent and the guard is cheap. Bases are now computed through one checked helper and every addition in `renumber_opcode` is checked, as are the per-phase accumulators in `assemble_module` that do the same arithmetic by hand. A fragment carrying none of a resource is exempt: it names no id, so there is nothing to represent, and with that exemption the separate base check is provably redundant. The tests pin the boundary in both directions -- 65,536 entries merge, 65,537 report an error -- so a future off-by-one in either direction fails.
`combine_scc_fragment` interleaves a resolved recurrence SCC's members by cutting each member's bytecode into per-element segments and emitting them in the SCC's element order. Jump offsets are relative, so a backward jump that targeted an opcode outside its own segment would, after the reorder, land on whatever happened to sit that far back -- a silent miscompile with no bad id, no bad reference, and nothing downstream able to notice. Nothing checked it, and the function's own loud-safe failure list did not mention jumps at all, though its consumer is precisely what reorders the code a relative jump lives in. Codegen cannot currently produce that shape: iteration opcodes are emitted from one place, whose operand is an array expression, while a member's per-element write comes from the statement-level assignment arm, so a loop is always wholly inside one segment. That is the argument for the check rather than against it -- the assumption lives in a different file, and nothing would notice it changing. The same statement-boundary reasoning is what makes each segment independently balanced on the value and view stacks, so an emitter that broke it would want re-examining on all three counts. The check is that a jump's target lies within its own segment's bounds. The trailing tail is the case worth naming: opcodes after the final write merge backwards into the last element's segment, so they inherit that segment's start rather than beginning one of their own. Getting that wrong is what the tail test exists for, and it caught it.
`FragmentMerger` was correct and carefully commented, but a substantial part of its contract was expressed as parity with the monolithic `Module::compile`'s resource layout -- an argument that points at another implementation rather than at what must be true. Earlier commits on this branch made that implementation `#[cfg(test)]`, stopped production from building one, and removed the address representation it shared with the fragment path. An invariant defined by reference to a test-only implementation cannot be checked without running it, and goes stale silently when the reference moves. The contract is now eight obligations stated on the merger, as properties of the merged fragment. Every fragment's opcodes index its own tables; merging relocates those private id spaces into one shared space; a merged opcode must still name the same thing. From that: flat resources are appended so their ranges tile, ids must fit their bytecode type, graphical functions share by content and only by content while a lookup run stays contiguous, temps must never alias between fragments whose live ranges overlap, absorption is one-to-one and order-stable so a prefix of the fragment list is a prefix of the merged stream, the merger touches neither variable references nor jumps, and the per-phase split assigns the same ids as the all-phases merge, with per-phase literal pools the one deliberate exception. Property tests check each obligation over generated fragments, and each carries a mutation probe recording what it catches that the previous tests did not. Two are worth naming because they are wrong answers the whole existing suite accepts: dropping graphical-function gap totality leaves all twelve characterization goldens and the entire file_io corpus green, and dropping a phase's resource base makes a flows-phase module call resolve to a different sub-model while every golden and all but two of the unit tests pass. A corpus can say numbers moved; it cannot say the merger mis-assigned a module id. The temp-merging tests had an oracle that was a local reimplementation of the monolithic rule -- and worse than that: both call sites asserted the oracle equal to a hard-coded literal before ever comparing it to the merger, so it constrained nothing at all, not the monolith's behavior and not even a belief about it. Deleted; both tests now assert the invariant directly, with the fixtures kept. The initials-phase renumbering is extracted so the phase-split property can drive the real function rather than a stand-in. That is not cosmetic: as an inline loop, freezing two of its three accumulators left the entire repository green. Regression seeds are checked in with provenance, following the precedent already in the tree. The two graphical-function seeds cover different halves of gap totality, and the header says which, because the leading gap is the one nothing else pins.
After the round-trip deletion a fragment carries no offsets of its own model at all, and consulting that model's layout during lowering does not compile. One layout channel into a fragment survives: a cross-module reference carries an element offset derived from the sub-model's already fixed layout, so growing a sub-model must shift the parent's cached fragment. Nothing asserted that, in either direction. It is the one place where fragment value equality still has independent teeth, because the type cannot see this channel -- it runs the other way, from a layout that is settled before the parent compiles. The test grows a sub-model so an output moves slot, and requires the parent's cross-module element offset to move with it, the fragment value to change, and the parent to recompile. A cache hit here would serve a stale offset and read the wrong slot of the instance -- a silent wrong number rather than a compile failure, which is the real hazard on this channel. Shrinking the sub-model back restores a byte-identical fragment, so the dependence is on the layout rather than on the fact of an edit. The negative direction is asserted on the same evidence: growing a model that nothing instantiates must leave the fragment bit-identical and must not recompile the cross-module reader. Both execution sets were measured rather than assumed; value equality alone cannot distinguish a correct cache hit from a recompile that happened to agree.
The engine had two independent circular-dependency gates that could disagree about whether a model is simulatable: the production salsa path, and a second transitive-closure walk inside `ModelStage1::set_dependencies` with its own `CircularDependency`, its own cross-model output resolution and its own topological sort. They did disagree, and the divergence was reproducible: an element-acyclic recurrence SCC that the production gate resolves was still a whole-variable `CircularDependency` on the legacy path, so that path rejected models production compiles and simulates. `set_dependencies` now reads the production dependency graph and copies its runlists. The second walk and everything only it used are deleted. Three differences in the runlists it now produces are all fixes the legacy walk never had: lookup-only holders excluded from the flow and initials lists, `INITIAL()`-backed seeding, and resolved-SCC contiguity. Agreement is pinned in both directions by a test written as agreement rather than as "it compiles", so a re-introduced second gate reds it rather than sitting unnoticed. Unifying the gate does not unify the emitter, and the difference had to be made explicit. Production lowers a resolved recurrence SCC by interleaving its members' per-element segments; the monolithic compiler has no equivalent, and the gate that used to reject those models is precisely what kept it from ever being handed one. Left alone, it accepted them and emitted a program that reads a co-member's element before assigning it -- a silent wrong answer from the path that serves as the run-invariance differential oracle, on exactly the model class the element-acyclicity refinement exists for. It now refuses them with a `NotSimulatable` naming the reason. Routing through the production graph also exposed a hazard the legacy walk had been hiding: a model in a module cycle that also contains a variable-level cycle drove salsa into an unrecoverable dependency-graph cycle -- a process abort under `panic = abort`, reachable from the public `From<datamodel::Project>` on a project a user can draw. The existing module-cycle test does not catch it, because without the variable cycle the recurrence refinement never runs. `from_salsa` now takes the same reachability gate production uses. Two error codes lose their only producer with the walk and are documented in place rather than removed, since the enum is mapped to a numbered FFI surface; both were already unreachable from any production compile. Also fixes a trap in `scripts/check-docs.py`, which walked the git-excluded agent scratch directory: a working copy of a `CLAUDE.md` landing there reported every relative path in it as broken and failed the pre-commit hook on a tree that was fine.
…fields Item 17 said the `errors`/`unit_errors` fields on `Variable` and the two model stages were dead weight carried through the monolithic compilation path, redundant with the salsa pipeline, and the rustdoc at each field site said the same: "Legacy field... New code should use `db::collect_model_diagnostics`." Half of that is backwards, and it is the dangerous half. The two `Variable` fields are the live channel by which lowering reports failure -- the salsa diagnostics are produced by reading them, not as an alternative to them. `lower_var_fragment` reads the parsed variable's unit errors to emit the non-fatal unit diagnostics, and the lowered variable's equation errors to emit the fatal ones; that second read is the only thing in the engine that reports a dimension mismatch. Acting on item 17 as written would have deleted a live channel, and every test that could have caught it reads the same field. So the fields stay and the rustdoc now describes the mechanism, naming both read sites and the test that pins them. The two model-stage fields are test-only in their readership but are kept deliberately: `ModelStage1::errors` is the monolithic path's only simulatability gate, it has no database to consult instead, and it now also carries the refusal that keeps that path from mis-compiling a resolved recurrence SCC. What was genuinely dead is gone: two mutators and two `ModelStage1` fields that only the deleted dependency walk used, one of which had no caller even before that. A standing gate asserts both halves of the channel -- the stage's value carries the error, and the matching diagnostic reaches `collect_all_diagnostics` -- with the lowering fixture asserting its parsed variable is clean, so it isolates a lowering error from a parse error. Each read site was probe-verified, which also corrected a claim: the parse-site read is a strict subset of the lowered one, since lowering clones parse errors forward, and its unique contribution is the conveyor and queue driven-flow suppression. Item 17 is marked resolved by correction, with the classification written out per field and an explicit note that the original premise was half wrong and should not be acted on.
A reference to a sub-model variable names the module *instance*, not the variable, so every lookup asking "how big is the variable this reference names?" answered about the wrong thing. One root cause, three symptoms, all of them predating this branch. `VECTOR ELM MAP` over a sub-model array got the whole instance's slot count as its source extent, so an offset past the array's end read the next variable in that sub-model instead of yielding `:NA:`. With a four-element array followed by another, the model means `[40, NaN, NaN]` and the engine produced `[40, 100, 200]` -- the neighbour's first two elements, with no diagnostic anywhere. The lowering-side twin that folds a constant-offset ELM MAP asked the module variable for its dimensions, got none, and bounded the source at a single element -- so every read folded to NaN, including the in-range one. The same cause failing in the opposite direction, which is what made the two sides worth unifying rather than fixing separately. And a flat name lookup during dimension resolution made every cross-module array look scalar, so a wildcard subscript on one was rejected outright: `SUM(m.arr[*])`, the ordinary way to reduce over a sub-model's arrayed output, did not compile. That one had a test sitting on top of it -- `db::stages_tests`' `arrayed_module_project` is built from exactly that expression to prove something else, and never noticed, because it stops at the lowering stage whose own `get_dimensions` twin has always followed a module variable into its sub-model correctly. Two implementations of one rule, one stage apart, disagreeing. The extent table is now keyed by the whole reference rather than by name, built by one function that walks a module instance into its sub-model and records an entry per sub-model variable at that variable's slot, recursing through nested instances. Lowering and emission read the same table, so they can no longer answer differently; before, they answered from two readings of the symbol table and got this case wrong in opposite directions. Not a regression of the symbolic emission commit earlier on this branch: the offset-scan it replaced returned the module entry's slot count by the same mechanism, because a sub-model variable at sub-offset zero shares its instance's base offset. Both lowering-side defects predate the branch point as well. The nested case is pinned by tests through two module hops, and the whole-array form is deliberately kept as a compiles-at-all check with the reason stated: its view carries the source's full dimensions, so the fallback coincides with the right answer and it cannot detect a wrong offset.
The capacity check added earlier on this branch ended up stated in three places that disagreed at the boundary. `resource_base` accepts a merged table of exactly 65,536 entries, which is right -- the last id assigned is 65,535. But the count that becomes a later phase's base was narrowed eagerly, and that narrowing ran first, so a table filled exactly was rejected before the merger could accept it. The same eager narrowing sat on the stocks-phase base and on the initials-phase accumulator, the latter rejecting an initials list that filled the table exactly while folding in the last initial -- with nothing left to assign an id to. Every rejected program had all of its ids in range, so the failure was a spurious refusal rather than a wrong answer. It is fixed by making the counts that flow between phases plain sizes and discharging the bound in one place, at the point an id is actually assigned. The eager narrowing helper is deleted rather than corrected, so the bound cannot drift apart again. That earlier commit also claimed its tests pinned the boundary in both directions. They did not: the test covers literals, and a literal pool is phase-local, so its base is always zero and it never reached the narrowing that was wrong. The three resources that do carry a base across phases were unpinned. They are pinned now, including one test that drives the real initials-phase renumbering rather than a stand-in.
Module assembly merges every fragment once more at the end to build the shared context tables -- graphical functions, module declarations, static views, temp offsets, dimension lists. That merge also produced an opcode stream and a literal pool, both of which are discarded: each compiled initial keeps its own literal pool, and the flow and stock phases keep theirs. The capacity check added earlier on this branch was applied to that discarded pool, so a model whose literals summed across phases exceeded the id space failed to assemble even though every pool that survives was well inside it. Around 33k scalar stocks is enough, each contributing one initialization and one stock-update literal -- a size this compiler targets, since the LTM model that motivated this row's performance work carries roughly 37k synthetic variables. It is a regression introduced here: before, the same computation wrapped silently, and the wrapped ids went only into the stream nobody reads. The aggregation now asks for side channels and gets only side channels. The bound stays stated in one place; literals simply have no base to compute there, because nothing names a merged literal id -- which is the rule the previous commit wrote down, applied to the pools the module actually keeps. The distinction worth recording, since the neighbouring merges look identical: the flow and stock concatenations also discard their side-channel tables, and bounding those is still right, because the ids they assign are baked into bytecode that is retained and must index the aggregate tables. The question is not whether a table is kept but whether a retained opcode names an id in it. Literals in the final aggregation are the only place the answer is no. Two tests, because they fail for different reasons: a merger-level one that a full merge bounds a literal pool while a side-channel merge does not, and an assembly-level one driving a real model, which is the only one that catches the call site being pointed back at a full merge. The second uses a test-only capacity override rather than a fixture with 65k literals, and asserts both directions -- at a capacity where the aggregate does not fit assembly must succeed, and at one where a retained pool does not fit it must still fail. Incidentally removes a full opcode renumber over every fragment in the model whose output was thrown away.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #991 +/- ##
==========================================
+ Coverage 91.70% 91.72% +0.02%
==========================================
Files 239 241 +2
Lines 157770 157779 +9
==========================================
+ Hits 144678 144728 +50
+ Misses 13092 13051 -41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Jul 28, 2026
bpowers
added a commit
that referenced
this pull request
Jul 29, 2026
… LTM on C-LEARN 101x) (#993) A profile-driven optimization pass on the compile path, aimed at the two cases that hurt: large models (C-LEARN) and LTM. Fixes #655 ## Results Criterion (`benches/compiler.rs`), against the branch point: | benchmark | before | after | | |---|---:|---:|---| | `bytecode_compile/clearn` | 408.3 ms | 271.8 ms | **−33.4%** | | `full_pipeline/clearn` | 444.1 ms | 294.7 ms | **−33.6%** | | `parse_mdl/clearn` | 32.47 ms | 20.45 ms | **−37.0%** | | `ltm_compile/wrld3` | 320.0 ms | 32.4 ms | **9.9x** | | `bytecode_compile/wrld3` | 8.18 ms | 7.57 ms | −7.4% | | `full_pipeline/wrld3` | 10.06 ms | 9.50 ms | −5.6% | | `salsa_incremental/*` | — | — | −1.4 … −2.3% | And the case the branch exists for, measured with `examples/ltm_full_bench` because LTM on C-LEARN is far outside a criterion budget: **C-LEARN with LTM: 221.7 s → 2.07 s (107x), peak RSS 1152 → 526 MiB.** The synthetic-variable stage alone went 219.3 s → 0.49 s. `ltm_compile` is a new benchmark group: "compile world3 with LTM on" had no repeatable measurement, and it is roughly an order of magnitude more expensive than the same model's ordinary compile. ## What was actually wrong Every one of these is the same shape — work redone because nobody had counted how often it ran. Commits are ordered so each one's measurement is attributable. **`reconstruct_model_variables` was not cached** (51cccdb, 11935ae). It parses and lowers every variable in a model, and it was a plain function. The LTM pipeline builds a causal graph once per query that needs polarity or module structure, so on a world3 LTM compile it ran **923 times and was 58% of every instruction the process executed**. Nothing between those calls can change its answer. It is now a `#[salsa::tracked]` query returning a shared `Arc<HashMap<..>>`, and `CausalGraph::variables` holds that Arc so the sharing survives into every graph built from it. `reconstruct_single_variable` — called once per link score, 6,721 times on C-LEARN — is now a lookup in that map. This is the 9.9x and the 107x. **`parse_var*` rebuilt the dimension context per variable** (0dde5da). `DimensionsContext::from` was 11% of all instructions on C-LEARN: `parse_var_with_module_context` built a fresh one from its `&[datamodel::Dimension]` argument on every call, and `get_dimensions` resolved each dimension name by scanning that slice, re-canonicalizing every declared name per lookup and then rebuilding the matched `Dimension` from scratch. Every production caller already had the cached `project_dimensions_context` in hand or was one line from it. The parse chain now takes `&DimensionsContext`. **`canonicalize` did three passes and gave up on non-ASCII** (90a66df, d6ae3df). It was 25% of a C-LEARN compile and 22% of a world3 LTM build — the largest single line item in both. Two problems: the fast path ran a Unicode `trim`, an `is_ascii` scan and then `is_canonical`'s own scan, and *one* non-ASCII character demoted the whole string to a per-character `to_lowercase()` case check. That is exactly the shape of the engine's own generated identifiers — an LTM link score is forty ASCII characters with three U+205A separators in it, and re-canonicalizing one case-checked all forty. It is now a single pass with one decode per non-ASCII character. The slow path also stopped allocating four intermediate `String`s per identifier part when usually none of the four rewrites applies. **`lower_variable` deep-copied an AST purely to destroy it** (905d516). `lower_ast(scope, ast.clone())` immediately consumed that clone to build an `Expr1`, which is a different tree with different identifier types and is constructed from scratch either way. `Expr0::clone` was roughly a million of the compile's 6.4 million allocations. `Expr1::from` now takes `&Expr0`. **`get_dimensions` handed back an owned `Vec` it had just copied** (d6ae3df). Both lowering contexts returned `Option<Vec<Dimension>>` built by `to_vec`-ing a slice they were holding, once per `Var` and `Subscript` node. A `Dimension` clone copies the element vector *and* the element→index `HashMap`; on C-LEARN that was 216k dimension-vector clones. **The MDL importer rebuilt its dimension context per array element** (6277f44). `build_element_context` resolves cross-dimension mapping substitutions and built a `DimensionsContext` to do it, once per element of every arrayed variable. Memoized in a `OnceLock`; worth −35.8% on import alone. **Identifier maps hashed with SipHash** (d6ea402). 4–6% of cycles, spread over every name resolution the compiler does. ## Review follow-ups **Per-variable invalidation for LTM link scores** (f1417af, review r3675119315). Routing `reconstruct_single_variable` straight through the whole-model map made every consumer depend on every variable's *lowered* form, so one unrelated equation edit regenerated every link score — 6,721 of them on C-LEARN. `link_score_equation_text_shaped` documents the opposite ("recomputed only when the involved variables change"), so this was a real regression the PR introduced. The lookup is now a `#[salsa::tracked]` firewall: it still reads the whole map and still re-executes on any edit, but its value is one variable, so salsa backdates it whenever that variable is untouched. Same pattern `model_variable_by_name` already uses over `SourceModel::variables`. Keeping one derivation rather than adding a second reconstruction path preserves what the original change was for — the map and the single-name lookup cannot disagree about what a name resolves to. The cold-build win is untouched. The pin counts query-body entries rather than comparing memo pointers, and that is load-bearing: salsa backdates a re-executed query whose value compares equal, so a pointer check would have gone green through the whole defect. Verified in both directions — the test fails on the commit that introduced the problem (4 of 4 scores regenerated) and passes on the implementation it replaced. **Vertical tab / form feed trimming** (review r3675119302) — correct analysis, deliberately declined. `str::trim` strips U+000B and U+000C and the new byte table marks both canonical, so a name bounded by one is no longer trimmed. No real model has such an identifier, so the two-byte change would buy a behavior nobody can reach. Recorded on the thread as a decision rather than silently dropped. ## Non-obvious decisions **FxHash is a deliberate security trade, and the reasoning lives on the type.** `crate::common::IdentMap` is `rustc-hash`-backed. The upside beyond speed is that a fixed seed makes iteration order reproducible across processes, which is the direction this crate already wants — a salsa-cached value built by iterating a map must not differ run to run (#595). The cost is that an adversary who chooses the *keys* can force collisions, and the keys here are variable names out of a model file. Every engine entry point today compiles a model on behalf of the person who supplied it (the CLI, the local MCP and viewer servers, pysimlin, the browser's wasm bundle), so a collision attack costs the attacker their own compile. The alias's doc says not to extend it to a map keyed by input from a party other than the one paying for the work. **If a hosted service ever compiles models server-side on behalf of other users, that alias has to be revisited.** **One behavior change, in a degenerate model only.** Where two declared dimensions canonicalize to the same name, `get_dimensions` used to resolve to the first in declaration order while the `DimensionsContext` every later stage consults keeps the last. It now agrees with the context, so a name cannot resolve to one dimension during parsing and another during lowering. **Explicit-wins precedence was made explicit rather than assumed.** `reconstruct_single_variable` checked explicit variables before implicit ones; the map it now reads inserted implicit ones over the top. Implicit names carry the reserved `$⁚` prefix so no collision is reachable and nothing changes today, but the map uses `or_insert_with` so the rule is decided in one place. **Cached-map iteration order is now fixed for a revision** where it used to differ between two calls in the same process (each `HashMap` seeds its own `RandomState`). That is strictly more deterministic; any output that depended on the old order was already irreproducible run to run. ## Measurement notes Two things worth recording, because they changed how the work was done: - **Instruction count is not time here.** The compile is allocator-bound: removing 8% of `canonicalize`'s instructions moved cycles by 0.3%. Callgrind was used only to find *structural* waste — call counts and who-calls-whom — and `perf stat -e cycles` plus criterion decided everything. - **Code layout noise on a big machine is ±1.5% on the small benches.** Adding literally unreferenced code moved `ltm_compile/wrld3` by 4 ms. Swings that size were confirmed against instruction counts, which are layout-immune, rather than chased. Also: the benches and `examples/*` use the system allocator, while production native binaries install mimalloc. The ~22% allocator share these profiles show overstates production. ## Tests un-ignored Ten `#[ignore]`d integration gates were gated on runtime, not on anything about what they assert, and that judgement is now stale. An ignored gate catches nothing until someone remembers to ask for it — `clearn_ltm_var_count_guardrail`'s own doc comment records a regression that slipped through for exactly that reason. Now in the default run: `simulates_clearn` (the cross-simulator ground-truth gate against `Ref.vdf`), `clearn_residual_exactness`, `oracle_clearn` and `oracle_wrld3` (the run-invariance soundness oracles), `clearn_ltm_var_count_guardrail`, `simulates_wrld3_03_wasm`, `corpus_clearn_macros_import`, `clearn_unit_error_flood_is_cleared`, `metasd_expansion_tier_full`, `corpus_sstats_multi_output_materializes`. Cost, measured on `taskset -c 0-3 RUST_TEST_THREADS=4` (which approximates a CI runner rather than a 32-core developer box): `cargo test --workspace` 25.4 → 31.1 s against the 180 s cap. Scaled to CI's ~60 s baseline that is roughly 74 s, leaving a ~2.4x rail. The twenty that stay ignored now give reasons that will not go stale: what is slow is *execution* under the non-JIT wasm interpreter, which no compiler speedup touches (`simulates_clearn_wasm` 34 s, `discovery_clearn_matches_vm_wasm` 287 s); or the test is a strict subset of one that now runs by default; or it is a diagnostic dump rather than an assertion. `docs/dev/rust.md` gains the rule this followed — `#[ignore]` for runtime is a judgement about today's engine, so re-take it after the engine gets faster — with the commands to time the ignored set and to check the suite against a CI-shaped core count. ## On #655 All four of its findings, checked against the current tree rather than assumed: 1. **Per-fragment deep-clone of `DimensionsContext`** — fixed, by #991 (`ModuleCtx` borrows it). 2. **`intern_name`'s O(D²) linear scan** — fixed; it is a hash lookup. 3. **Each LTM equation parsed 2–3 times** — fixed by #980's typed `LtmEquation`. 4. **Interning under a global lock, ~15–20%** — substantially reduced here (both the call volume and the per-call cost), and the issue itself scopes the residual to #317, which stays open. ## Not fixed - **#317** (intern `Ident<Canonical>` via `salsa::interned`) stays open. The interner is still ~5–6% of compile cycles; this branch cut the *map* hashing around it, not the sharded-mutex interner itself. - **#955** (element-level circuit enumeration churn) stays open. Its witness test improved 2.9 s → 1.92 s, but the 20x gap to its sibling tests is intact and the structural directions it proposes — rotate on integer node ids, defer subscript materialization — are untouched. - After this branch no single item dominates: allocator ~22% of cycles, the interner ~5–6%, `canonicalize` ~4–6%. ## Verification `cargo test --workspace` green. All 20 runnable `#[ignore]`d heavy gates pass, including `simulates_clearn` against `Ref.vdf`, `clearn_residual_exactness`, both invariance oracles, both wasm parity corpora, `clearn_with_ltm_simulates_model_vars_identically`, and the pinned-loop C-LEARN gate. The seven ignored tests that fail (`simulates_delayfixed*`, `simulates_getdata*`, `simulates_except_xmile`) fail identically on the branch point — they are known-unsupported features, not regressions. Every commit passed the full pre-commit hook. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DE767rH3hiqzC5u7uaEfCt
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
1 participant
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.
engine: delete the fragment-compiler address round trip (row C)
Fixes #964
Fixes #568
Row C of the "Deleting the Round Trips" plan, the last of the three. Rows A
(#973/#980/#985) and B (#990) are already on main.
The round trip this row deletes: production compiles each variable by assigning
it and its dependencies arbitrary private mini-offsets from 0, building a
stand-in one-variable
compiler::Moduleto compile it, and then reverse-mappingevery concrete offset back to a name so the fragment can be cached and
merged. The address representation was
names -> private offsets -> names -> global offsets; it is nownames -> global offsets, resolved once at assembly.Fifteen commits, each independently green and reviewed adversarially before
landing. Every one of the twelve characterization goldens introduced in the
first commit is byte-identical at the last one, with no regeneration at any
point -- which is the strongest evidence available that the shorter route
produces the same program rather than a plausible one.
What landed
C1a -- the gate (3593cc9, 49d6b1a, d2b940b)
Before removing machinery, pin what it produces. The existing corpus pins
whole-model numerics, which cannot see a fragment change shape, stop being
emitted, or lose its cache entry.
Twelve characterization fixtures pin every variable's per-phase symbolic
fragment across the shapes the deletion must preserve (scalar/stock phases, the
three arrayed equation forms incl. an EXCEPT default, static and dynamic
subscripts, a runtime array view, reducers and array-producing builtins that
hoist into temps, scalar and per-element graphical functions, a lookup-only
table, an explicit sub-model at two input sets, stdlib module expansion with its
implicit helpers, PREVIOUS/INIT in both forms, a resolved recurrence SCC and its
combined fragment, and LTM synthetic scores in both modes). Each fixture also
carries a hand-written never-regenerated variable-to-phases map with a required
justification, and hand-computed VM spot checks -- so a fragment that silently
stops being emitted fails on the phase that vanished rather than as a downstream
NotSimulatable, and a fixture cannot contribute zero runtime coverage.Three defects found while building it, each fixed in its own commit:
VECTOR ELM MAPreturned all-NaN for a dynamically-subscripted source.codegen::full_source_lenhad arms forStaticSubscript,VarandTempArraybut none for the dynamicExpr::Subscript, so it reported anextent of one element and every offset outside
[0,1)became:NA:. Aliteral subscript was already correct, which is why no test covered it.
HashMap-ordered values reached salsa-cached fragments (temp_sizesat all three emission sites, the
graphical_functionsblock layout and everybase_gfoperand, and the LTM lowered-dependency set). No wrong numbers --which is why they survived -- but identical compiles compared unequal, so
backdating stopped and the compiled artifact was not reproducible run to run
(GH engine: init runlist ordering is nondeterministic for an init-time algebraic cycle through a synthetic-module stock output #595's class). The GF one fires on
test/metasd/theil-statistics/Theil_2011.mdl.C1b -- one emitter, borrowing, and the salsa edges (d4044b2, a0859a7, 907b93e)
Emission had three hand-copied implementations; there is now one, reached by
five call sites, taking a context that borrows instead of a stand-in
compiler::Modulethat cloned five fields per phase.module_refsturned outto be read by nothing, and it was the only consumer of
build_caller_module_refs-- a second complete per-variable dependency walk existing purely to populate
a field codegen never looked at. Both deleted, along with
runlist_orderandn_temps.With production no longer constructing one,
compiler::Moduleis#[cfg(test)]. The compiler reported it as never-constructed on the libtarget, which is #964's "no production
compiler::Moduleliteral remains"criterion proved mechanically rather than asserted.
Two opcodes the compiler cannot emit (
Opcode::PushVarViewand the un-fusedOpcode::AssignNext) are deleted from the VM, the symbolize pass (itselfdeleted in C1c), the resolver, the merger, the wasm backend and the bytecode
metadata. Their reachability claim is no longer
prose: the fusion is an explicit emit-time step that returns false when the
last opcode is not an
Op2, and the failure is raised at the loweringchokepoint as a per-variable error naming the stock. This is a capability
deletion -- a
MAX-wrapped stock update (the shape #545 would produce) wouldhave compiled and run before and is now a compile error.
Three coarse salsa edges narrowed, each pulling in a whole-model value to
answer a per-variable question. An unrelated add now re-executes one fragment
instead of all of them; a delete re-executes none. A module-instantiating add
is still saturated, pinned by a test with both causes named rather than
described in a comment.
C1c -- emit layout-independent fragments directly
symbolize_*,ReverseOffsetMap,layout_from_metadataand everyper-fragment mini-layout offset loop are gone. Lowering produces
VarRef { name: Ident<Canonical>, element_offset }, codegen emits symbolicopcodes, and assembly resolves once. The test-only monolithic path resolves
the same symbolic module through the existing pass, so there is one codegen
and one direction of travel.
The three reverse lookups codegen performed -- recovering a variable's size,
a table's ident, and a module's offset from an integer -- are direct name
lookups now. They existed only because lowering threw the name away.
Position-independence stopped being a convention and became a type:
VariableMetadata.offsetisNonefor the model being compiled, soconsulting its layout during lowering is a compile error rather than a silent
wrong answer.
C2 -- the merger's contract as fragment semantics
FragmentMergerwas correct and unusually well commented, but a substantialpart of its contract was expressed as parity with the monolithic
Module::compile's resource layout -- an argument that points at anotherimplementation rather than at what must be true. After C1a-C1c that
implementation is
#[cfg(test)], production never builds one, and it nolonger shares an address representation with the code those comments claim to
constrain.
The contract is now eight obligations stated on the merger itself, as
properties of the merged fragment: referential integrity per resource family,
flat ranges that tile, ids that fit their bytecode type, graphical functions
shared by content and only by content, temps that never alias between
simultaneously-live fragments, absorption that is 1:1 and order-stable (so a
prefix of the fragment list is a prefix of the merged stream -- what the
run-invariant flow prefix and the SCC segmentation are counts into), no
touching of variable references or jumps, and a phase split that assigns the
same ids as the all-phases merge.
Eleven property tests check them over generated fragments, each with a
mutation probe demonstrating what it catches that the previous tests did not.
Two are worth naming because they are silent failures the whole existing
suite accepts: dropping graphical-function gap totality leaves all 12 goldens
and the entire
file_iocorpus green, and dropping the phase-split resourcebase makes a flows-phase
EvalModulecall the wrong sub-model while everygolden and 5,363 of 5,365 unit tests pass.
The plan suspected the temp-merging test's oracle was a test-local
reimplementation of the monolithic rule rather than a call into it. It was
worse: both call sites asserted the oracle equal to a hard-coded literal
before comparing it to the merger, so it pinned nothing at all -- not the
monolith's behavior, not even a belief about it. Deleted, and both tests now
assert the real invariants with no oracle.
Two defects fell out of writing the obligations down. Four
u16resourcebases were narrowed with
as u16and added unchecked, so a merged table past65,536 entries would produce a wrapped id naming a real in-range resource --
compiles, runs, returns different numbers, no diagnostic. This is the third
member of the family whose other two bit production (#582, #583) and the only
unguarded one. Measured headroom on the largest model in the repo is 5.8x, so
it was latent rather than live. And
combine_scc_fragmentreorders a member'sper-element segments while jump offsets are relative, with nothing checking
that a jump stays inside its own segment; codegen cannot currently produce
one, which is exactly why the assumption -- which lives in a different file --
is now a check rather than a comment.
C3 -- one cycle gate, and a tech-debt item corrected rather than executed
docs/tech-debt.mditem 17 said the embeddederrors/unit_errorsfields onVariableand the two model stages were "dead weight carried through themonolithic compilation path", redundant with the salsa pipeline. Half of that
is backwards. The two
Variablefields are the live channel by which loweringreports failure -- the salsa diagnostics are downstream of them, not an
alternative -- and the
lowered.equation_errors()read is the only thing inthe engine that reports a dimension-mismatch error. The field rustdoc actively
invited the deletion ("Legacy field... New code should use
db::collect_model_diagnostics").So the fields stay, the rustdoc now describes the mechanism, and item 17 is
resolved by correction with the classification written out: every read site,
for each of the four fields, marked as live channel / test-only readership /
dead. What was genuinely dead went: the two mutators and two
ModelStage1fields that only the deleted dependency walk used. A standing gate asserts
both halves of the channel -- the stage's value carries the error and the
matching diagnostic reaches
collect_all_diagnostics-- and each of the threeread sites is probe-verified.
#568 tracked two independent circular-dependency gates that could disagree
about whether a model is simulatable. They did disagree, and it was
reproducible: an element-acyclic recurrence SCC that the production gate
resolves was still a whole-variable
CircularDependencyon the legacy path,so that path rejected models production compiles and simulates. It is resolved
by unification rather than documentation --
set_dependenciesreads theproduction dependency graph and copies its runlists, and the second transitive
closure, its cross-model output resolution, its topological sort and its
CircularDependencyare deleted. Agreement is pinned in both directions by atest written as agreement rather than as "it compiles", so a re-introduced
second gate reds it.
Routing through the production graph then exposed a hazard the legacy walk had
been hiding: a model in a module cycle that also contains a variable-level
cycle drove salsa into an unrecoverable dependency-graph cycle -- a process
abort under
panic = abort, reachable from the publicFrom<datamodel::Project>on a project a user can draw.from_salsanowtakes the same reachability gate production uses.
Unifying the gate does not unify the capability, and the difference matters.
Production compiles a resolved recurrence SCC by interleaving its members'
per-element segments; the monolithic compiler has no equivalent, and the gate
that used to reject those models is what kept it from having to. So the
monolith now refuses them explicitly, with a
NotSimulatablenaming thereason. Without that it accepted them and emitted a program that reads an
element before assigning it -- a silent wrong answer from the path track
invariant 3 designates as a live differential oracle, on exactly the model
class the element-acyclicity refinement exists for.
Found by reviewing the branch as a whole
Every box above was reviewed adversarially in isolation before it landed. A
pass over the finished branch then found two defects that per-box review
structurally could not see, and the first of them contradicted a hypothesis an
earlier reviewer had investigated and dismissed as unreachable.
A cross-module reference names the module instance, so every "how big is the
variable this reference names?" lookup answered about the wrong variable. That
one root cause had three symptoms, all predating this branch: a
VECTOR ELM MAPover a sub-model array read the neighbouring sub-model variable insteadof yielding
:NA:(the extent came back as the whole instance's slot count);its lowering-side twin got no dimensions for a module variable, bounded the
source at one element, and folded every read to NaN -- the opposite direction,
the same cause; and a flat name lookup made every cross-module array look
scalar, so
SUM(m.arr[*])did not compile at all.The last is the one a user could hit without touching
VECTOR ELM MAP, and ithad a fixture sitting on top of it:
db::stages_tests'arrayed_module_projectis built from exactly that expression, to prove something else, and never
noticed -- because it stops at the lowering stage, whose
get_dimensionstwinone stage away has always followed a module variable into its sub-model
correctly. Two implementations of one rule, disagreeing, with the test on the
working side.
The fix keys the extent table by the whole reference rather than by name, with
one constructor that both lowering and emission read, so the two sides can no
longer answer differently. The second defect was a boundary the merger accepted
and its callers rejected; the capacity bound is now stated in one place.
Measurements
All on one machine,
cargo bench -p simlin-engine --bench compiler, bothcolumns measured rather than quoted from an intermediate state.
bytecode_compile/clearnbytecode_compile/wrld3full_pipeline/clearnfull_pipeline/wrld3salsa_incremental/equation_edit/100salsa_incremental/incremental_add/100salsa_incremental/incremental_remove/100Run-to-run variation on the large model is around half a percent, so the
whole-model figures are "five to six percent" rather than anything finer.
The last commit deletes an opcode renumber over every fragment in the model
whose output was discarded, and that did not surface above the noise --
recorded because it is the kind of change one expects to show up and it did
not.
The plan's Fig. 1 claim that the win rides on turning the stand-in's clones
into borrows is measurably wrong in both directions. The borrows were worth
4-6% of whole-model compile time; deleting the round trip itself was worth
~3%, the smallest of the three; and the large win -- 39% on incremental add --
came from narrowing salsa edges, which Fig. 1 does not mention at all. One row
(
salsa_incremental/equation_edit) is ~1% slower, because a loweredexpression grew from 8 to 16 bytes per reference.
Stated plainly because the plan asked for cost claims to be kept honest: if
this row were justified on compile time alone, the number is single-digit
percent. The case is architectural -- one emitter, one direction, three
reverse lookups gone, and the round-trip deletion alone removing ~830 net
lines from the compiler.
Deliberate decisions and residuals
Sixteen
SymbolicOpcodevariants are now compiler-provably dead and areannotated per-variant rather than deleted. Deleting the symbolize pass made
codegen their only producer, so the dead-code lint proves what previously
needed an empirical probe. They are the superseded halves of incremental
view-stack construction and broadcast iteration. Retiring them together with
their
Opcodetwins, VM arms and wasm arms is roughly 149 non-test sitesacross the VM instruction set and the wasm parity harness -- a different
subsystem with a different risk profile, sequenced as its own PR.
Deleting two opcodes in this PR was a capability deletion, not an encoding
cleanup. A
MAX-wrapped stock update -- the shape implementingnon_negative(#545) would produce -- would have compiled and run before andis now a compile error. The guard is written so whoever implements #545 gets
a loud failure naming the shape codegen needs, rather than a silent one.
accumulate_var_compile_errordiscards a compile error'sdetails, sothe reason for a lowering failure never reaches a user; the variable name
does, via the diagnostic. The obvious fix trades the message for the
variable-level classification, the source snippet, and the
has_variable_errorsflag, so it was correctly reverted. The real fix is adetailsfield onEquationError-- roughly 50 sites across 20 files on thetype the FFI error surface is built on -- and belongs in its own PR.
A module-instantiating add still invalidates every fragment in the model.
Three coarse salsa edges were narrowed; the remaining saturation has two
causes, both named in a test that pins the limitation rather than describing
it: a first stdlib call splices the template into the project's model map, and
the module-ident context is an interned handle whose id changes when the set
grows, so every variable's parse gets a cold cache key and cannot backdate at
all. Narrowing that means keying each parse on only the module idents that
variable references, which is circular with parsing.
One layout channel into a fragment survives by design -- a cross-module
reference's element offset comes from the sub-model's already-fixed layout --
and is now pinned in both directions.
compile_phase_to_per_var_bytecodes'sErr(_) => Nonestill loses thecause of a codegen failure. Pre-existing; not made worse. The stock-update
check was placed at the lowering chokepoint specifically so its failure does
not route through it.
An LTM link fragment re-executes nondeterministically (33 of 40 identical
repetitions). Established: the value is equal every time, so salsa backdates
it and no consumer ever sees a wrong or nondeterministic artifact -- it is
wasted recomputation. Not established: which recorded dependency reports
"maybe changed" given an unchanged dependency set. That is a salsa
dependency-verification question rather than a fragment-compiler one.
On the plan's own claims
Two of the plan's assertions did not survive contact, and both are worth
recording because the plan asked for cost claims to be kept honest.
Fig. 1's claim that the win rides on turning the stand-in's clones into
borrows is wrong in both magnitude and attribution. Borrows bought 4-6%;
deleting the round trip itself bought ~3%, the smallest of the three; and the
large win came from narrowing salsa edges, which Fig. 1 does not mention.
The acceptance criterion "layout-only project edits continue to reuse
unchanged salsa-cached fragments" did not hold when the row started. Adding,
deleting or renaming any variable re-executed every fragment in the model. It
holds now for a plain aux or a PREVIOUS/INIT helper; the module case is the
documented residual above.