engine: compiler optimization pass (C-LEARN -33%, LTM on world3 9.8x, LTM on C-LEARN 101x) by bpowers · Pull Request #993 · bpowers/simlin

bpowers · 2026-07-29T14:09:39Z

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 Strings 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:

Per-fragment deep-clone of DimensionsContext — fixed, by engine: delete the fragment-compiler address round trip (row C) #991 (ModuleCtx borrows it).
intern_name's O(D²) linear scan — fixed; it is a hash lookup.
Each LTM equation parsed 2–3 times — fixed by engine: delete the LTM classify-twice round trip and type its synthetic equations #980's typed LtmEquation.
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 Intern Ident<Canonical> strings via salsa::interned to reduce hashing and cloning overhead #317, which stays open.

Not fixed

Intern Ident<Canonical> strings via salsa::interned to reduce hashing and cloning overhead #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.
engine: LTM element-level circuit enumeration spends most of its time in string canonicalization/rotation churn (cross-product shapes) #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.ai/code/session_01DE767rH3hiqzC5u7uaEfCt

`parse_var_with_module_context` built a fresh `DimensionsContext` from its `&[datamodel::Dimension]` argument on every call, and `get_dimensions` resolved each dimension name by scanning that slice, re-canonicalizing every declared dimension name per lookup and then rebuilding the matched `Dimension` (re-interning its every element name) from scratch. Callgrind on C-LEARN: `DimensionsContext::from` was 11% of all instructions (5752 calls), and `canonicalize` -- 25% of the program, its single largest line item -- had `Dimension::from` and that scan as its top two callers. Every production caller already had the project's canonicalized context in hand from the salsa-cached `project_dimensions_context`, or was one line away from it; the datamodel slice was being carried purely so the callee could redo work the caller had already paid for. So the parse chain (`parse_var`/`parse_var_with_module_context`/`parse_equation`/`build_tables`/ `get_dimensions`, plus LTM's `to_flow_ast`/`parse_ltm_equation`) now takes `&DimensionsContext` and resolves a dimension name with one canonicalize and a hash probe. Three call sites that were threading two or three views of the same dimension list (`extract_implicit_var_deps`, `reconstruct_implicit_ variable`, `lower_implicit_var`) lose the redundant one. `db::query::parse_source_variable_impl` deliberately keeps building its own context, from the NARROWED per-variable dimension list rather than the project-wide one: that narrowing is what keeps an unrelated dimension edit from invalidating every cached parse, and a scalar variable's list is empty so it costs nothing. Two lookups on the hot path also stopped interning. `DimensionsContext:: is_dimension_name`/`lookup` and `NamedDimension::get_element_index` built a `CanonicalDimensionName`/`CanonicalElementName` -- taking a shard of the global interner and installing an `Arc` payload -- only to hand it to a hash probe and drop it. They now probe by `&str` through new `Borrow<str>` impls, sound for the same reason `Ident<Canonical>`'s already-present one is: the storage hashes string content, so a `&str` probe hashes identically. 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. Measured (criterion, vs the commit before): bytecode_compile/clearn 408.3 -> 362.7ms (-11.2%), full_pipeline/clearn 444.1 -> 397.0ms (-10.6%), ltm_compile/wrld3 320.0 -> 310.3ms (-3.0%). The new `ltm_compile` group is added here: "compile world3 with LTM on" had no repeatable measurement, and it is ~38x the model's ordinary compile.

`canonicalize`'s non-canonical path ran four unconditional `String`-returning rewrites over every identifier part -- map `.`, unescape `\\`, collapse whitespace, lowercase -- so a name whose only defect was its capitalization still paid four allocations plus a two-way substring searcher for the `\\` pattern it did not contain. Callgrind put it at roughly 1000-1600 instructions per call, and `canonicalize` is reached once per identifier occurrence: it was 25% of all instructions on a C-LEARN compile and 22% on a world3 LTM build, the largest single line item in both. Each rewrite is now guarded by a cheap "is there anything to do" test and borrows when there is not, and the last two are fused for an ASCII part so they write straight into the output buffer. The fusion is deliberately ASCII only, for two reasons that both fail outside it: ASCII lowercasing is per-byte and context-free, while `str::to_lowercase` is not in general (Greek capital sigma lowercases differently at the end of a word), and U+00A0 -- the one non-ASCII character the whitespace pass recognizes -- cannot occur. The non-ASCII arm keeps the two steps separate and unchanged. The `canonicalize_slow_path` reference in the test module is untouched, which turns the existing fast-vs-slow proptest into a genuine differential oracle for this rewrite. Two things are added on top, because that property's `\PC{0,100}` generator reaches these branches roughly never: a second proptest over an alphabet chosen to hit every branch (quotes, periods, backslashes, both literal escapes, whitespace runs, U+00A0, and the context-sensitive sigma), and hand-written cases for the step interactions whose ORDER matters -- `\\n` is an escape only because the backslash collapse runs first, and a non-escape backslash breaks a whitespace run. Measured (criterion, cumulative with the previous commit, vs the branch point): parse_mdl/clearn 32.47 -> 29.01ms (-10.7%), bytecode_compile/clearn 408.3 -> 350.5ms (-14.2%), full_pipeline/clearn 444.1 -> 381.6ms (-14.1%), ltm_compile/wrld3 320.0 -> 296.3ms (-7.4%).

…nsions per AST node Three changes to the same hot path, measured together because the last one is what makes the first two show up. `canonicalize`'s fast path did three passes: a Unicode `trim`, an `is_ascii` scan, then `is_canonical`'s own scan -- and if the name held ANY non-ASCII character, `is_canonical` demoted the whole string to a per-character `to_lowercase().ne(once(c))` 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 one pass over a byte table with a single decode per non-ASCII character, and the case predicate is spelled as two `next()` calls rather than an iterator comparison (the generic `Iterator::eq_by` did not collapse, and was 3.6% of a world3 LTM build's cycles). The scan is conservative in one direction only -- what it rejects falls through to the unchanged `trim` + `is_canonical` pair, so a name it declines is still answered exactly as before. `Expr2Context::get_dimensions` and `Expr3LowerContext::get_dimensions` returned `Option<Vec<Dimension>>`, and every implementation built that Vec by `to_vec`-ing a slice it was already holding. Both are called once per `Var` and `Subscript` node during lowering, and a `Dimension` clone is not cheap: it copies the element vector AND the element->index `HashMap`. On C-LEARN that was 216k dimension-vector clones, showing up as `RawTable::clone`, `drop_in_place<Dimension>` and a slab of the allocator profile. They now return `Option<&[Dimension]>`; no caller needed to change. Finally, several sites re-canonicalized and re-interned a string whose TYPE already proved it canonical -- `CanonicalDimensionName::from_raw(dim.name())` on a `dimensions::Dimension`, or `::from_raw(ident.as_str())` on an `Ident<Canonical>`. The three canonical newtypes are tags over one interned storage, so `From<&Ident<Canonical>>` (and `Dimension::canonical_name()`) make those conversions a refcount bump. A test now pins the property the fused scan exists for, which is invisible to a correctness test: engine-generated names must come back `Cow::Borrowed`. Demoting them to the slow path returns an EQUAL `Cow::Owned`, so only the discriminant shows the regression. Measured (criterion, vs the branch point): bytecode_compile/clearn 408.3 -> 297.5ms (-27.1%), full_pipeline/clearn 444.1 -> 328.4ms (-26.1%), ltm_compile/wrld3 320.0 -> 279.9ms (-12.5%), bytecode_compile/wrld3 -4.2%, parse_mdl/clearn -10.3%. Incremental recompile is unchanged.

…first `model::lower_variable` called `lower_ast(scope, ast.clone())`, and `lower_ast` immediately consumed that clone to build an `Expr1` and then an `Expr2`. The clone existed only to be destroyed: `Expr1` is a different tree with different identifier types, so it is constructed from scratch either way, and the only fields the by-value form actually MOVED were a `String` per `Const` and per identifier -- each of which the caller had just allocated in the clone. So the deep copy bought nothing and cost a `Box` per AST node plus a `String` per identifier, per variable, per lowering. `Expr0::clone` was the single largest allocator in a C-LEARN compile: roughly a million of its 6.4 million allocations, showing up under `HashMap<CanonicalElementName, Expr0>::clone` (the per-element equation map of an arrayed variable, cloned wholesale). `Expr1::from` and `IndexExpr1::from` now take `&Expr0` / `&IndexExpr0` and `lower_ast` takes `&Ast<Expr0>`. The one thing that moves the other way is `Ast::ApplyToAll`/`Ast::Arrayed`'s `Vec<Dimension>`, now cloned per lowering rather than moved -- a few dimensions per variable against a whole equation tree per variable. Measured (criterion, vs the branch point): bytecode_compile/clearn 408.3 -> 287.1ms (-29.7%), full_pipeline/clearn 444.1 -> 318.0ms (-28.4%), ltm_compile/wrld3 320.0 -> 270.7ms (-15.4%), bytecode_compile/wrld3 -5.8%. C-LEARN compile allocations 6.44M -> 5.60M.

…er query `db::analysis::reconstruct_model_variables` 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 -- `causal_graph_with_modules`, `causal_graph_from_element_edges_with_modules`, `model_ltm_reference_sites`, the pinned-loop path, `compute_link_polarities` -- so on a world3 LTM compile it ran 923 times and accounted for 58% of every instruction the process executed. Nothing between those calls can change its answer: it reads the model's variable set, the per-variable parse memos and the project dimension context, all of them salsa values. It is now a `#[salsa::tracked]` query returning a shared `Arc<HashMap<Ident, Variable>>`, and `CausalGraph::variables` holds that Arc rather than an owned map, so the sharing survives into every graph built from it. Its salsa dependencies are exactly the ones its callers already took by calling it inline, so invalidation granularity is unchanged. A side effect worth naming: the map's iteration order used to differ between two calls in the same process (each `HashMap` seeds its own `RandomState`) and is now fixed for a revision. That is strictly more deterministic -- any output that depended on the old order was already irreproducible run to run, the GH #595 class. Measured (criterion, vs the branch point): ltm_compile/wrld3 320.0 -> 33.7ms, a 9.5x speedup. Non-LTM compiles are untouched (bytecode_compile/clearn stays at -29.7%). The heavy `#[ignore]`d gates were run: oracle_clearn, oracle_wrld3, simulates_clearn vs Ref.vdf, clearn_residual_exactness, both wasm parity suites, clearn_ltm_discovery, clearn_with_ltm_simulates_model_vars_identically, and the pinned-loop C-LEARN gate all pass.

…y element `build_element_context` resolves an arrayed variable's cross-dimension mapping substitutions, and it built a fresh `DimensionsContext` from the project's dimension list to do it -- once per ELEMENT of every arrayed variable. Building one canonicalizes and interns every dimension's every element name, so on C-LEARN that was 2% of the whole import's instructions spent rebuilding a value that does not change. It is memoized on the `ConversionContext` in a `OnceLock`. The memo is invalidated by a new `push_dimension`, which is now the only way `dimensions` grows -- so the cache is correct by construction rather than by relying on the phase ordering (dimension building does finish before any variable is converted, but nothing enforced that). Making it the only path meant hoisting the equivalence-alias materialization out of its `&self.equivalences` loop, since `push_dimension` takes `&mut self`. Measured (criterion, vs the branch point): parse_mdl/clearn 32.47 -> 20.92ms (-35.6%), full_pipeline/clearn 444.1 -> 308.6ms (-30.5%). The win is larger than the rebuild alone because the memo also keeps the subdimension-relationship cache that rides on the context, which every element used to discard.

`reconstruct_single_variable` did its own parse-and-lower: look the name up among the model's explicit variables, and failing that, walk EVERY variable's parse result hunting for a matching implicit one. The LTM link-score emitter calls it once per link, which on C-LEARN is 6,721 times. Since the previous commit, `reconstruct_model_variables` caches a map holding exactly those variables, built by the same construction against the same scope, so this is a lookup in it. That is not only cheaper -- it means the two can no longer disagree about what a name resolves to, which they could while each searched the model its own way. One precedence detail had to be made to match rather than assumed: this function checked explicit variables FIRST, while the map inserted implicit ones over the top. Implicit names carry the reserved `$⁚` prefix so no collision is reachable and no behavior changes today, but the map now uses `or_insert_with` so explicit-wins is the rule in the one place it is decided. Measured: C-LEARN under LTM 2539 -> 2194ms total, with the synthetic-variable stage 854 -> 500ms (-41%) and peak RSS 567 -> 544 MiB. world3 is unchanged (it has 428 links to C-LEARN's 6,721). The heavy `#[ignore]`d LTM gates pass.

SipHash over a short identifier was 4-6% of a large model's compile cycles, spread over every name resolution the compiler does -- one per AST node, per element, per fragment. The maps on that path (the compiler's symbol table and module map, the dimension context and its element index, codegen's name interner, `model_module_map`) now use a `crate::common::IdentMap` alias over `rustc_hash::FxBuildHasher`, which the crate already depends on. Two consequences, both stated on the alias rather than left implicit. The good one: FxHash's seed is fixed, so iteration order is now reproducible across processes -- the direction this crate already wants, since a salsa-cached value built by iterating a map must not differ run to run (GH #595). The cost: a fixed seed means 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, and 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. Measured (criterion, vs the branch point): bytecode_compile/clearn 408.3 -> 273.9ms (-32.9%), full_pipeline/clearn 444.1 -> 294.3ms (-33.7%), bytecode_compile/wrld3 -6.2%, ltm_compile/wrld3 -89.8%. All 20 runnable `#[ignore]`d heavy gates pass, the determinism suites included.

Ten `#[ignore]`d integration tests were gated on runtime, not on anything about what they assert. The `engine-compile-perf` branch cut C-LEARN's compile by a third and its MDL import by 36%, so that judgement is stale -- and 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 ("this gate is `#[ignore]`d, so neither pre-commit nor CI caught it"). Now in the default run, with their debug-build cost in parentheses: `simulates_clearn` (3.8s -- the engine's cross-simulator ground-truth gate against `Ref.vdf`), `clearn_residual_exactness` (3.8s), `oracle_clearn` (3.7s) and `oracle_wrld3` (0.1s -- the run-invariance soundness oracles), `clearn_ltm_var_count_guardrail` (4.2s), `simulates_wrld3_03_wasm` (1.1s), `corpus_clearn_macros_import` (2.3s), `clearn_unit_error_flood_is_cleared` (1.9s), `metasd_expansion_tier_full` (2.9s), and `corpus_sstats_multi_output_materializes` (0.1s). Cost, measured on a 4-core/4-thread run (`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.1s, against a 180s cap. Scaled to CI's ~60s baseline that is roughly 74s, so the cap keeps a ~2.4x rail. The twenty that stay ignored now say why in terms that will not go stale. Three classes: what is slow is EXECUTION under the non-JIT wasm interpreter, which no compiler speedup touches (`simulates_clearn_wasm` 34s, `discovery_clearn_matches_vm_wasm` 287s); the test is a strict subset of one that now runs by default (`metasd_expansion_tier_heavy`, `compiles_and_runs_clearn_structural`); or it is a diagnostic dump rather than an assertion. The rest are still genuinely over budget. docs/dev/rust.md gains the rule this commit 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 whole suite against a CI-shaped core count.

codecov · 2026-07-29T14:17:06Z

Codecov Report

❌ Patch coverage is 97.85575% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.84%. Comparing base (30637f4) to head (f1417af).

Files with missing lines	Patch %	Lines
src/simlin-engine/src/common.rs	96.96%	4 Missing ⚠️
src/simlin-engine/src/model.rs	83.33%	3 Missing ⚠️
src/simlin-engine/src/db/ltm/compile.rs	96.29%	2 Missing ⚠️
src/simlin-engine/src/compiler/subscript.rs	87.50%	1 Missing ⚠️
src/simlin-engine/src/mdl/convert/dimensions.rs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #993      +/-   ##
==========================================
+ Coverage   91.70%   91.84%   +0.14%     
==========================================
  Files         244      244              
  Lines      157154   157340     +186     
==========================================
+ Hits       144115   144511     +396     
+ Misses      13039    12829     -210

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 915985dfea

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-07-29T14:17:40Z

+    while b < 128 {
+        table[b] = !matches!(
+            b as u8,
+            b'"' | b'.' | b' ' | b'\n' | b'\r' | b'\t' | b'\\' | b'A'..=b'Z'


Route vertical-tab and form-feed names through trimming

When an identifier starts or ends with ASCII vertical tab (U+000B) or form feed (U+000C), this table marks the byte canonical, so is_canonical_needing_no_trim returns true and canonicalize bypasses str::trim. Before this change those characters were stripped, so such datamodel inputs now produce different identifiers and can fail to collide with or resolve references using the trimmed spelling. Mark these two ASCII-whitespace bytes false so the existing slow path preserves the prior behavior.

AGENTS.md reference: AGENTS.md:L97-L97

Useful? React with 👍 / 👎.

Correct, but deliberately not changing it.

The analysis holds: str::trim strips char::is_whitespace, which includes U+000B and U+000C, and the new byte table marks both canonical -- so a name with a leading or trailing vertical tab / form feed is no longer trimmed. (The prior is_canonical was already inconsistent here in its own way: u8::is_ascii_whitespace covers form feed but not vertical tab, so only the trim call was doing the work.)

Declined because no real model has an identifier bounded by a vertical tab or form feed, and the two-byte table change would buy a behavior nobody can reach. Recording it here rather than silently dropping it, so it does not get re-litigated: this is a decision, not an oversight.

chatgpt-codex-connector · 2026-07-29T14:17:40Z

-    }
-
-    None
+    reconstruct_model_variables(db, model, project)


Preserve per-variable invalidation for LTM link scores

When LTM is enabled and an unrelated variable's equation changes, this lookup now depends on reconstruct_model_variables, whose tracked result reads and lowers every variable in the model. The changed map invalidates every link_score_equation_text_shaped query that called this helper, including scores whose source and target did not change; on models such as C-LEARN that means regenerating thousands of link scores after a local edit, defeating the query's documented per-involved-variable incrementality. Keep the cached whole-model map for bulk graph consumers, but use a separately tracked per-variable reconstruction for this helper.

Useful? React with 👍 / 👎.

Good catch, and it was material -- fixed in f1417af.

Verified rather than reasoned about: I added a body-entry counter to link_score_equation_text_shaped and a test that edits one variable in a two-independent-loop fixture. On the commit you flagged it regenerated 4 of 4 link scores; on the implementation that commit replaced it regenerates fewer. So this was a real regression the PR introduced, not a pre-existing condition.

The fix is the shape you suggested, spelled as a salsa firewall rather than a second reconstruction path: reconstruct_single_variable is now a #[salsa::tracked] query. It still reads the whole-model map and still re-executes on any edit, but its VALUE is one variable, so salsa backdates it whenever that variable is untouched and no reader re-runs -- the same pattern db::query::model_variable_by_name already uses over SourceModel::variables. Keeping one derivation (rather than a separate per-variable reconstruction) preserves the property the original change was for: the whole-model map and the single-name lookup cannot disagree about what a name resolves to.

The cold-build win is untouched -- C-LEARN under LTM stays at ~2.1s -- since the query body is still a hash lookup rather than a parse-and-lower.

Worth noting for anyone reading later: the pin has to count query-body entries, not compare memo pointers. Salsa backdates a re-executed query whose value compares equal, so every link score's memo looks untouched whether its body ran or not, and a pointer check would have gone green through the entire defect.

claude · 2026-07-29T14:19:33Z

Review

[P2] Rustdoc for `canonicalize` was accidentally glued onto the new `IdentMap` type

File: src/simlin-engine/src/common.rs around lines 918-944

The insertion that added IdentMap placed its doc-comment block directly after the existing canonicalize doc block with no blank line between them. As a result, the entire block (starting from /// Canonicalize a variable/model name into a normalized form.) now attaches to pub(crate) type IdentMap<K, V> = ... on line 944, and the pub fn canonicalize at line 1027 is left with no doc comment at all. Since canonicalize is a pub API item it appears in generated rustdoc; readers browsing the crate will see the canonicalize documentation as a preamble to the IdentMap type description. The fix is to insert a blank line before the /// The engine's hash map for identifier-keyed lookups. line (or move the IdentMap definition elsewhere) so the two doc blocks are separate.

Overall correctness verdict

Correct — this is documentation-only; the behavior of the compile pipeline, canonicalization, IdentMap, and the reconstruct-caching changes is sound. The is_canonical_needing_no_trim fast path is conservative in the safe direction (matches the slow-path answer whenever it accepts), the fused ASCII whitespace/lowercase pass is equivalent to replace_whitespace_with_underscore + to_lowercase for ASCII input, the shared CanonicalStorage Arc in the new From<&Ident<Canonical>> impls preserves interner identity, and the OnceLock-cached dimensions_context() is correctly invalidated by the single push_dimension mutation site.

…s not whole-model Review catch on PR #993 (r3675119315). Routing `reconstruct_single_variable` straight through the cached whole-model `reconstruct_model_variables` map made every consumer depend on every variable's LOWERED form, because that is what the map holds. `link_score_equation_text_shaped` is tracked per `(from, to, shape)` and documents that "a per-shape link score is recomputed only when the involved variables ... change" -- and that stopped being true: one unrelated equation edit regenerated all of them. On C-LEARN that is 6,721 link scores per keystroke. The lookup is now a `#[salsa::tracked]` query. 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 and no reader re-runs. Same shape and same reason as `db::query::model_variable_by_name` over `SourceModel::variables`. The cold-build win the map lookup bought is untouched -- C-LEARN under LTM stays at ~2.1s -- because the query body is still a hash lookup rather than a parse-and-lower. The regression pin is a body-entry COUNT, not memo identity, and that is load-bearing: salsa backdates a re-executed query whose value compares equal, so every link score's memo looks untouched whether its body ran or not -- a pointer check would have gone green through the whole defect. The new counter lives beside the query. The test asserts "strictly fewer than the cold build" rather than an exact number: WHICH scores touch the edited variable is an emitter property that may legitimately change, while "an edit in one loop must not regenerate an independent loop's scores" is the contract, and an exact pin would fail on every unrelated emitter change and teach nothing. Verified in both directions rather than assumed: the test fails on the previous commit (4 of 4 scores regenerated) and passes on the implementation that commit replaced, so it pins a real regression this change introduced rather than a pre-existing condition.

claude · 2026-07-29T17:29:01Z

[P2] Fast-path `is_canonical_needing_no_trim` accepts VT/FF, diverges from `trim` — src/simlin-engine/src/common.rs:970-981

The CANONICAL_BYTE table only excludes " / . / space / \n / \r / \t / \\ / A..=Z, but str::trim (via char::is_whitespace, i.e. Unicode White_Space) also strips vertical tab (0x0B) and form feed (0x0C). The fast path therefore treats a name containing either as "canonical, no trim needed" and returns Cow::Borrowed(name), whereas the pre-PR path (name.trim() then is_canonical(trimmed)) returned the trimmed slice. Concretely, canonicalize("hello\x0C") and canonicalize("hello\x0B") used to return "hello" but now return the untrimmed input, so "hello" and "hello\x0B" (which used to be equal identifiers) become distinct map keys — silently splitting downstream lookups and duplicate-canonical detection. This violates the fn'"'"'s own stated contract ("Nothing it ACCEPTS may be non-canonical") in the doc comment. Low-frequency in practice (few model files carry VT/FF in identifiers), and neither the property test around line 1571 (bounded control-char exclusion) nor SLOW_PATH_ALPHABET exercises them. Fix: extend the !matches! set to also exclude b'\x0B' and b'\x0C' (any ASCII byte in Unicode White_Space), and add coverage.

Overall correctness: correct (with one low-severity edge case)

The optimization pass is otherwise well-verified. The Expr1::from / IndexExpr1::from by-reference rewrite handles all Expr0 variants correctly (only Const'"'"'s String field needs cloning; every other field is Copy). The salsa caching of reconstruct_model_variables via Arc<HashMap> is sound — the Arc is treated read-only after construction, OnceLock memoization keys are correct, and the intentional behavior changes (get_dimensions LAST-wins agreement with DimensionsContext, or_insert_with for explicit-vs-implicit precedence) are consistent across all consumers. The IdentMap / FxHash trade-off is documented on the alias with a well-scoped security caveat. Only the canonicalization edge case above needs attention.

bpowers added 9 commits July 28, 2026 18:47

chatgpt-codex-connector Bot reviewed Jul 29, 2026

View reviewed changes

bpowers merged commit e913b3f into main Jul 29, 2026
19 checks passed

bpowers deleted the engine-compile-perf branch July 29, 2026 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

engine: compiler optimization pass (C-LEARN -33%, LTM on world3 9.8x, LTM on C-LEARN 101x) - #993

engine: compiler optimization pass (C-LEARN -33%, LTM on world3 9.8x, LTM on C-LEARN 101x)#993
bpowers merged 10 commits into
mainfrom
engine-compile-perf

bpowers commented Jul 29, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 29, 2026

Uh oh!

bpowers Jul 29, 2026

Uh oh!

chatgpt-codex-connector Bot Jul 29, 2026

Uh oh!

bpowers Jul 29, 2026

Uh oh!

claude Bot commented Jul 29, 2026

Uh oh!

Uh oh!

claude Bot commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

bpowers commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Results

What was actually wrong

Review follow-ups

Non-obvious decisions

Measurement notes

Tests un-ignored

On #655

Not fixed

Verification

Uh oh!

codecov Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

bpowers Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

bpowers Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 29, 2026

Review

[P2] Rustdoc for canonicalize was accidentally glued onto the new IdentMap type

Overall correctness verdict

Uh oh!

Uh oh!

claude Bot commented Jul 29, 2026

[P2] Fast-path is_canonical_needing_no_trim accepts VT/FF, diverges from trim — src/simlin-engine/src/common.rs:970-981

Overall correctness: correct (with one low-severity edge case)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bpowers commented Jul 29, 2026 •

edited

Loading

codecov Bot commented Jul 29, 2026 •

edited

Loading

[P2] Rustdoc for `canonicalize` was accidentally glued onto the new `IdentMap` type

[P2] Fast-path `is_canonical_needing_no_trim` accepts VT/FF, diverges from `trim` — src/simlin-engine/src/common.rs:970-981