From 0dde5da939ef8b7692930ca3177eda1395c11a2b Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 18:47:35 -0700 Subject: [PATCH 01/10] engine: parse against the cached dimension context, not a rebuilt one `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` impls, sound for the same reason `Ident`'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. --- src/simlin-engine/benches/compiler.rs | 59 +++++++++++++++++++- src/simlin-engine/src/common.rs | 19 +++++++ src/simlin-engine/src/db/analysis.rs | 25 +++------ src/simlin-engine/src/db/fragment_compile.rs | 3 +- src/simlin-engine/src/db/implicit_deps.rs | 12 ++-- src/simlin-engine/src/db/ltm/compile.rs | 18 +++--- src/simlin-engine/src/db/ltm/equation.rs | 3 +- src/simlin-engine/src/db/ltm/mod.rs | 6 +- src/simlin-engine/src/db/ltm/parse.rs | 6 +- src/simlin-engine/src/db/ltm_tests.rs | 2 +- src/simlin-engine/src/db/prev_init_tests.rs | 34 +++++++---- src/simlin-engine/src/db/query.rs | 17 ++++-- src/simlin-engine/src/db/stages.rs | 6 +- src/simlin-engine/src/db/tests.rs | 2 +- src/simlin-engine/src/db/units.rs | 9 ++- src/simlin-engine/src/dimensions.rs | 32 ++++++++--- src/simlin-engine/src/ltm_augment_tests.rs | 8 +-- src/simlin-engine/src/model.rs | 11 +++- src/simlin-engine/src/variable.rs | 55 ++++++++++-------- 19 files changed, 219 insertions(+), 108 deletions(-) diff --git a/src/simlin-engine/benches/compiler.rs b/src/simlin-engine/benches/compiler.rs index df19b2ef1..07959ed43 100644 --- a/src/simlin-engine/benches/compiler.rs +++ b/src/simlin-engine/benches/compiler.rs @@ -9,6 +9,9 @@ //! //! - `parse_mdl` -- MDL text -> `datamodel::Project` (lexing + parsing + conversion) //! - `bytecode_compile` -- `datamodel::Project` -> `CompiledSimulation` (bytecode generation) +//! - `ltm_compile` -- the same, with LTM (Loops That Matter) enabled, which adds +//! causal-edge extraction, loop enumeration, and synthetic link/loop-score +//! variable generation on top of ordinary compilation //! - `full_pipeline` -- MDL text -> `CompiledSimulation` (all stages end-to-end) //! - `salsa_incremental` -- measures `compile_project_incremental` on synthetic //! chain models after a single-variable equation edit or variable add/remove @@ -29,7 +32,9 @@ use std::time::Duration; use criterion::{BenchmarkId, Criterion, black_box, criterion_group, criterion_main}; use simlin_engine::datamodel::{self, Aux, Compat, Dt, Equation, SimMethod, SimSpecs, Variable}; -use simlin_engine::db::{SimlinDb, compile_project_incremental, sync_from_datamodel_incremental}; +use simlin_engine::db::{ + SimlinDb, compile_project_incremental, set_project_ltm_enabled, sync_from_datamodel_incremental, +}; use simlin_engine::open_vensim; /// Model metadata for benchmark parameterization. @@ -132,6 +137,57 @@ fn bench_bytecode_compile(c: &mut Criterion) { group.finish(); } +/// Models the `ltm_compile` group runs. Deliberately not all of [`MODELS`]: +/// C-LEARN under LTM is a multi-minute compile (~37k synthetic variables), far +/// outside a criterion sample budget. WRLD3 is the largest model that compiles +/// with LTM in a benchmarkable time, and it is the model this group exists for +/// -- its 428 causal edges each get a synthetic link-score variable, so the +/// LTM-only stages dominate its ordinary compile by roughly an order of +/// magnitude. +static LTM_MODELS: &[&str] = &["wrld3"]; + +/// Benchmark: datamodel::Project -> CompiledSimulation with LTM enabled. +/// +/// Covers the whole LTM surface -- element causal edges, loop enumeration, +/// synthetic link/loop-score generation (`model_ltm_variables`), and the +/// per-link fragment compiles -- on top of the ordinary bytecode compile the +/// `bytecode_compile` group measures, so the difference between the two groups +/// is the LTM cost. +fn bench_ltm_compile(c: &mut Criterion) { + let mut group = c.benchmark_group("ltm_compile"); + group.measurement_time(Duration::from_secs(10)); + + for fixture in MODELS.iter().filter(|f| LTM_MODELS.contains(&f.name)) { + let contents = load_model(fixture); + let datamodel = open_vensim(&contents).unwrap(); + + if !is_simulatable(&datamodel) { + eprintln!( + "skipping ltm_compile/{}: model is not simulatable", + fixture.name + ); + continue; + } + + group.bench_with_input( + BenchmarkId::from_parameter(fixture.name), + &datamodel, + |b, datamodel| { + b.iter(|| { + let mut db = SimlinDb::default(); + let state = sync_from_datamodel_incremental(&mut db, datamodel, None); + // The flag rides on the project input, so it must be set + // before the first compile; every LTM query reads it. + set_project_ltm_enabled(&mut db, state.project, true); + black_box(compile_project_incremental(&db, state.project, "main").unwrap()) + }); + }, + ); + } + + group.finish(); +} + /// Benchmark: MDL text -> CompiledSimulation (full pipeline). /// /// Measures the entire compilation pipeline end-to-end, including parsing, @@ -317,6 +373,7 @@ criterion_group!( benches, bench_parse_mdl, bench_bytecode_compile, + bench_ltm_compile, bench_full_pipeline, bench_incremental_equation_edit, bench_incremental_add_remove, diff --git a/src/simlin-engine/src/common.rs b/src/simlin-engine/src/common.rs index 3e963c78a..bf1cfd480 100644 --- a/src/simlin-engine/src/common.rs +++ b/src/simlin-engine/src/common.rs @@ -2667,6 +2667,25 @@ impl std::borrow::Borrow for Ident { } } +// The same, for the two other newtypes over `CanonicalStorage`. Probing a map +// with a `&str` skips the global interner entirely: constructing a +// `CanonicalDimensionName`/`CanonicalElementName` just to ask whether a key is +// present takes a sharded mutex, allocates an `Arc` payload on first sighting, +// and bumps/drops a refcount every time -- all to produce a value the lookup +// discards. Sound for exactly the reason above: `CanonicalStorage::hash` hashes +// the string content, so a `&str` probe hashes identically to the stored key. +impl std::borrow::Borrow for CanonicalDimensionName { + fn borrow(&self) -> &str { + self.0.as_str() + } +} + +impl std::borrow::Borrow for CanonicalElementName { + fn borrow(&self) -> &str { + self.0.as_str() + } +} + impl<'a> AsRef for IdentRef<'a, Canonical> { fn as_ref(&self) -> &str { self.inner diff --git a/src/simlin-engine/src/db/analysis.rs b/src/simlin-engine/src/db/analysis.rs index 4fab5a244..4b33bc314 100644 --- a/src/simlin-engine/src/db/analysis.rs +++ b/src/simlin-engine/src/db/analysis.rs @@ -3356,9 +3356,8 @@ pub(crate) fn reconstruct_model_variables( let source_vars = model.variables(db); let module_ctx = model_module_ident_context(db, model, project, vec![]); - // The datamodel dims are needed by `reconstruct_implicit_variable`; the - // canonicalized context comes from the project-global salsa-cached query. - let dims = project_datamodel_dims(db, project); + // The canonicalized dimension context comes from the project-global + // salsa-cached query; every parse and lowering below reads that one value. let dim_context = project_dimensions_context(db, project); let models = HashMap::new(); let scope = crate::model::ScopeStage0 { @@ -3382,7 +3381,7 @@ pub(crate) fn reconstruct_model_variables( // must keep module wiring.) let dm_var = super::datamodel_variable_from_source(db, *source_var); if matches!(dm_var, datamodel::Variable::Module(_)) { - let lowered = reconstruct_implicit_variable(db, model, dims, &scope, &dm_var); + let lowered = reconstruct_implicit_variable(db, model, &scope, &dm_var); variables.insert(Ident::new(name), lowered); continue; } @@ -3395,8 +3394,7 @@ pub(crate) fn reconstruct_model_variables( // Add implicit variables (module instances from SMOOTH/DELAY expansion) for implicit_dm_var in &parsed.implicit_vars { let imp_name = canonicalize(implicit_dm_var.get_ident()).into_owned(); - let lowered_imp = - reconstruct_implicit_variable(db, model, dims, &scope, implicit_dm_var); + let lowered_imp = reconstruct_implicit_variable(db, model, &scope, implicit_dm_var); variables.insert(Ident::new(&imp_name), lowered_imp); } } @@ -3419,9 +3417,8 @@ pub(super) fn reconstruct_single_variable( let source_vars = model.variables(db); let module_ctx = model_module_ident_context(db, model, project, vec![]); - // The datamodel dims are needed by `reconstruct_implicit_variable`; the - // canonicalized context comes from the project-global salsa-cached query. - let dims = project_datamodel_dims(db, project); + // The canonicalized dimension context comes from the project-global + // salsa-cached query; every parse and lowering below reads that one value. let dim_context = project_dimensions_context(db, project); let models = HashMap::new(); let scope = crate::model::ScopeStage0 { @@ -3445,9 +3442,7 @@ pub(super) fn reconstruct_single_variable( // input→user-module link score. let dm_var = super::datamodel_variable_from_source(db, *source_var); if matches!(dm_var, datamodel::Variable::Module(_)) { - return Some(reconstruct_implicit_variable( - db, model, dims, &scope, &dm_var, - )); + return Some(reconstruct_implicit_variable(db, model, &scope, &dm_var)); } let parsed = parse_source_variable_with_module_context(db, *source_var, project, module_ctx); @@ -3464,8 +3459,7 @@ pub(super) fn reconstruct_single_variable( for implicit_dm_var in &parsed.implicit_vars { let imp_name = canonicalize(implicit_dm_var.get_ident()).into_owned(); if Ident::::new(&imp_name) == canonical_target { - let lowered_imp = - reconstruct_implicit_variable(db, model, dims, &scope, implicit_dm_var); + let lowered_imp = reconstruct_implicit_variable(db, model, &scope, implicit_dm_var); return Some(lowered_imp); } } @@ -3483,7 +3477,6 @@ pub(super) fn reconstruct_single_variable( fn reconstruct_implicit_variable( db: &dyn Db, model: SourceModel, - dims: &[datamodel::Dimension], scope: &crate::model::ScopeStage0<'_>, implicit_dm_var: &datamodel::Variable, ) -> crate::variable::Variable { @@ -3514,7 +3507,7 @@ fn reconstruct_implicit_variable( let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut dummy_implicits = Vec::new(); let parsed_imp = crate::variable::parse_var( - dims, + scope.dimensions, implicit_dm_var, &mut dummy_implicits, &units_ctx, diff --git a/src/simlin-engine/src/db/fragment_compile.rs b/src/simlin-engine/src/db/fragment_compile.rs index 6ca3a3185..761c0a10b 100644 --- a/src/simlin-engine/src/db/fragment_compile.rs +++ b/src/simlin-engine/src/db/fragment_compile.rs @@ -356,14 +356,13 @@ fn lower_implicit_var<'db>( let implicit_dm_var = parsed.implicit_vars.get(meta.index_in_parent)?; let implicit_name = canonicalize(implicit_dm_var.get_ident()).into_owned(); - let dm_dims = project_datamodel_dims(db, project); let dim_context = project_dimensions_context(db, project); let units_ctx = project_units_context(db, project); let mut dummy_implicits = Vec::new(); let parsed_implicit = crate::variable::parse_var( - dm_dims, + dim_context, implicit_dm_var, &mut dummy_implicits, units_ctx, diff --git a/src/simlin-engine/src/db/implicit_deps.rs b/src/simlin-engine/src/db/implicit_deps.rs index 120dd568e..31dbb6590 100644 --- a/src/simlin-engine/src/db/implicit_deps.rs +++ b/src/simlin-engine/src/db/implicit_deps.rs @@ -23,14 +23,12 @@ pub struct ImplicitVarDeps { pub referenced_tables: BTreeSet, } -/// `dims`, `dim_context`, and `converted_dims` are three views of the *same* -/// project dimension list (datamodel form, context form, and converted form); -/// the caller sources all three from the per-project salsa caches, so they are -/// consistent by construction. Passing inconsistent slices would silently -/// misclassify dependencies. +/// `dim_context` and `converted_dims` are two views of the *same* project +/// dimension list (context form and converted form); the caller sources both +/// from the per-project salsa caches, so they are consistent by construction. +/// Passing inconsistent views would silently misclassify dependencies. pub(super) fn extract_implicit_var_deps( parsed: &ParsedVariableResult, - dims: &[datamodel::Dimension], dim_context: &crate::dimensions::DimensionsContext, converted_dims: &[crate::dimensions::Dimension], module_inputs: Option<&BTreeSet>>, @@ -77,7 +75,7 @@ pub(super) fn extract_implicit_var_deps( let mut dummy_implicits = Vec::new(); let parsed_implicit = crate::variable::parse_var( - dims, + dim_context, implicit_var, &mut dummy_implicits, &units_ctx, diff --git a/src/simlin-engine/src/db/ltm/compile.rs b/src/simlin-engine/src/db/ltm/compile.rs index d6b85a1b6..d3eea2f02 100644 --- a/src/simlin-engine/src/db/ltm/compile.rs +++ b/src/simlin-engine/src/db/ltm/compile.rs @@ -26,8 +26,8 @@ use crate::db::{ compile_phase_to_per_var_bytecodes, compute_layout, extract_tables_from_source_var, fragment_emit_ctx, model_dependency_graph, model_implicit_var_info, model_module_ident_context, model_module_map, parse_source_variable_with_module_context, project_converted_dimensions, - project_datamodel_dims, project_dimensions_context, project_units_context, - reconstruct_single_variable, variable_dimensions, variable_size, + project_dimensions_context, project_units_context, reconstruct_single_variable, + variable_dimensions, variable_size, }; use super::parse::{ltm_equation_dimensions, parse_ltm_equation, scalarize_ltm_equation}; @@ -785,7 +785,7 @@ fn lower_ltm_variable( let model_name_str = model.name(db); let module_ctx = model_module_ident_context(db, model, project, vec![]); - let dims = project_datamodel_dims(db, project); + let dim_ctx = project_dimensions_context(db, project); let units_ctx = project_units_context(db, project); let mut stage0_vars: HashMap, crate::model::VariableStage0> = HashMap::new(); stage0_vars.insert(Ident::new(parsed_variable.ident()), parsed_variable.clone()); @@ -799,7 +799,7 @@ fn lower_ltm_variable( // in their own right; here only the dep's own dimensions matter. let mut nested = Vec::new(); let dep_parsed = - crate::variable::parse_var(dims, implicit_dm, &mut nested, units_ctx, |mi| { + crate::variable::parse_var(dim_ctx, implicit_dm, &mut nested, units_ctx, |mi| { Ok(Some(mi.clone())) }); stage0_vars.insert(Ident::new(dep_name), dep_parsed); @@ -853,7 +853,6 @@ pub(crate) fn compile_ltm_equation_fragment( // Project-global dims (datamodel form, used to resolve the equation's // dimension names) plus the canonicalized context + converted dims, all // from the salsa-cached queries rather than rebuilt per LTM fragment. - let dims = project_datamodel_dims(db, project); let dim_context = project_dimensions_context(db, project); let converted_dims = project_converted_dimensions(db, project); @@ -865,7 +864,7 @@ pub(crate) fn compile_ltm_equation_fragment( let parsed = parse_ltm_equation( var_name, equation, - dims, + dim_context, Some(module_idents), Some(model_var_names), ); @@ -1968,9 +1967,8 @@ pub(crate) fn compile_ltm_implicit_var_fragment( } } - // Project-global dims (datamodel form needed by `parse_var`) plus the - // canonicalized context + converted dims, from the salsa-cached queries. - let dims = project_datamodel_dims(db, project); + // The project-global canonicalized dimension context + converted dims, + // from the salsa-cached queries. let dim_context = project_dimensions_context(db, project); let converted_dims = project_converted_dimensions(db, project); @@ -1978,7 +1976,7 @@ pub(crate) fn compile_ltm_implicit_var_fragment( let mut dummy_implicits = Vec::new(); let parsed_implicit = crate::variable::parse_var( - dims, + dim_context, implicit_dm_var, &mut dummy_implicits, units_ctx, diff --git a/src/simlin-engine/src/db/ltm/equation.rs b/src/simlin-engine/src/db/ltm/equation.rs index 8b234d148..b799480c0 100644 --- a/src/simlin-engine/src/db/ltm/equation.rs +++ b/src/simlin-engine/src/db/ltm/equation.rs @@ -26,7 +26,6 @@ use std::collections::HashMap; use crate::ast::{Ast, Expr0}; use crate::common::{CanonicalElementName, EquationError}; -use crate::datamodel; use crate::lexer::LexerType; /// One equation arm: the authoritative parsed AST plus its diagnostic text. @@ -306,7 +305,7 @@ impl LtmEquation { /// init-phase ast to build. pub fn to_flow_ast( &self, - dimensions: &[datamodel::Dimension], + dimensions: &crate::dimensions::DimensionsContext, ) -> (Option>, Vec) { // A generated arm that failed to parse rejects the whole equation, // BEFORE any shape-specific assembly -- see the note above on why the diff --git a/src/simlin-engine/src/db/ltm/mod.rs b/src/simlin-engine/src/db/ltm/mod.rs index ba1027eef..d656899b7 100644 --- a/src/simlin-engine/src/db/ltm/mod.rs +++ b/src/simlin-engine/src/db/ltm/mod.rs @@ -29,7 +29,7 @@ use crate::ltm::strip_subscript; use super::{ Db, SourceModel, SourceProject, SourceVariable, SourceVariableKind, compute_layout, model_causal_edges, model_implicit_var_info, project_datamodel_dims, - reconstruct_single_variable, + project_dimensions_context, reconstruct_single_variable, }; mod compile; @@ -846,7 +846,7 @@ pub fn model_ltm_implicit_var_info( let ltm_vars = model_ltm_variables(db, model, project); - let dims = project_datamodel_dims(db, project); + let dim_ctx = project_dimensions_context(db, project); let module_idents = ltm_module_idents(db, model, project); let model_var_names = ltm_model_var_names(db, model, project); @@ -856,7 +856,7 @@ pub fn model_ltm_implicit_var_info( let parsed = parse_ltm_equation( <m_var.name, <m_var.equation, - dims, + dim_ctx, Some(module_idents), Some(model_var_names), ); diff --git a/src/simlin-engine/src/db/ltm/parse.rs b/src/simlin-engine/src/db/ltm/parse.rs index 508957940..a025e8679 100644 --- a/src/simlin-engine/src/db/ltm/parse.rs +++ b/src/simlin-engine/src/db/ltm/parse.rs @@ -21,7 +21,6 @@ use std::collections::HashSet; use crate::builtins_visitor::{empty_macro_registry, instantiate_implicit_modules}; use crate::common::{Canonical, Ident}; -use crate::datamodel; use crate::dimensions::DimensionsContext; use crate::db::ParsedVariableResult; @@ -46,11 +45,10 @@ use super::LtmEquation; pub(super) fn parse_ltm_equation( var_name: &str, equation: &LtmEquation, - dims: &[datamodel::Dimension], + dims: &DimensionsContext, module_idents: Option<&HashSet>>, model_var_names: Option<&HashSet>>, ) -> ParsedVariableResult { - let dimensions_ctx = DimensionsContext::from(dims); let (flow_ast, mut errors) = equation.to_flow_ast(dims); let mut implicit_vars = Vec::new(); @@ -58,7 +56,7 @@ pub(super) fn parse_ltm_equation( Some(ast) => match instantiate_implicit_modules( var_name, ast, - Some(&dimensions_ctx), + Some(dims), module_idents, model_var_names, // LTM synthetic equations are engine-generated and never contain diff --git a/src/simlin-engine/src/db/ltm_tests.rs b/src/simlin-engine/src/db/ltm_tests.rs index cd5dfcc8c..5a0485a13 100644 --- a/src/simlin-engine/src/db/ltm_tests.rs +++ b/src/simlin-engine/src/db/ltm_tests.rs @@ -726,7 +726,7 @@ fn unparseable_generated_arm_degrades_loudly_without_panicking() { let db = SimlinDb::default(); let sync = sync_from_datamodel(&db, &project); let model = sync.models["main"].source; - let dims = crate::db::project_datamodel_dims(&db, sync.project); + let dims = crate::db::project_dimensions_context(&db, sync.project); // (b) Scalar: no ast, errors surfaced, fragment rejected. let scalar = LtmEquation::scalar(bad.to_string()); diff --git a/src/simlin-engine/src/db/prev_init_tests.rs b/src/simlin-engine/src/db/prev_init_tests.rs index cc01198d0..c378a1022 100644 --- a/src/simlin-engine/src/db/prev_init_tests.rs +++ b/src/simlin-engine/src/db/prev_init_tests.rs @@ -143,9 +143,13 @@ fn test_previous_qualified_element_subscript_no_helper() { let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit_vars: Vec = Vec::new(); let parsed: crate::variable::Variable = - crate::variable::parse_var(&dims, &aux, &mut implicit_vars, &units_ctx, |mi| { - Ok(Some(mi.clone())) - }); + crate::variable::parse_var( + &crate::dimensions::DimensionsContext::from(dims.as_slice()), + &aux, + &mut implicit_vars, + &units_ctx, + |mi| Ok(Some(mi.clone())), + ); assert!( parsed.equation_errors().is_none(), "equation should parse cleanly: {:?}", @@ -338,9 +342,13 @@ fn test_init_qualified_element_subscript_no_helper() { let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit_vars: Vec = Vec::new(); let parsed: crate::variable::Variable = - crate::variable::parse_var(&dims, &aux, &mut implicit_vars, &units_ctx, |mi| { - Ok(Some(mi.clone())) - }); + crate::variable::parse_var( + &crate::dimensions::DimensionsContext::from(dims.as_slice()), + &aux, + &mut implicit_vars, + &units_ctx, + |mi| Ok(Some(mi.clone())), + ); assert!( parsed.equation_errors().is_none(), "equation should parse cleanly: {:?}", @@ -421,7 +429,7 @@ fn test_previous_bare_element_no_helper_with_var_names() { let mut implicit_vars: Vec = Vec::new(); let parsed: crate::variable::Variable = crate::variable::parse_var_with_module_context( - &dims, + &crate::dimensions::DimensionsContext::from(dims.as_slice()), &aux, &mut implicit_vars, &units_ctx, @@ -456,7 +464,7 @@ fn test_previous_bare_element_no_helper_with_var_names() { let mut implicit_vars: Vec = Vec::new(); let parsed: crate::variable::Variable = crate::variable::parse_var_with_module_context( - &dims, + &crate::dimensions::DimensionsContext::from(dims.as_slice()), &aux, &mut implicit_vars, &units_ctx, @@ -481,9 +489,13 @@ fn test_previous_bare_element_no_helper_with_var_names() { // conservative helper path is also kept. let mut implicit_vars: Vec = Vec::new(); let parsed: crate::variable::Variable = - crate::variable::parse_var(&dims, &aux, &mut implicit_vars, &units_ctx, |mi| { - Ok(Some(mi.clone())) - }); + crate::variable::parse_var( + &crate::dimensions::DimensionsContext::from(dims.as_slice()), + &aux, + &mut implicit_vars, + &units_ctx, + |mi| Ok(Some(mi.clone())), + ); assert!(parsed.equation_errors().is_none()); assert_eq!( implicit_vars.len(), diff --git a/src/simlin-engine/src/db/query.rs b/src/simlin-engine/src/db/query.rs index 9fa20d672..1b9420142 100644 --- a/src/simlin-engine/src/db/query.rs +++ b/src/simlin-engine/src/db/query.rs @@ -189,11 +189,18 @@ fn parse_source_variable_impl( .cloned() .collect() }; + // Built from the NARROWED `dims`, not from the project-wide + // `project_dimensions_context`: this parse's dimension dependency is + // deliberately only the dimensions the variable references, so that an + // unrelated dimension edit does not invalidate every cached parse + // (`db::dimension_invalidation_tests`). A scalar variable's `dims` is + // empty, so this is free in the common case. + let dim_ctx = crate::dimensions::DimensionsContext::from(&dims); let units_ctx = project_units_context(db, project); let dm_var = datamodel_variable_from_source(db, var); let mut implicit_vars = Vec::new(); let variable = crate::variable::parse_var_with_module_context( - &dims, + &dim_ctx, &dm_var, &mut implicit_vars, units_ctx, @@ -338,10 +345,8 @@ fn variable_direct_dependencies_impl( _ => { let parsed = parse_source_variable_with_module_context(db, var, project, module_ident_context); - // The datamodel-form dims are still needed for the implicit-var - // parse below; the canonicalized context + converted dims come from - // the project-global salsa-cached queries (no per-variable rebuild). - let dims = project_datamodel_dims(db, project); + // The canonicalized context + converted dims come from the + // project-global salsa-cached queries (no per-variable rebuild). let dim_context = project_dimensions_context(db, project); let converted_dims = project_converted_dimensions(db, project); let models = HashMap::new(); @@ -367,7 +372,7 @@ fn variable_direct_dependencies_impl( }; let implicit_vars = - extract_implicit_var_deps(parsed, dims, dim_context, converted_dims, module_inputs); + extract_implicit_var_deps(parsed, dim_context, converted_dims, module_inputs); VariableDeps { dt_deps: dt_classification diff --git a/src/simlin-engine/src/db/stages.rs b/src/simlin-engine/src/db/stages.rs index 42bfe7ee5..195e0b164 100644 --- a/src/simlin-engine/src/db/stages.rs +++ b/src/simlin-engine/src/db/stages.rs @@ -83,7 +83,7 @@ use crate::common::{Canonical, Ident}; use crate::datamodel; use crate::db::{ Db, ModuleIdentContext, SourceModel, SourceProject, model_duplicate_variables, - model_module_ident_context, parse_source_variable_with_module_context, project_datamodel_dims, + model_module_ident_context, parse_source_variable_with_module_context, project_dimensions_context, project_units_context, }; use crate::model::{ModelStage0, ModelStage1, ScopeStage0, VariableStage0}; @@ -485,7 +485,7 @@ pub(crate) fn model_stage0(db: &dyn Db, model: SourceModel, project: SourceProje note_execution(&STAGE0_EXECUTIONS); let units_ctx = project_units_context(db, project); - let dm_dims = project_datamodel_dims(db, project); + let dim_ctx = project_dimensions_context(db, project); let display_name = model.name(db); let ident: Ident = Ident::new(display_name); @@ -510,7 +510,7 @@ pub(crate) fn model_stage0(db: &dyn Db, model: SourceModel, project: SourceProje // second generation into the `nested` sink. let mut nested_implicit: Vec = Vec::new(); var_list.extend(implicit_dm.into_iter().map(|dm_var| { - crate::variable::parse_var(dm_dims, &dm_var, &mut nested_implicit, units_ctx, |mi| { + crate::variable::parse_var(dim_ctx, &dm_var, &mut nested_implicit, units_ctx, |mi| { Ok(Some(mi.clone())) }) })); diff --git a/src/simlin-engine/src/db/tests.rs b/src/simlin-engine/src/db/tests.rs index 76eb1ccc9..e1d85f52e 100644 --- a/src/simlin-engine/src/db/tests.rs +++ b/src/simlin-engine/src/db/tests.rs @@ -565,7 +565,7 @@ fn test_parse_source_variable_matches_direct_parse() { let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit_vars = Vec::new(); let direct_result = parse_var( - &project.dimensions, + &crate::dimensions::DimensionsContext::from(project.dimensions.as_slice()), dm_var, &mut implicit_vars, &units_ctx, diff --git a/src/simlin-engine/src/db/units.rs b/src/simlin-engine/src/db/units.rs index eca30fa8b..cc95abd3f 100644 --- a/src/simlin-engine/src/db/units.rs +++ b/src/simlin-engine/src/db/units.rs @@ -46,8 +46,7 @@ use crate::datamodel; use crate::db::{ CompilationDiagnostic, Db, Diagnostic, DiagnosticError, DiagnosticSeverity, SourceModel, SourceProject, SourceVariable, model_scope_models, model_scope_stage0, model_stage0, - model_stage1, project_datamodel_dims, project_dimensions_context, project_units_context, - source_model_is_stdlib, + model_stage1, project_dimensions_context, project_units_context, source_model_is_stdlib, }; /// Collect the identifiers that must share units because they sit in the @@ -656,11 +655,11 @@ fn check_conveyor_param_units( // inference and the ordinary unit check) or the cached stage itself: the // synthetic auxes must not add constraints to the model under analysis, and // must never reach another reader of the memo. - let dm_dims = project_datamodel_dims(db, project); + let dim_ctx = project_dimensions_context(db, project); let mut aug_ms0 = model_stage0(db, model, project).clone(); for dm_var in &synth_dm_vars { let mut dummy: Vec = Vec::new(); - let vs0 = crate::variable::parse_var(dm_dims, dm_var, &mut dummy, units_ctx, |mi| { + let vs0 = crate::variable::parse_var(dim_ctx, dm_var, &mut dummy, units_ctx, |mi| { Ok(Some(mi.clone())) }); aug_ms0.variables.insert(Ident::new(vs0.ident()), vs0); @@ -673,7 +672,7 @@ fn check_conveyor_param_units( let models_s0 = model_scope_stage0(db, model, project); let scope = crate::model::ScopeStage0 { models: &models_s0, - dimensions: project_dimensions_context(db, project), + dimensions: dim_ctx, model_name: aug_ms0.ident.as_str(), }; let aug_s1 = crate::model::ModelStage1::new(&scope, &aug_ms0); diff --git a/src/simlin-engine/src/dimensions.rs b/src/simlin-engine/src/dimensions.rs index 94aeb6680..8d23692f3 100644 --- a/src/simlin-engine/src/dimensions.rs +++ b/src/simlin-engine/src/dimensions.rs @@ -39,8 +39,12 @@ impl NamedDimension { /// The input should be in canonical form (lowercase, spaces as underscores). /// This is more efficient than iterating through `elements` for large dimensions. pub fn get_element_index(&self, element: &str) -> Option { + // Probe by `&str` (`CanonicalElementName: Borrow`) rather than + // building a `CanonicalElementName`: a lookup has no reason to take a + // shard of the global interner and install an `Arc` payload for a name + // it is only asking about. self.indexed_elements - .get(&CanonicalElementName::from_raw(element)) + .get(crate::common::canonicalize(element).as_ref()) .map(|&idx| idx - 1) // Convert from 1-based to 0-based } } @@ -414,17 +418,31 @@ impl DimensionsContext { self.dimensions.get(name) } + /// Get a dimension by a name that may not be in canonical form yet. + /// + /// The `&str` probe (via `CanonicalDimensionName: Borrow`) is the + /// point: a lookup must not intern. `CanonicalDimensionName::from_raw` + /// takes a shard of the global interner and installs an `Arc` payload for + /// a name that may not even be a dimension, and every miss leaves a + /// refcount round trip behind -- on the parse path that is per dimension + /// reference per variable. + pub(crate) fn get_by_raw_name(&self, name: &str) -> Option<&Dimension> { + self.dimensions + .get(crate::common::canonicalize(name).as_ref()) + } + pub(crate) fn is_dimension_name(&self, name: &str) -> bool { - let canonical_name = CanonicalDimensionName::from_raw(name); - self.dimensions.contains_key(&canonical_name) + self.dimensions + .contains_key(crate::common::canonicalize(name).as_ref()) } pub(crate) fn lookup(&self, element: &str) -> Option { if let Some(pos) = element.find('·') { - let dimension_name = CanonicalDimensionName::from_raw(&element[..pos]); - let element_name = CanonicalElementName::from_raw(&element[pos + '·'.len_utf8()..]); - if let Some(Dimension::Named(_, dimension)) = self.dimensions.get(&dimension_name) - && let Some(off) = dimension.indexed_elements.get(&element_name) + let dimension_name = crate::common::canonicalize(&element[..pos]); + let element_name = crate::common::canonicalize(&element[pos + '·'.len_utf8()..]); + if let Some(Dimension::Named(_, dimension)) = + self.dimensions.get(dimension_name.as_ref()) + && let Some(off) = dimension.indexed_elements.get(element_name.as_ref()) { return Some(*off as u32); } diff --git a/src/simlin-engine/src/ltm_augment_tests.rs b/src/simlin-engine/src/ltm_augment_tests.rs index 737eb0551..34bc349d8 100644 --- a/src/simlin-engine/src/ltm_augment_tests.rs +++ b/src/simlin-engine/src/ltm_augment_tests.rs @@ -3297,7 +3297,7 @@ fn arrayed_var_from_text( let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit_vars = Vec::new(); let stage0 = crate::variable::parse_var::( - dims, + &crate::dimensions::DimensionsContext::from(dims), &dm_var, &mut implicit_vars, &units_ctx, @@ -3355,7 +3355,7 @@ fn scalar_aux_from_text(ident: &str, eqn_text: &str) -> Variable { let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit_vars = Vec::new(); let stage0 = crate::variable::parse_var::( - &[], + &crate::dimensions::DimensionsContext::default(), &dm_var, &mut implicit_vars, &units_ctx, @@ -3548,7 +3548,7 @@ fn lower_dm_var( let units_ctx = crate::units::Context::new(&[], &Default::default()).0; let mut implicit_vars = Vec::new(); let stage0 = crate::variable::parse_var::( - dims, + &crate::dimensions::DimensionsContext::from(dims), &dm_var, &mut implicit_vars, &units_ctx, @@ -3958,7 +3958,7 @@ fn test_scalar_and_a2a_link_scores_keep_their_shapes() { compat: crate::datamodel::Compat::default(), }); let stage0 = crate::variable::parse_var::( - &dims, + &crate::dimensions::DimensionsContext::from(dims.as_slice()), &a2a_dm, &mut implicit, &units_ctx, diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index 133eb6b1c..5a1cbc5fd 100644 --- a/src/simlin-engine/src/model.rs +++ b/src/simlin-engine/src/model.rs @@ -578,12 +578,16 @@ impl ModelStage0 { // like-named macro resolves to the intrinsic, not the macro. let enclosing_model: Option<&str> = x_model.macro_spec.as_ref().map(|_| x_model.name.as_str()); + // One context for the whole model: `parse_var*` needs the canonical + // form, and building it per variable is what the salsa path stopped + // doing (it reads the cached `project_dimensions_context` instead). + let dimensions_ctx = DimensionsContext::from(dimensions); let mut variable_list: Vec = x_model .variables .iter() .map(|v| { parse_var_with_module_context( - dimensions, + &dimensions_ctx, v, &mut implicit_vars, units_ctx, @@ -601,7 +605,7 @@ impl ModelStage0 { let mut dummy_implicit_vars: Vec = Vec::new(); variable_list.extend(implicit_vars.into_iter().map(|x_var| { parse_var( - dimensions, + &dimensions_ctx, &x_var, &mut dummy_implicit_vars, units_ctx, @@ -1070,7 +1074,8 @@ fn test_module_parse() { let hares_var = &main_model.variables[2]; assert_eq!("hares", hares_var.get_ident()); - let actual = parse_var(&[], hares_var, &mut implicit_vars, &units_ctx, |mi| { + let dims_ctx = DimensionsContext::default(); + let actual = parse_var(&dims_ctx, hares_var, &mut implicit_vars, &units_ctx, |mi| { resolve_module_input(&models, "main", hares_var.get_ident(), &mi.src, &mi.dst) }); assert!(actual.equation_errors().is_none()); diff --git a/src/simlin-engine/src/variable.rs b/src/simlin-engine/src/variable.rs index 8d47d0558..7c2f16611 100644 --- a/src/simlin-engine/src/variable.rs +++ b/src/simlin-engine/src/variable.rs @@ -366,13 +366,13 @@ pub(crate) fn reorder_arrayed_element_tables( /// [`reorder_arrayed_element_tables`]), NOT by `elems` Vec position. For scalar /// variables or arrayed without per-element gfs, uses the variable-level gf. /// -/// `dimensions` are the project/model dimension definitions; the arrayed -/// equation's dimension names are resolved against them to drive the +/// `dimensions` is the project/model dimension context; the arrayed +/// equation's dimension names are resolved against it to drive the /// element-name -> dimension-index reorder. fn build_tables( gf: &Option, equation: &datamodel::Equation, - dimensions: &[datamodel::Dimension], + dimensions: &DimensionsContext, ) -> (Vec, Vec) { let mut errors = Vec::new(); @@ -743,14 +743,14 @@ mod is_lookup_only_tests { } pub(crate) fn get_dimensions( - dimensions: &[datamodel::Dimension], + dimensions: &DimensionsContext, names: &[DimensionName], ) -> Result, EquationError> { names .iter() .map(|name| -> Result { // Match by canonical name, not raw string equality: a dimension's - // identity is its canonical name (the dims map is keyed by it, so + // identity is its canonical name (the context is keyed by it, so // two distinct dimensions can never canonicalize to the same // string). A synthesized `Equation::ApplyToAll` whose dimension // names came from `print_eqn` carries CANONICAL names (`hfc_type`), @@ -760,20 +760,23 @@ pub(crate) fn get_dimensions( // PREVIOUS/INIT helper regression on C-LEARN's capitalized // dimensions). Importer-produced equations already match exactly, // so canonical matching is a strict superset. - let canonical_name = canonicalize(name); - for dim in dimensions { - if canonicalize(dim.name()) == canonical_name { - return Ok(Dimension::from(dim)); - } + // + // Taking the already-built `DimensionsContext` rather than the raw + // `&[datamodel::Dimension]` turns this from a linear scan that + // re-canonicalized every declared dimension name per lookup (and + // then rebuilt the matched `Dimension` from scratch, re-interning + // its every element name) into one canonicalize plus a hash probe. + match dimensions.get_by_raw_name(name) { + Some(dim) => Ok(dim.clone()), + None => eqn_err!(BadDimensionName, 0, 0), } - eqn_err!(BadDimensionName, 0, 0) }) .collect() } fn parse_equation( eqn: &datamodel::Equation, - dimensions: &[datamodel::Dimension], + dimensions: &DimensionsContext, is_initial: bool, active_initial: Option<&str>, ) -> (Option>, Vec) { @@ -856,7 +859,7 @@ fn parse_equation( } pub fn parse_var( - dimensions: &[datamodel::Dimension], + dimensions: &DimensionsContext, v: &datamodel::Variable, implicit_vars: &mut Vec, units_ctx: &units::Context, @@ -910,7 +913,7 @@ where /// to the intrinsic, not recurse into the like-named macro). #[allow(clippy::too_many_arguments)] pub fn parse_var_with_module_context( - dimensions: &[datamodel::Dimension], + dimensions: &DimensionsContext, v: &datamodel::Variable, implicit_vars: &mut Vec, units_ctx: &units::Context, @@ -928,9 +931,6 @@ where // rebinding here -- unifying a borrowed `Some(&'a _)` with the // `&'static` empty default before the parse closure captures it would // force the closure (and hence `'a`) to `'static`. - // Create DimensionsContext for dimension mapping lookups in builtin expansion - let dimensions_ctx = DimensionsContext::from(dimensions); - let mut parse_and_lower_eqn = |ident: &str, eqn: &datamodel::Equation, is_initial: bool, @@ -946,7 +946,7 @@ where match instantiate_implicit_modules( ident, ast, - Some(&dimensions_ctx), + Some(dimensions), module_idents, model_var_names, registry, @@ -1426,7 +1426,7 @@ pub(crate) fn scalar_ast(eqn: &str) -> Ast { let (ast, err) = parse_equation( &datamodel::Equation::Scalar(eqn.to_owned()), - &[], + &DimensionsContext::default(), false, None, ); @@ -2091,7 +2091,12 @@ fn test_parse_equation_arrayed_preserves_default_expression() { true, ); - let (ast, errors) = parse_equation(&equation, &dimensions, false, None); + let (ast, errors) = parse_equation( + &equation, + &DimensionsContext::from(&dimensions), + false, + None, + ); assert!(errors.is_empty(), "arrayed parse should not emit errors"); let Some(Ast::Arrayed(_, _, default_expr, apply_default_to_missing)) = ast else { @@ -2122,7 +2127,12 @@ fn test_parse_equation_arrayed_applies_default_when_element_matches_default() { true, ); - let (ast, errors) = parse_equation(&equation, &dimensions, false, None); + let (ast, errors) = parse_equation( + &equation, + &DimensionsContext::from(&dimensions), + false, + None, + ); assert!(errors.is_empty(), "arrayed parse should not emit errors"); let Some(Ast::Arrayed(_, _, default_expr, apply_default_to_missing)) = ast else { @@ -2199,7 +2209,8 @@ fn test_tables() { let mut implicit_vars: Vec = Vec::new(); let unit_ctx = crate::units::Context::new(&[], &Default::default()).0; - let output = parse_var(&[], &input, &mut implicit_vars, &unit_ctx, |mi| { + let dims_ctx = DimensionsContext::default(); + let output = parse_var(&dims_ctx, &input, &mut implicit_vars, &unit_ctx, |mi| { Ok(Some(mi.clone())) }); From 90a66df612da4e215fc2a0a5f517df8161a9bc94 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 18:59:59 -0700 Subject: [PATCH 02/10] engine: stop allocating four Strings per identifier in canonicalize `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%). --- src/simlin-engine/src/common.rs | 152 ++++++++++++++++++++++++++++++-- 1 file changed, 146 insertions(+), 6 deletions(-) diff --git a/src/simlin-engine/src/common.rs b/src/simlin-engine/src/common.rs index bf1cfd480..b5b3d9db9 100644 --- a/src/simlin-engine/src/common.rs +++ b/src/simlin-engine/src/common.rs @@ -932,7 +932,15 @@ pub fn canonicalize(name: &str) -> Cow<'_, str> { return Cow::Borrowed(trimmed); } - // Slow path: full canonicalization with allocation. + // Slow path: four rewrites per identifier part -- period mapping, + // doubled-backslash unescaping, whitespace collapse, lowercasing. Each is + // guarded by a cheap "is there anything to do" test and skipped by + // borrowing when there is not, and the last two are fused for an ASCII part + // so they write straight into the output. Spelling them as four + // unconditional `String`-returning steps cost four allocations and a + // two-way substring searcher per part even for a name whose only defect was + // its capitalization -- and this is the single hottest function in a large + // model's compile, reached once per identifier occurrence. let mut canonicalized_name = String::with_capacity(trimmed.len()); for part in IdentifierPartIterator::new(trimmed) { @@ -953,24 +961,85 @@ pub fn canonicalize(name: &str) -> Cow<'_, str> { } else { Cow::Borrowed(inner) } - } else { + } else if part.contains('.') { // Replace periods with middle dots (·) for module hierarchy separators. // This allows us to distinguish between: // - Module separators: model.variable -> model·variable // - Literal periods in quoted names: "a.b" -> ab Cow::Owned(part.replace('.', "·")) + } else { + Cow::Borrowed(part) }; - let part = part.replace("\\\\", "\\"); - let part = replace_whitespace_with_underscore(&part); - let part = part.to_lowercase(); + // Unescape doubled backslashes. Guarded on a single-character search + // (which is memchr) rather than on the two-character pattern, whose + // searcher setup costs more than the scan: no backslash at all implies + // no doubled one, and a part holding a lone backslash is rare enough + // that letting it fall through to a no-op `replace` is free. + let part = if part.contains('\\') { + Cow::Owned(part.replace("\\\\", "\\")) + } else { + part + }; - canonicalized_name.push_str(&part); + push_whitespace_folded_lowercase(&mut canonicalized_name, &part); } Cow::Owned(canonicalized_name) } +/// Append `part` to `out` with the last two canonicalization steps applied: +/// [`replace_whitespace_with_underscore`], then `to_lowercase`. +/// +/// The ASCII arm fuses the two into one pass that writes directly into `out`. +/// That is sound for exactly two reasons, both of which fail outside ASCII: +/// ASCII lowercasing is per-byte and context-free (`str::to_lowercase` is +/// context-sensitive in general -- Greek capital sigma lowercases differently +/// at the end of a word), and the only non-ASCII character the whitespace pass +/// recognizes, U+00A0, cannot occur. The non-ASCII arm therefore keeps the two +/// steps separate and unchanged, and skips `to_lowercase` when no character +/// would change -- the same predicate `is_canonical` uses. +fn push_whitespace_folded_lowercase(out: &mut String, part: &str) { + if part.is_ascii() { + let bytes = part.as_bytes(); + let mut in_whitespace = false; + let mut i = 0; + while i < bytes.len() { + let b = bytes[i]; + // A literal `\n` / `\r` escape (two characters) counts as + // whitespace; any other backslash passes through. + if b == b'\\' && matches!(bytes.get(i + 1), Some(b'n' | b'r')) { + i += 2; + if !in_whitespace { + out.push('_'); + in_whitespace = true; + } + } else if matches!(b, b'\n' | b'\r' | b'\t' | b' ') { + i += 1; + if !in_whitespace { + out.push('_'); + in_whitespace = true; + } + } else { + i += 1; + in_whitespace = false; + out.push(char::from(b.to_ascii_lowercase())); + } + } + return; + } + + let replaced = replace_whitespace_with_underscore(part); + if replaced + .chars() + .any(|c| c.to_lowercase().ne(std::iter::once(c))) + { + out.push_str(&replaced.to_lowercase()); + } else { + out.push_str(&replaced); + } +} + /// Group a variable-ident list by canonical form and return the colliding /// groups: for each canonical ident declared more than once, the canonical /// form plus every as-written spelling, in declaration order (GH #885). @@ -1370,6 +1439,23 @@ mod canonicalize_invariant_tests { result } + /// The alphabet that actually reaches every branch of the slow path. + /// + /// `\PC{0,100}` alone does not: the branches are selected by quotes, + /// periods, backslashes, the two literal escapes, whitespace runs, and + /// characters whose lowercasing is context-sensitive or non-ASCII, and a + /// generator over all non-control characters produces those roughly never. + /// Two of these deserve naming. `Σ` is the one character for which + /// `str::to_lowercase` is context-sensitive (it lowercases to `ς` at the + /// end of a word and `σ` elsewhere), which is why the fused rewrite is + /// restricted to ASCII. `\u{00A0}` is the one non-ASCII character + /// `replace_whitespace_with_underscore` treats as whitespace, and `str:: + /// trim` also strips it, so it exercises both the trim and the fold. + const SLOW_PATH_ALPHABET: &[&str] = &[ + "a", "Z", "_", "0", ".", " ", "\t", "\n", "\r", "\\", "\"", "·", "\u{2024}", "\u{00A0}", + "Σ", "σ", "ς", "É", "Dž", "İ", + ]; + proptest! { #[test] fn fast_path_agrees_with_slow_path(s in "\\PC{0,100}") { @@ -1384,6 +1470,60 @@ mod canonicalize_invariant_tests { "Borrowed result should equal trimmed input for {:?}", s); } } + + /// The same differential check, driven by [`SLOW_PATH_ALPHABET`] so the + /// slow path's branches are reached rather than hoped for. Named + /// separately from the broad-alphabet property because the two catch + /// different things: that one covers the input space, this one covers + /// the code. + #[test] + fn canonicalize_agrees_with_the_unfused_reference( + pieces in proptest::collection::vec( + proptest::sample::select(SLOW_PATH_ALPHABET), 0..24) + ) { + let s: String = pieces.concat(); + prop_assert_eq!(&*canonicalize(&s), &*canonicalize_slow_path(&s), + "canonicalize disagrees with the unfused reference for {:?}", s); + } + } + + /// Hand-written cases for the interactions the fused ASCII rewrite has to + /// get right, each of which composes two steps whose order matters. + #[test] + fn fused_ascii_fold_matches_the_reference_on_step_interactions() { + let cases = [ + // Doubled backslash collapses BEFORE the escape scan sees it, so + // `\\n` is a backslash followed by `n`, i.e. an escape. + "A\\\\nB", + // ...whereas a single backslash before `n` is already the escape, + // and a lone trailing backslash passes through. + "A\\nB", + "A\\", + // Whitespace runs (mixed real and escaped) collapse to one `_`. + "A \t\nB", + "A\\n\\rB", + "A \\n B", + // A backslash that is not an escape resets the run, so the + // whitespace on either side of it yields two underscores. + "A \\ B", + // Quoted parts keep their interior spacing rules but lose the + // quotes, and a period inside them is the literal-period sentinel. + "\"A B\".C", + "\"a.b\"", + // Mixed quoted/unquoted parts in one identifier. + "Mod.\"Var Name\".Sub", + // Leading/trailing whitespace is trimmed before anything else. + " Net Flow ", + // A part that is purely whitespace. + "A. .B", + ]; + for case in cases { + assert_eq!( + &*canonicalize(case), + &*canonicalize_slow_path(case), + "canonicalize disagrees with the unfused reference for {case:?}" + ); + } } #[test] From d6ae3df0eb50bbf335572609ee4029719733f841 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 20:07:18 -0700 Subject: [PATCH 03/10] engine: decide "already canonical" in one scan, and stop cloning dimensions 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>`, 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` 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`. The three canonical newtypes are tags over one interned storage, so `From<&Ident>` (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. --- src/simlin-engine/src/ast/expr2.rs | 10 +- src/simlin-engine/src/ast/expr3.rs | 25 ++-- src/simlin-engine/src/ast/mod.rs | 7 +- src/simlin-engine/src/common.rs | 153 +++++++++++++++++++- src/simlin-engine/src/compiler/context.rs | 4 +- src/simlin-engine/src/compiler/subscript.rs | 30 ++-- 6 files changed, 184 insertions(+), 45 deletions(-) diff --git a/src/simlin-engine/src/ast/expr2.rs b/src/simlin-engine/src/ast/expr2.rs index efd8a389c..9788484aa 100644 --- a/src/simlin-engine/src/ast/expr2.rs +++ b/src/simlin-engine/src/ast/expr2.rs @@ -86,7 +86,7 @@ impl IndexExpr2 { let expr = match expr { IndexExpr1::Wildcard(loc) => IndexExpr2::Wildcard(loc), IndexExpr1::StarRange(ident, loc) => { - IndexExpr2::StarRange(CanonicalDimensionName::from_raw(ident.as_str()), loc) + IndexExpr2::StarRange(CanonicalDimensionName::from(&ident), loc) } IndexExpr1::Range(l, r, loc) => { IndexExpr2::Range(Expr2::from(l, ctx)?, Expr2::from(r, ctx)?, loc) @@ -171,7 +171,7 @@ pub enum Expr2 { /// Provides access to variable dimension information and temp ID allocation pub trait Expr2Context { /// Get the dimensions of a variable, or None if it's a scalar - fn get_dimensions(&self, ident: &str) -> Option>; + fn get_dimensions(&self, ident: &str) -> Option<&[Dimension]>; /// Allocate a new temp ID for the current equation fn allocate_temp_id(&mut self) -> u32; @@ -816,7 +816,7 @@ impl Expr2 { && ctx.is_dimension_name(id.as_str()) { // Convert SIZE(DimName) to a constant - let dim_name = CanonicalDimensionName::from_raw(id.as_str()); + let dim_name = CanonicalDimensionName::from(id); if let Some(len) = ctx.get_dimension_len(&dim_name) { // Return a constant expression with the dimension size return Ok(Expr2::Const( @@ -1223,8 +1223,8 @@ mod tests { } impl Expr2Context for TestContext { - fn get_dimensions(&self, ident: &str) -> Option> { - self.dimensions.get(ident).cloned() + fn get_dimensions(&self, ident: &str) -> Option<&[Dimension]> { + self.dimensions.get(ident).map(|dims| dims.as_slice()) } fn allocate_temp_id(&mut self) -> u32 { diff --git a/src/simlin-engine/src/ast/expr3.rs b/src/simlin-engine/src/ast/expr3.rs index 0d37eca62..31e0990ba 100644 --- a/src/simlin-engine/src/ast/expr3.rs +++ b/src/simlin-engine/src/ast/expr3.rs @@ -286,7 +286,7 @@ impl Expr3 { /// - Detecting dimension name references in subscripts pub trait Expr3LowerContext { /// Get the dimensions of a variable, or None if it's a scalar. - fn get_dimensions(&self, ident: &str) -> Option>; + fn get_dimensions(&self, ident: &str) -> Option<&[Dimension]>; /// Check if an identifier is a dimension name (not a variable). /// Used to detect A2A dimension references in subscripts. @@ -325,7 +325,7 @@ impl IndexExpr3 { // For named dimensions like Cities{Boston,NYC,LA}, this becomes StarRange("cities"). // The downstream compiler/evaluator must recognize that StarRange(parent_dim) // means "iterate over all elements" (equivalent to IndexOp::Wildcard). - let dim_name = CanonicalDimensionName::from_raw(dim.name()); + let dim_name = dim.canonical_name().clone(); Ok(IndexExpr3::StarRange(dim_name, *loc)) } IndexExpr2::StarRange(subdim_name, loc) => { @@ -362,13 +362,13 @@ impl IndexExpr3 { && ctx.is_dimension_name(ident.as_str()) { // Check if this is an element of the parent dimension first - let element_name = CanonicalElementName::from_raw(ident.as_str()); + let element_name = CanonicalElementName::from(ident); let is_element_of_parent = dim .map(|d| d.get_offset(&element_name).is_some()) .unwrap_or(false); if !is_element_of_parent { - let canonical = CanonicalDimensionName::from_raw(ident.as_str()); + let canonical = CanonicalDimensionName::from(ident); return Ok(IndexExpr3::Dimension(canonical, *loc)); } } @@ -401,10 +401,7 @@ impl Expr3 { // which are immediately resolved to star ranges let subscripts: Vec = dims .iter() - .map(|dim| { - let dim_name = CanonicalDimensionName::from_raw(dim.name()); - IndexExpr3::StarRange(dim_name, *loc) - }) + .map(|dim| IndexExpr3::StarRange(dim.canonical_name().clone(), *loc)) .collect(); return Ok(Expr3::Subscript( @@ -441,7 +438,7 @@ impl Expr3 { // Validate subscript count matches dimension count. // This catches cases like arr[*, *, *] on a 2D array before // we hit misleading errors in individual subscript lowering. - if let Some(ref d) = dims + if let Some(d) = dims && args.len() > d.len() { // Find the first out-of-bounds subscript for error location @@ -450,12 +447,11 @@ impl Expr3 { return eqn_err!(MismatchedDimensions, extra_loc.start, extra_loc.end); } - let dims_ref = dims.as_deref(); let lowered_args: EquationResult> = args .iter() .enumerate() .map(|(i, arg)| { - let dim = dims_ref.and_then(|d| d.get(i)); + let dim = dims.and_then(|d| d.get(i)); IndexExpr3::from_index_expr2(arg, dim, ctx) }) .collect(); @@ -710,8 +706,7 @@ impl<'a> Pass1Context<'a> { { // Find the active dimension that matches this dimension name for (dim, sub) in active_dims.iter().zip(active_subs.iter()) { - let active_dim_name = CanonicalDimensionName::from_raw(dim.name()); - if active_dim_name.as_str() == dim_name.as_str() { + if dim.name() == dim_name.as_str() { // Found a match - resolve to the concrete index if let Some(index) = Self::subscript_to_index(dim, sub) { let const_expr = @@ -1576,8 +1571,8 @@ mod tests { } impl Expr3LowerContext for TestLowerContext { - fn get_dimensions(&self, ident: &str) -> Option> { - self.dimensions.get(ident).cloned() + fn get_dimensions(&self, ident: &str) -> Option<&[Dimension]> { + self.dimensions.get(ident).map(|dims| dims.as_slice()) } fn is_dimension_name(&self, ident: &str) -> bool { diff --git a/src/simlin-engine/src/ast/mod.rs b/src/simlin-engine/src/ast/mod.rs index 279becf49..8b5373cda 100644 --- a/src/simlin-engine/src/ast/mod.rs +++ b/src/simlin-engine/src/ast/mod.rs @@ -178,11 +178,11 @@ impl<'a> ArrayContext<'a> { } impl<'a> Expr2Context for ArrayContext<'a> { - fn get_dimensions(&self, ident: &str) -> Option> { + fn get_dimensions(&self, ident: &str) -> Option<&[crate::dimensions::Dimension]> { // During AST lowering, we may encounter variables that don't exist yet // (e.g., in tests or when processing incomplete models) let var = self.get_variable(self.model_name, ident)?; - var.get_dimensions().map(|dims| dims.to_vec()) + var.get_dimensions() } fn allocate_temp_id(&mut self) -> u32 { @@ -205,10 +205,9 @@ impl<'a> Expr2Context for ArrayContext<'a> { } fn is_indexed_dimension(&self, name: &str) -> bool { - let canonical_name = crate::common::CanonicalDimensionName::from_raw(name); self.scope .dimensions - .get(&canonical_name) + .get_by_raw_name(name) .map(|dim| matches!(dim, crate::dimensions::Dimension::Indexed(_, _))) .unwrap_or(false) } diff --git a/src/simlin-engine/src/common.rs b/src/simlin-engine/src/common.rs index b5b3d9db9..7b114f667 100644 --- a/src/simlin-engine/src/common.rs +++ b/src/simlin-engine/src/common.rs @@ -907,7 +907,7 @@ fn is_canonical(name: &str) -> bool { // This covers both uppercase (Lu) and titlecase (Lt) Unicode // categories -- titlecase letters like Dž are NOT uppercase but // to_lowercase() still maps them to a different character (dž). - c if c.to_lowercase().ne(std::iter::once(c)) => return false, + c if changes_when_lowercased(c) => return false, _ => {} } } @@ -923,7 +923,97 @@ fn is_canonical(name: &str) -> bool { /// Note: the borrowed slice may be a sub-slice of the input when there is /// leading/trailing whitespace but the trimmed content is already canonical. /// The returned `Cow` borrows from the input `&str` in all borrowed cases. +/// Whether `to_lowercase` would change `c`, i.e. Unicode's +/// `Changes_When_Lowercased`. +/// +/// Spelled as two `next()` calls rather than `c.to_lowercase().ne(once(c))`: +/// the iterator-comparison form goes through the generic `Iterator::eq_by`, +/// which does not collapse, and this predicate is on the identifier fast path. +#[inline] +fn changes_when_lowercased(c: char) -> bool { + let mut lower = c.to_lowercase(); + lower.next() != Some(c) || lower.next().is_some() +} + +/// Per-byte "this byte alone cannot make a name non-canonical" table, the +/// fast path's whole decision. +/// +/// `false` for every non-ASCII byte (the Unicode rules need `char`s), for the +/// characters [`is_canonical`] rejects outright, for ASCII uppercase, and for +/// the backslash -- which is excluded not because it is always wrong but +/// because deciding needs a lookahead, and a backslash in an identifier is +/// rare enough that paying the slower check for it is free. +/// +/// A byte table rather than a 128-bit mask, which was measured and is worse on +/// both counts: the mask needs a range test and a variable shift per byte, +/// where the table is one load the scan can keep in flight. +static CANONICAL_BYTE: [bool; 256] = { + let mut table = [false; 256]; + let mut b = 0usize; + while b < 128 { + table[b] = !matches!( + b as u8, + b'"' | b'.' | b' ' | b'\n' | b'\r' | b'\t' | b'\\' | b'A'..=b'Z' + ); + b += 1; + } + table +}; + +/// Whether `name` is already canonical AND `str::trim` would not change it -- +/// [`is_canonical`] composed with "needs no trimming", decided in one pass. +/// +/// The point of handling non-ASCII here rather than bailing to +/// [`is_canonical`] is that a non-ASCII character costs one decode instead of +/// demoting the WHOLE string to that function's Unicode arm: LTM's synthetic +/// variable names are mostly ASCII with a handful of U+205A / U+2192 +/// separators, and re-canonicalizing one used to case-check every character. +/// +/// Conservative in one direction and never the other: a character this rejects +/// may still be canonical (any non-ASCII whitespace, say), in which case the +/// caller's `trim` + [`is_canonical`] pair answers exactly as it always did -- +/// only slower. Nothing it ACCEPTS may be non-canonical. +fn is_canonical_needing_no_trim(name: &str) -> bool { + let bytes = name.as_bytes(); + let mut i = 0; + while i < bytes.len() { + let b = bytes[i]; + if CANONICAL_BYTE[b as usize] { + i += 1; + continue; + } + if b < 0x80 { + // An ASCII byte the table rejects: uppercase, a character + // `is_canonical` rejects outright, or a backslash (whose verdict + // needs a lookahead this scan deliberately does not take). + return false; + } + // Non-ASCII. Only two of `is_canonical`'s Unicode rules can apply -- + // every character it rejects by value is ASCII apart from U+00A0, and + // `char::is_whitespace` covers that one. Rejecting EVERY Unicode + // whitespace (not just U+00A0) is what makes "needs no trimming" sound: + // `str::trim` strips the whole White_Space property. + let Some(c) = name[i..].chars().next() else { + return false; + }; + if c.is_whitespace() || changes_when_lowercased(c) { + return false; + } + i += c.len_utf8(); + } + true +} + pub fn canonicalize(name: &str) -> Cow<'_, str> { + // Fastest path: one scan proving the name is already canonical AND needs + // no trimming. This is the overwhelmingly common case -- every already-canonical + // identifier the compiler re-canonicalizes on its way through a map lookup + // or an AST lowering lands here -- and it replaces a Unicode `trim`, an + // `is_ascii` scan, and `is_canonical`'s own scan with a single pass. + if is_canonical_needing_no_trim(name) { + return Cow::Borrowed(name); + } + // Fast path: if the name is already trimmed and canonical, avoid allocation. let trimmed = name.trim(); if is_canonical(trimmed) { @@ -1487,6 +1577,36 @@ mod canonicalize_invariant_tests { } } + /// A canonical name is returned BORROWED, non-ASCII separators included. + /// + /// The engine's own generated identifiers -- LTM link/loop scores, the + /// module separator, the literal-period sentinel -- are mostly ASCII with a + /// few non-ASCII separators, and they are re-canonicalized constantly on + /// their way through map lookups and AST lowerings. Answering "already + /// canonical" for them without allocating is the point of the fused scan, + /// and it is invisible in a correctness test: demoting them to the slow + /// path returns an EQUAL `Cow::Owned`, so only the discriminant shows it. + #[test] + fn engine_generated_names_canonicalize_without_allocating() { + let names = [ + "population", + "net_flow_2", + "model\u{00b7}variable", + "goal_1\u{2024}5_for_temperature", + "$\u{205a}ltm\u{205a}link_score\u{205a}food\u{2192}population", + "$\u{205a}ltm\u{205a}loop_score\u{205a}r1", + "$\u{205a}ltm\u{205a}agg\u{205a}3", + "stdlib\u{205a}smth1", + ]; + for name in names { + assert!( + matches!(canonicalize(name), Cow::Borrowed(_)), + "{name:?} is canonical and must not be re-built" + ); + assert_eq!(&*canonicalize(name), &*canonicalize_slow_path(name)); + } + } + /// Hand-written cases for the interactions the fused ASCII rewrite has to /// get right, each of which composes two steps whose order matters. #[test] @@ -2807,6 +2927,37 @@ impl std::borrow::Borrow for Ident { } } +// Re-tagging one canonical newtype as another, for the cases where the source +// value's TYPE already proves the string is canonical: `Ident` and +// the dimension/element names are three tags over one interned storage, so +// there is nothing to compute and nothing to allocate -- the payload is shared +// and the conversion is a refcount bump. +// +// Spelling this `CanonicalDimensionName::from_raw(ident.as_str())` instead is +// what these exist to stop: that re-runs the whole canonicalize pass over a +// string that is canonical by construction, then takes a shard of the global +// interner to look up a payload the caller is already holding. On the array +// lowering path (`Expr3::from_expr2` expanding a bare array reference into one +// star range per declared dimension) that was measured at 6% of a C-LEARN +// compile's total instructions. +impl From<&Ident> for CanonicalDimensionName { + fn from(ident: &Ident) -> Self { + CanonicalDimensionName(ident.inner.clone()) + } +} + +impl From<&Ident> for CanonicalElementName { + fn from(ident: &Ident) -> Self { + CanonicalElementName(ident.inner.clone()) + } +} + +impl From<&CanonicalDimensionName> for CanonicalElementName { + fn from(name: &CanonicalDimensionName) -> Self { + CanonicalElementName(name.0.clone()) + } +} + // The same, for the two other newtypes over `CanonicalStorage`. Probing a map // with a `&str` skips the global interner entirely: constructing a // `CanonicalDimensionName`/`CanonicalElementName` just to ask whether a key is diff --git a/src/simlin-engine/src/compiler/context.rs b/src/simlin-engine/src/compiler/context.rs index 8e31bf843..32d428730 100644 --- a/src/simlin-engine/src/compiler/context.rs +++ b/src/simlin-engine/src/compiler/context.rs @@ -1077,7 +1077,7 @@ impl Expr3LowerContext for Context<'_> { /// sub-model's own metadata and gets the dimensions this method did not. /// Bare references, whole-array copies and transposes all produced correct /// results before the fix. - fn get_dimensions(&self, ident: &str) -> Option> { + fn get_dimensions(&self, ident: &str) -> Option<&[Dimension]> { let canonical = canonicalize(ident); let vars = self.metadata.get(self.model_name)?; // A plain name is its own key, so the direct lookup answers it without @@ -1092,7 +1092,7 @@ impl Expr3LowerContext for Context<'_> { .get_metadata(&Ident::from_unchecked(canonical.into_owned())) .ok()?, }; - var_metadata.var.get_dimensions().map(|dims| dims.to_vec()) + var_metadata.var.get_dimensions() } fn is_dimension_name(&self, ident: &str) -> bool { diff --git a/src/simlin-engine/src/compiler/subscript.rs b/src/simlin-engine/src/compiler/subscript.rs index 1024bcece..4c220cbb9 100644 --- a/src/simlin-engine/src/compiler/subscript.rs +++ b/src/simlin-engine/src/compiler/subscript.rs @@ -83,8 +83,6 @@ pub(crate) fn normalize_subscripts3( args: &[IndexExpr3], config: &Subscript3Config, ) -> Option> { - use crate::common::CanonicalDimensionName; - let mut operations = Vec::with_capacity(args.len()); for (i, arg) in args.iter().enumerate() { @@ -93,7 +91,7 @@ pub(crate) fn normalize_subscripts3( } let parent_dim = &config.dims[i]; - let parent_name = CanonicalDimensionName::from_raw(parent_dim.name()); + let parent_name = parent_dim.canonical_name(); let op = match arg { IndexExpr3::StarRange(subdim_name, _) => { @@ -125,7 +123,7 @@ pub(crate) fn normalize_subscripts3( // Named subdimension - look up relationship let relation = config .dimensions_ctx - .get_subdimension_relation(subdim_name, &parent_name)?; + .get_subdimension_relation(subdim_name, parent_name)?; if relation.is_contiguous() { let start = relation.start_offset(); @@ -235,14 +233,14 @@ pub(crate) fn normalize_subscripts3( let is_dim_name = config .all_dimensions .iter() - .any(|d| &*canonicalize(d.name()) == ident.as_str()); + .any(|d| d.name() == ident.as_str()); if is_dim_name { // It's a dimension name - find matching active dimension let active_dims = config.active_dimension?; let active_idx = active_dims .iter() - .position(|ad| &*canonicalize(ad.name()) == ident.as_str())?; + .position(|ad| ad.name() == ident.as_str())?; IndexOp::ActiveDimRef(active_idx) } else { // Not a known element or dimension - need dynamic handling @@ -269,24 +267,20 @@ pub(crate) fn normalize_subscripts3( // A2A dimension reference - need to find matching active dimension. // Check by name first, then by dimension mapping. let active_dims = config.active_dimension?; + // Both sides are already canonical -- `name` is a + // `CanonicalDimensionName` and a `dimensions::Dimension`'s name + // is one by construction -- so neither needs re-canonicalizing + // or re-interning to be compared or looked up. let active_idx = active_dims .iter() - .position(|ad| &*canonicalize(ad.name()) == name.as_str()) + .position(|ad| ad.name() == name.as_str()) .or_else(|| { // Try dimension mapping in both directions (handles // multi-target dimensions correctly). - let dim_name = - crate::common::CanonicalDimensionName::from_raw(name.as_str()); active_dims.iter().position(|ad| { - let active_canonical = crate::common::CanonicalDimensionName::from_raw( - &canonicalize(ad.name()), - ); - config - .dimensions_ctx - .has_mapping_to(&dim_name, &active_canonical) - || config - .dimensions_ctx - .has_mapping_to(&active_canonical, &dim_name) + let active_canonical = ad.canonical_name(); + config.dimensions_ctx.has_mapping_to(name, active_canonical) + || config.dimensions_ctx.has_mapping_to(active_canonical, name) }) })?; IndexOp::ActiveDimRef(active_idx) From 905d51645025cebe38f929b3104409669d0f212c Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 20:23:35 -0700 Subject: [PATCH 04/10] engine: lower the parsed AST by reference instead of deep-copying it 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::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`. The one thing that moves the other way is `Ast::ApplyToAll`/`Ast::Arrayed`'s `Vec`, 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. --- src/simlin-engine/src/ast/expr1.rs | 52 ++++++++++++++++++------------ src/simlin-engine/src/ast/mod.rs | 12 +++---- src/simlin-engine/src/model.rs | 34 +++++++++---------- src/simlin-engine/src/variable.rs | 2 +- 4 files changed, 53 insertions(+), 47 deletions(-) diff --git a/src/simlin-engine/src/ast/expr1.rs b/src/simlin-engine/src/ast/expr1.rs index 7ceefbcbd..92829f82f 100644 --- a/src/simlin-engine/src/ast/expr1.rs +++ b/src/simlin-engine/src/ast/expr1.rs @@ -24,14 +24,14 @@ pub enum IndexExpr1 { } impl IndexExpr1 { - pub(crate) fn from(expr: IndexExpr0) -> EquationResult { + pub(crate) fn from(expr: &IndexExpr0) -> EquationResult { let expr = match expr { - IndexExpr0::Wildcard(loc) => IndexExpr1::Wildcard(loc), - IndexExpr0::StarRange(ident, loc) => IndexExpr1::StarRange(ident.canonicalize(), loc), + IndexExpr0::Wildcard(loc) => IndexExpr1::Wildcard(*loc), + IndexExpr0::StarRange(ident, loc) => IndexExpr1::StarRange(ident.canonicalize(), *loc), IndexExpr0::Range(l, r, loc) => { - IndexExpr1::Range(Expr1::from(l)?, Expr1::from(r)?, loc) + IndexExpr1::Range(Expr1::from(l)?, Expr1::from(r)?, *loc) } - IndexExpr0::DimPosition(n, loc) => IndexExpr1::DimPosition(n, loc), + IndexExpr0::DimPosition(n, loc) => IndexExpr1::DimPosition(*n, *loc), IndexExpr0::Expr(e) => IndexExpr1::Expr(Expr1::from(e)?), }; @@ -72,13 +72,23 @@ pub enum Expr1 { } impl Expr1 { - pub(crate) fn from(expr: Expr0) -> EquationResult { + /// Lower a parsed `Expr0` into an `Expr1`, by REFERENCE. + /// + /// Deliberately not by value even though every field is either copied or + /// re-derived here: `Expr1` is a different tree with different identifier + /// types, so it is built from scratch either way, and taking `&Expr0` is + /// what lets `lower_ast` read a variable's parsed AST straight out of its + /// (shared, salsa-cached) home. Consuming it forced the caller to deep-copy + /// the whole `Expr0` tree -- a `Box` per node and a `String` per identifier + /// -- purely so this function could destroy it, which was the single + /// largest source of allocations in a C-LEARN compile. + pub(crate) fn from(expr: &Expr0) -> EquationResult { let expr = match expr { - Expr0::Const(s, n, loc) => Expr1::Const(s, n, loc), - Expr0::Var(id, loc) => Expr1::Var(id.canonicalize(), loc), + Expr0::Const(s, n, loc) => Expr1::Const(s.clone(), *n, *loc), + Expr0::Var(id, loc) => Expr1::Var(id.canonicalize(), *loc), Expr0::App(UntypedBuiltinFn(id, orig_args), loc) => { - let args: EquationResult> = - orig_args.into_iter().map(Expr1::from).collect(); + let loc = *loc; + let args: EquationResult> = orig_args.iter().map(Expr1::from).collect(); let mut args = args?; macro_rules! check_arity { @@ -301,21 +311,21 @@ impl Expr1 { } Expr0::Subscript(id, args, loc) => { let args: EquationResult> = - args.into_iter().map(IndexExpr1::from).collect(); - Expr1::Subscript(id.canonicalize(), args?, loc) + args.iter().map(IndexExpr1::from).collect(); + Expr1::Subscript(id.canonicalize(), args?, *loc) } - Expr0::Op1(op, l, loc) => Expr1::Op1(op, Box::new(Expr1::from(*l)?), loc), + Expr0::Op1(op, l, loc) => Expr1::Op1(*op, Box::new(Expr1::from(l)?), *loc), Expr0::Op2(op, l, r, loc) => Expr1::Op2( - op, - Box::new(Expr1::from(*l)?), - Box::new(Expr1::from(*r)?), - loc, + *op, + Box::new(Expr1::from(l)?), + Box::new(Expr1::from(r)?), + *loc, ), Expr0::If(cond, t, f, loc) => Expr1::If( - Box::new(Expr1::from(*cond)?), - Box::new(Expr1::from(*t)?), - Box::new(Expr1::from(*f)?), - loc, + Box::new(Expr1::from(cond)?), + Box::new(Expr1::from(t)?), + Box::new(Expr1::from(f)?), + *loc, ), }; Ok(expr) diff --git a/src/simlin-engine/src/ast/mod.rs b/src/simlin-engine/src/ast/mod.rs index 8b5373cda..392bf0f96 100644 --- a/src/simlin-engine/src/ast/mod.rs +++ b/src/simlin-engine/src/ast/mod.rs @@ -231,7 +231,7 @@ impl<'a> Expr2Context for ArrayContext<'a> { } } -pub(crate) fn lower_ast(scope: &ScopeStage0, ast: Ast) -> EquationResult> { +pub(crate) fn lower_ast(scope: &ScopeStage0, ast: &Ast) -> EquationResult> { match ast { Ast::Scalar(expr) => { let mut ctx = ArrayContext::new(scope, scope.model_name); @@ -245,18 +245,18 @@ pub(crate) fn lower_ast(scope: &ScopeStage0, ast: Ast) -> EquationResult< Expr1::from(expr) .map(|expr| expr.constify_dimensions(scope)) .and_then(|expr| Expr2::from(expr, &mut ctx)) - .map(|expr| Ast::ApplyToAll(dims, expr)) + .map(|expr| Ast::ApplyToAll(dims.clone(), expr)) } Ast::Arrayed(dims, elements, default_expr, apply_default_to_missing) => { let mut ctx = ArrayContext::with_array_context(scope, scope.model_name); let elements: EquationResult> = elements - .into_iter() + .iter() .map(|(id, expr)| { match Expr1::from(expr) .map(|expr| expr.constify_dimensions(scope)) .and_then(|expr| Expr2::from(expr, &mut ctx)) { - Ok(expr) => Ok((id, expr)), + Ok(expr) => Ok((id.clone(), expr)), Err(err) => Err(err), } }) @@ -271,10 +271,10 @@ pub(crate) fn lower_ast(scope: &ScopeStage0, ast: Ast) -> EquationResult< }; match elements { Ok(elements) => Ok(Ast::Arrayed( - dims, + dims.clone(), elements, default_expr, - apply_default_to_missing, + *apply_default_to_missing, )), Err(err) => Err(err), } diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index 5a1cbc5fd..17d666c8a 100644 --- a/src/simlin-engine/src/model.rs +++ b/src/simlin-engine/src/model.rs @@ -173,15 +173,13 @@ pub(crate) fn lower_variable(scope: &ScopeStage0, var_s0: &VariableStage0) -> Va unit_errors, } => { let mut errors = errors.clone(); - let ast = ast - .as_ref() - .and_then(|ast| match lower_ast(scope, ast.clone()) { - Ok(ast) => Some(ast), - Err(err) => { - errors.push(err); - None - } - }); + let ast = ast.as_ref().and_then(|ast| match lower_ast(scope, ast) { + Ok(ast) => Some(ast), + Err(err) => { + errors.push(err); + None + } + }); Variable::Stock { ident: ident.clone(), init_ast: ast, @@ -208,18 +206,16 @@ pub(crate) fn lower_variable(scope: &ScopeStage0, var_s0: &VariableStage0) -> Va unit_errors, } => { let mut errors = errors.clone(); - let ast = ast - .as_ref() - .and_then(|ast| match lower_ast(scope, ast.clone()) { - Ok(ast) => Some(ast), - Err(err) => { - errors.push(err); - None - } - }); + let ast = ast.as_ref().and_then(|ast| match lower_ast(scope, ast) { + Ok(ast) => Some(ast), + Err(err) => { + errors.push(err); + None + } + }); let init_ast = init_ast .as_ref() - .and_then(|ast| match lower_ast(scope, ast.clone()) { + .and_then(|ast| match lower_ast(scope, ast) { Ok(ast) => Some(ast), Err(err) => { errors.push(err); diff --git a/src/simlin-engine/src/variable.rs b/src/simlin-engine/src/variable.rs index 7c2f16611..bdd37d46c 100644 --- a/src/simlin-engine/src/variable.rs +++ b/src/simlin-engine/src/variable.rs @@ -1436,7 +1436,7 @@ pub(crate) fn scalar_ast(eqn: &str) -> Ast { dimensions: &Default::default(), model_name: "test", }; - lower_ast(&scope, ast.unwrap()).unwrap() + lower_ast(&scope, &ast.unwrap()).unwrap() } /// Table-driven matrix test for `classify_dependencies`. From 51cccdb1394f72e9352ede603a316c18c644b842 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 20:48:00 -0700 Subject: [PATCH 05/10] engine: cache the LTM variable reconstruction instead of redoing it per 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>`, 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. --- src/simlin-engine/src/db/analysis.rs | 25 ++++++++++++++----- src/simlin-engine/src/db/ltm/pinned.rs | 2 +- src/simlin-engine/src/ltm/graph.rs | 11 ++++++--- src/simlin-engine/src/ltm/tests.rs | 34 +++++++++++++------------- 4 files changed, 45 insertions(+), 27 deletions(-) diff --git a/src/simlin-engine/src/db/analysis.rs b/src/simlin-engine/src/db/analysis.rs index 4b33bc314..dca0bf9f7 100644 --- a/src/simlin-engine/src/db/analysis.rs +++ b/src/simlin-engine/src/db/analysis.rs @@ -1317,7 +1317,7 @@ pub fn causal_graph_from_edges(result: &CausalEdgesResult) -> crate::ltm::Causal crate::ltm::CausalGraph { edges, stocks, - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), } } @@ -1363,7 +1363,9 @@ pub(crate) fn causal_graph_with_modules( /// The `(variables, module_graphs)` maps a CausalGraph carries for polarity /// analysis, stock enrichment, and the GH #698 per-exit-port recompute. type CausalGraphModuleData = ( - HashMap, crate::variable::Variable>, + std::sync::Arc< + HashMap, crate::variable::Variable>, + >, HashMap, Box>, ); @@ -2851,7 +2853,7 @@ pub fn causal_graph_from_element_edges( crate::ltm::CausalGraph { edges, stocks, - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), } } @@ -3177,7 +3179,7 @@ pub fn model_loop_circuits_tiered( let graph = crate::ltm::CausalGraph { edges: sub_edge_idents, stocks: sub_stocks, - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; let scc = graph.largest_scc_size(); @@ -3347,11 +3349,22 @@ pub fn model_element_cycle_partitions( /// Reconstruct `Variable` objects from salsa-tracked parse results for /// all variables in a model (including implicit variables). +/// +/// Cached, and returning a SHARED map, because it is expensive and repeated: +/// it lowers every variable in the model, and the LTM pipeline builds a causal +/// graph once per query that needs polarity or module structure. On a world3 +/// LTM compile it ran 923 times and was 58% of all instructions executed. +/// Nothing between those calls can change its answer -- it reads only the +/// model's variable set and the per-variable parse memos -- so the repetition +/// was pure recomputation. +#[salsa::tracked] pub(crate) fn reconstruct_model_variables( db: &dyn Db, model: SourceModel, project: SourceProject, -) -> HashMap, crate::variable::Variable> { +) -> std::sync::Arc< + HashMap, crate::variable::Variable>, +> { use crate::common::{Canonical, Ident}; let source_vars = model.variables(db); @@ -3399,7 +3412,7 @@ pub(crate) fn reconstruct_model_variables( } } - variables + std::sync::Arc::new(variables) } /// Reconstruct a single `Variable` by name from a model's parse results. diff --git a/src/simlin-engine/src/db/ltm/pinned.rs b/src/simlin-engine/src/db/ltm/pinned.rs index 7165cbc0a..0b7a27201 100644 --- a/src/simlin-engine/src/db/ltm/pinned.rs +++ b/src/simlin-engine/src/db/ltm/pinned.rs @@ -398,7 +398,7 @@ fn expand_pin_on_element_graph( let sub_graph = crate::ltm::CausalGraph { edges: sub_edges, stocks: sub_stocks, - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; diff --git a/src/simlin-engine/src/ltm/graph.rs b/src/simlin-engine/src/ltm/graph.rs index d0affcb26..9252548af 100644 --- a/src/simlin-engine/src/ltm/graph.rs +++ b/src/simlin-engine/src/ltm/graph.rs @@ -157,8 +157,13 @@ pub struct CausalGraph { pub(crate) edges: HashMap, Vec>>, /// Set of stocks in the model pub(crate) stocks: HashSet>, - /// Variables in the model for polarity analysis - pub(crate) variables: HashMap, Variable>, + /// Variables in the model for polarity analysis. + /// + /// Shared rather than owned: the map is the salsa-cached + /// `db::reconstruct_model_variables`, and a graph is built many times per + /// LTM compile (once per query that needs polarity or module structure) + /// while the variables in it do not change between those builds. + pub(crate) variables: std::sync::Arc, Variable>>, /// Module instances and their internal graphs pub(crate) module_graphs: HashMap, Box>, } @@ -286,7 +291,7 @@ impl CausalGraph { Ok(CausalGraph { edges, stocks, - variables, + variables: std::sync::Arc::new(variables), module_graphs, }) } diff --git a/src/simlin-engine/src/ltm/tests.rs b/src/simlin-engine/src/ltm/tests.rs index b45c2f08a..35a7a84ff 100644 --- a/src/simlin-engine/src/ltm/tests.rs +++ b/src/simlin-engine/src/ltm/tests.rs @@ -1498,7 +1498,7 @@ fn test_calculate_polarity_all_unknown_links() { let graph = CausalGraph { edges: HashMap::new(), stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -1529,7 +1529,7 @@ fn test_calculate_polarity_mixed_unknown_and_known() { let graph = CausalGraph { edges: HashMap::new(), stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -1587,7 +1587,7 @@ fn test_calculate_polarity_all_known_links() { let graph = CausalGraph { edges: HashMap::new(), stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -1661,7 +1661,7 @@ fn test_loop_id_assignment_undetermined_polarity() { let graph = CausalGraph { edges: HashMap::new(), stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -3178,7 +3178,7 @@ fn test_enumerate_pathways_to_outputs_non_standard_output() { let graph = CausalGraph { edges, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -3217,7 +3217,7 @@ fn test_enumerate_pathways_to_outputs_standard_output() { let graph = CausalGraph { edges, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -3262,7 +3262,7 @@ fn test_enumerate_module_pathways_deeper_than_legacy_cap() { let graph = CausalGraph { edges, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -3325,7 +3325,7 @@ fn diamond_chain_graph(n: usize, permute: bool) -> CausalGraph { CausalGraph { edges, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), } } @@ -3491,7 +3491,7 @@ fn module_pathways_work_bounded_on_dead_end_frontier() { let graph = CausalGraph { edges, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -3539,7 +3539,7 @@ fn enrich_with_module_stocks_falls_back_to_all_stocks_on_truncation() { let sub_graph = CausalGraph { edges: sub_edges, stocks: sub_stocks, - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; @@ -3563,7 +3563,7 @@ fn enrich_with_module_stocks_falls_back_to_all_stocks_on_truncation() { let parent = CausalGraph { edges: HashMap::new(), stocks: HashSet::new(), - variables: parent_variables, + variables: std::sync::Arc::new(parent_variables), module_graphs, }; @@ -3612,7 +3612,7 @@ fn tiny_cycle_graph() -> CausalGraph { CausalGraph { edges, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), } } @@ -3673,7 +3673,7 @@ fn build_causal_graph(edges: &[(&str, &[&str])]) -> CausalGraph { CausalGraph { edges: map, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), } } @@ -3703,7 +3703,7 @@ fn indexed_graph_empty_round_trip() { let cg = CausalGraph { edges, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; let circuits = cg @@ -3973,7 +3973,7 @@ fn johnson_empty_graph_no_circuits() { let cg = CausalGraph { edges: HashMap::new(), stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), }; let circuits = cg.find_circuit_node_lists_with_limit(usize::MAX).unwrap(); @@ -4866,7 +4866,7 @@ fn graph_from_edges(edges: &[(&str, &str)]) -> CausalGraph { CausalGraph { edges: map, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), } } @@ -5012,7 +5012,7 @@ fn build_graph_from_pairs(n: usize, pairs: &[(u8, u8)]) -> CausalGraph { CausalGraph { edges: map, stocks: HashSet::new(), - variables: HashMap::new(), + variables: std::sync::Arc::new(HashMap::new()), module_graphs: HashMap::new(), } } From 6277f4474610b3be21d071b76b22a020829309ea Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 21:09:27 -0700 Subject: [PATCH 06/10] engine: build the MDL importer's dimension context once, not per array 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. --- .../src/mdl/convert/dimensions.rs | 20 ++++++++----- src/simlin-engine/src/mdl/convert/mod.rs | 30 +++++++++++++++++++ .../src/mdl/convert/variables.rs | 3 +- 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/src/simlin-engine/src/mdl/convert/dimensions.rs b/src/simlin-engine/src/mdl/convert/dimensions.rs index 0a5e3f87d..604d23a7f 100644 --- a/src/simlin-engine/src/mdl/convert/dimensions.rs +++ b/src/simlin-engine/src/mdl/convert/dimensions.rs @@ -60,7 +60,7 @@ impl<'input> ConversionContext<'input> { for (original_name, canonical) in subscript_names { let dim = self.build_dimension_recursive(&original_name, &canonical)?; - self.dimensions.push(dim); + self.push_dimension(dim); } // Phase 3: build dimension_elements map @@ -91,6 +91,10 @@ impl<'input> ConversionContext<'input> { // Phase 5: materialize equivalence dimensions as actual Dimension entries // and add them to dimension_elements for expand_subscript to find them + // Built first and applied after: `push_dimension` takes `&mut self` + // (it invalidates the memoized dimension context), which cannot be + // called while `self.equivalences` is borrowed by the loop. + let mut aliases: Vec<(String, Dimension)> = Vec::new(); for (src, dst) in &self.equivalences { if let Some(target_dim) = self .dimensions @@ -109,13 +113,15 @@ impl<'input> ConversionContext<'input> { parent: None, }; alias.set_maps_to(dst.clone()); - // Add alias to dimension_elements so expand_subscript can find it - if let DimensionElements::Named(elements) = &alias.elements { - self.dimension_elements - .insert(src.clone(), elements.clone()); - } - self.dimensions.push(alias); + aliases.push((src.clone(), alias)); + } + } + for (src, alias) in aliases { + // Add alias to dimension_elements so expand_subscript can find it + if let DimensionElements::Named(elements) = &alias.elements { + self.dimension_elements.insert(src, elements.clone()); } + self.push_dimension(alias); } Ok(()) diff --git a/src/simlin-engine/src/mdl/convert/mod.rs b/src/simlin-engine/src/mdl/convert/mod.rs index c57c53497..4bcc7daf3 100644 --- a/src/simlin-engine/src/mdl/convert/mod.rs +++ b/src/simlin-engine/src/mdl/convert/mod.rs @@ -53,6 +53,17 @@ pub struct ConversionContext<'input> { symbols: HashMap>, /// Collected dimensions dimensions: Vec, + /// [`Self::dimensions`] in canonicalized `DimensionsContext` form, built on + /// first use. + /// + /// Cached because it is read once per ELEMENT of every arrayed variable + /// (`build_element_context`, resolving cross-dimension mapping + /// substitutions), and building it canonicalizes and interns every + /// dimension's every element name -- 2% of a C-LEARN import's instructions + /// spent rebuilding one immutable value. Invalidated by + /// [`Self::push_dimension`], which is the only way `dimensions` grows, so + /// the cache cannot go stale by construction rather than by convention. + dims_ctx: std::sync::OnceLock, /// Dimension equivalences: source canonical -> target canonical equivalences: HashMap, /// Original (pre-canonicalization) names for equivalence sources @@ -162,6 +173,7 @@ impl<'input> ConversionContext<'input> { items, symbols: HashMap::with_capacity(n_items), dimensions, + dims_ctx: std::sync::OnceLock::new(), equivalences: HashMap::new(), equivalence_original_names: HashMap::new(), sim_specs, @@ -181,6 +193,24 @@ impl<'input> ConversionContext<'input> { } } + /// Append a dimension, dropping the memoized [`Self::dims_ctx`]. + /// + /// The ONLY place `dimensions` grows; going through it is what makes the + /// memo correct without relying on the phase ordering (dimension building + /// happens to finish before any variable is converted today). + pub(in crate::mdl::convert) fn push_dimension(&mut self, dim: Dimension) { + self.dimensions.push(dim); + self.dims_ctx.take(); + } + + /// The canonicalized form of [`Self::dimensions`], built once. + pub(in crate::mdl::convert) fn dimensions_context( + &self, + ) -> &crate::dimensions::DimensionsContext { + self.dims_ctx + .get_or_init(|| crate::dimensions::DimensionsContext::from(self.dimensions.as_slice())) + } + /// Convert the MDL to a Project. pub fn convert(mut self) -> Result { // Pass 1: Collect symbols and build initial symbol table diff --git a/src/simlin-engine/src/mdl/convert/variables.rs b/src/simlin-engine/src/mdl/convert/variables.rs index 9e13ed956..01249d006 100644 --- a/src/simlin-engine/src/mdl/convert/variables.rs +++ b/src/simlin-engine/src/mdl/convert/variables.rs @@ -717,7 +717,6 @@ impl<'input> ConversionContext<'input> { ) -> crate::mdl::xmile_compat::ElementContext { use crate::common::CanonicalDimensionName; use crate::common::CanonicalElementName; - use crate::dimensions::DimensionsContext; use crate::mdl::xmile_compat::ElementContext; debug_assert_eq!( @@ -746,7 +745,7 @@ impl<'input> ConversionContext<'input> { // specific element via the mapping. For example, if DimD maps to // DimA and we're iterating at DimA=A2, then DimD -> D1 (where // D1 is the DimD element that maps to A2). - let dims_ctx = DimensionsContext::from(self.dimensions.as_slice()); + let dims_ctx = self.dimensions_context(); for dim_canonical in self.dimension_elements.keys() { if substitutions.contains_key(dim_canonical) || subrange_mappings.contains_key(dim_canonical) From 11935aea30cdd76ec2afb34c0e2701d1901bf49c Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 21:25:54 -0700 Subject: [PATCH 07/10] engine: derive the single-variable reconstruction from the cached map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/simlin-engine/src/db/analysis.rs | 80 ++++++++-------------------- 1 file changed, 23 insertions(+), 57 deletions(-) diff --git a/src/simlin-engine/src/db/analysis.rs b/src/simlin-engine/src/db/analysis.rs index dca0bf9f7..ef828543f 100644 --- a/src/simlin-engine/src/db/analysis.rs +++ b/src/simlin-engine/src/db/analysis.rs @@ -3407,8 +3407,16 @@ pub(crate) fn reconstruct_model_variables( // Add implicit variables (module instances from SMOOTH/DELAY expansion) for implicit_dm_var in &parsed.implicit_vars { let imp_name = canonicalize(implicit_dm_var.get_ident()).into_owned(); - let lowered_imp = reconstruct_implicit_variable(db, model, &scope, implicit_dm_var); - variables.insert(Ident::new(&imp_name), lowered_imp); + // `or_insert_with`, not `insert`: an explicit variable wins a + // canonical-name collision with a synthesized implicit one. That + // is the precedence `reconstruct_single_variable` had when it + // searched explicit variables first, and it now reads this map. + // Unreachable today -- implicit names carry the reserved `$⁚` + // prefix -- so this fixes no bug; it keeps the two from being + // able to differ. + variables.entry(Ident::new(&imp_name)).or_insert_with(|| { + reconstruct_implicit_variable(db, model, &scope, implicit_dm_var) + }); } } @@ -3417,8 +3425,14 @@ pub(crate) fn reconstruct_model_variables( /// Reconstruct a single `Variable` by name from a model's parse results. /// -/// Checks explicit source variables first, then searches implicit variables -/// (from SMOOTH/DELAY module expansion) if the name isn't found. +/// A projection of [`reconstruct_model_variables`] rather than its own +/// parse-and-lower: the map that query caches already holds every explicit and +/// implicit variable, built by the same construction against the same scope. +/// Deriving one from the other is not merely cheaper (the LTM link-score +/// emitter calls this once per link, and on C-LEARN that is 6,721 times) -- +/// it also means the two can no longer disagree about what a name resolves +/// to, which they could when each searched the model its own way. +/// /// Returns None if the name doesn't match any variable in the model. pub(super) fn reconstruct_single_variable( db: &dyn Db, @@ -3426,59 +3440,11 @@ pub(super) fn reconstruct_single_variable( project: SourceProject, var_name: &str, ) -> Option { - use crate::common::{Canonical, Ident}; - - let source_vars = model.variables(db); - let module_ctx = model_module_ident_context(db, model, project, vec![]); - // The canonicalized dimension context comes from the project-global - // salsa-cached query; every parse and lowering below reads that one value. - let dim_context = project_dimensions_context(db, project); - let models = HashMap::new(); - let scope = crate::model::ScopeStage0 { - models: &models, - dimensions: dim_context, - model_name: "", - }; - - // Check explicit variables first - if let Some(source_var) = source_vars.get(var_name) { - // Explicit module instances take the same direct-construction path - // as implicit ones (see `reconstruct_implicit_variable`'s doc): the - // generic parse+lower path resolves module inputs against - // `scope.models`, which is EMPTY here, so `resolve_module_input` - // drops every input and the reconstructed `Variable::Module` carries - // `inputs: []`. The LTM module link-score generators match the - // edge's source against those inputs to pick the composite-reference - // (exhaustive) or output-port delta-ratio (discovery) formula; with - // the inputs lost they silently fell through to a bare-module-name - // delta-ratio whose fragment cannot compile, zeroing every - // input→user-module link score. - let dm_var = super::datamodel_variable_from_source(db, *source_var); - if matches!(dm_var, datamodel::Variable::Module(_)) { - return Some(reconstruct_implicit_variable(db, model, &scope, &dm_var)); - } - let parsed = - parse_source_variable_with_module_context(db, *source_var, project, module_ctx); - let lowered = crate::model::lower_variable(&scope, &parsed.variable); - return Some(lowered); - } - - // Search implicit variables from all source variables - let canonical_target: Ident = Ident::new(var_name); - - for (_name, source_var) in source_vars.iter() { - let parsed = - parse_source_variable_with_module_context(db, *source_var, project, module_ctx); - for implicit_dm_var in &parsed.implicit_vars { - let imp_name = canonicalize(implicit_dm_var.get_ident()).into_owned(); - if Ident::::new(&imp_name) == canonical_target { - let lowered_imp = reconstruct_implicit_variable(db, model, &scope, implicit_dm_var); - return Some(lowered_imp); - } - } - } - - None + reconstruct_model_variables(db, model, project) + .get(&crate::common::Ident::::new( + var_name, + )) + .cloned() } /// Reconstruct an implicit (compiler-generated) variable from its datamodel form. From d6ea402273cfbb08462b21a59bbdddcf7d878434 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 21:53:24 -0700 Subject: [PATCH 08/10] engine: hash identifier-keyed maps with FxHash 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. --- src/simlin-engine/src/ast/expr3.rs | 11 +- src/simlin-engine/src/common.rs | 20 ++++ src/simlin-engine/src/compiler/codegen.rs | 4 +- src/simlin-engine/src/compiler/context.rs | 68 ++++++----- src/simlin-engine/src/compiler/dimensions.rs | 14 +-- src/simlin-engine/src/compiler/mod.rs | 113 ++++++++++++------- src/simlin-engine/src/db/analysis.rs | 6 +- src/simlin-engine/src/db/assemble.rs | 10 +- src/simlin-engine/src/db/fragment_compile.rs | 14 ++- src/simlin-engine/src/db/ltm/compile.rs | 86 +++++++------- src/simlin-engine/src/db/query.rs | 10 +- src/simlin-engine/src/db/var_fragment.rs | 17 ++- src/simlin-engine/src/dimensions.rs | 15 ++- src/simlin-engine/src/ltm_augment_tests.rs | 3 +- 14 files changed, 238 insertions(+), 153 deletions(-) diff --git a/src/simlin-engine/src/ast/expr3.rs b/src/simlin-engine/src/ast/expr3.rs index 31e0990ba..dcdde85d7 100644 --- a/src/simlin-engine/src/ast/expr3.rs +++ b/src/simlin-engine/src/ast/expr3.rs @@ -1524,11 +1524,12 @@ mod tests { .map(|e| CanonicalElementName::from_raw(e)) .collect(); - let indexed_elements: HashMap = canonical_elements - .iter() - .enumerate() - .map(|(i, e)| (e.clone(), i)) - .collect(); + let indexed_elements: crate::common::IdentMap = + canonical_elements + .iter() + .enumerate() + .map(|(i, e)| (e.clone(), i)) + .collect(); Dimension::Named( CanonicalDimensionName::from_raw(name), diff --git a/src/simlin-engine/src/common.rs b/src/simlin-engine/src/common.rs index 7b114f667..a1402a42d 100644 --- a/src/simlin-engine/src/common.rs +++ b/src/simlin-engine/src/common.rs @@ -923,6 +923,26 @@ fn is_canonical(name: &str) -> bool { /// Note: the borrowed slice may be a sub-slice of the input when there is /// leading/trailing whitespace but the trimmed content is already canonical. /// The returned `Cow` borrows from the input `&str` in all borrowed cases. +/// The engine's hash map for identifier-keyed lookups. +/// +/// `FxHashMap`, not `std::collections::HashMap`. Identifier lookups are the +/// compiler's densest operation -- a name resolution per AST node, per element, +/// per fragment -- and SipHash over a short string was measured at 4-6% of a +/// large model's compile cycles. FxHash's fixed seed additionally makes +/// iteration order reproducible across processes, which is the direction this +/// crate already wants (GH #595): a salsa-cached value built by iterating a map +/// must not differ run to run. +/// +/// The cost, stated because it is the reason this is not the default +/// everywhere: 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. Do not extend this alias to a map keyed by input from a party other +/// than the one paying for the work. +pub(crate) type IdentMap = std::collections::HashMap; + /// Whether `to_lowercase` would change `c`, i.e. Unicode's /// `Changes_When_Lowercased`. /// diff --git a/src/simlin-engine/src/compiler/codegen.rs b/src/simlin-engine/src/compiler/codegen.rs index 2e4651ef1..5534629ab 100644 --- a/src/simlin-engine/src/compiler/codegen.rs +++ b/src/simlin-engine/src/compiler/codegen.rs @@ -97,7 +97,7 @@ pub(super) struct Compiler<'module> { /// large LTM builds), and `Compiler::new` interns every dimension and /// element name up front -- with a linear-scan intern that was O(D^2) /// string comparisons per fragment (GH #655). - name_ids: HashMap, + name_ids: crate::common::IdentMap, static_views: Vec, dim_lists: Vec<(u8, [u16; 4])>, // Iteration context - set when compiling inside AssignTemp @@ -159,7 +159,7 @@ impl<'module> Compiler<'module> { dimensions: vec![], subdim_relations: vec![], names: vec![], - name_ids: HashMap::new(), + name_ids: Default::default(), static_views: vec![], dim_lists: Vec::new(), in_iteration: false, diff --git a/src/simlin-engine/src/compiler/context.rs b/src/simlin-engine/src/compiler/context.rs index 32d428730..ca2dc38aa 100644 --- a/src/simlin-engine/src/compiler/context.rs +++ b/src/simlin-engine/src/compiler/context.rs @@ -2,15 +2,15 @@ // Use of this source code is governed by the Apache License, // Version 2.0, that can be found in the LICENSE file. -use std::collections::{BTreeSet, HashMap}; +use std::collections::BTreeSet; use std::sync::Arc; use crate::ast::{ self, ArrayView, BinaryOp, Expr3, Expr3LowerContext, IndexExpr3, Loc, Pass1Context, }; use crate::common::{ - Canonical, CanonicalDimensionName, CanonicalElementName, ErrorCode, ErrorKind, Ident, Result, - canonicalize, + Canonical, CanonicalDimensionName, CanonicalElementName, ErrorCode, ErrorKind, Ident, IdentMap, + Result, canonicalize, }; use crate::dimensions::{Dimension, DimensionsContext}; use crate::variable::Variable; @@ -50,7 +50,7 @@ pub(crate) struct VariableMetadata<'a> { /// the model being compiled plus every sub-model reachable from it, which is /// what makes a cross-module name resolvable. pub(crate) type MetadataByModel<'a> = - HashMap, HashMap, VariableMetadata<'a>>>; + IdentMap, IdentMap, VariableMetadata<'a>>>; /// Project [`super::VarSizes`] out of the symbol table: the extent of every /// variable a reference in `model` can address **in whole**. @@ -176,7 +176,7 @@ pub(crate) struct ContextCore<'a> { /// table. Derived, never authored: build it with `whole_variable_extents`. pub(crate) var_sizes: &'a super::VarSizes, pub(crate) module_models: - &'a HashMap, HashMap, Ident>>, + &'a IdentMap, IdentMap, Ident>>, pub(crate) inputs: &'a BTreeSet>, } @@ -2747,8 +2747,10 @@ fn test_lower() { }; let inputs = &BTreeSet::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let true_var = Variable::Var { ident: Ident::new(""), ast: None, @@ -2775,7 +2777,8 @@ fn test_lower() { errors: vec![], unit_errors: vec![], }; - let mut metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); metadata.insert( Ident::new("true_input"), VariableMetadata { @@ -2792,7 +2795,7 @@ fn test_lower() { var: &false_var, }, ); - let mut metadata2 = HashMap::new(); + let mut metadata2 = crate::common::IdentMap::default(); let main_ident = Ident::new("main"); let test_ident = Ident::new("test"); metadata2.insert(main_ident.clone(), metadata); @@ -2862,8 +2865,10 @@ fn test_lower() { }; let inputs = &BTreeSet::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let true_var = Variable::Var { ident: Ident::new(""), ast: None, @@ -2890,7 +2895,8 @@ fn test_lower() { errors: vec![], unit_errors: vec![], }; - let mut metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); metadata.insert( Ident::new("true_input"), VariableMetadata { @@ -2907,7 +2913,7 @@ fn test_lower() { var: &false_var, }, ); - let mut metadata2 = HashMap::new(); + let mut metadata2 = crate::common::IdentMap::default(); let main_ident = Ident::new("main"); let test_ident = Ident::new("test"); metadata2.insert(main_ident.clone(), metadata); @@ -2962,10 +2968,14 @@ fn test_with_active_subscripts_reuses_dimension_storage() { CanonicalDimensionName::from_raw("letters"), 3, )]; - let metadata: HashMap, HashMap, VariableMetadata<'_>>> = - HashMap::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let metadata: crate::common::IdentMap< + Ident, + crate::common::IdentMap, VariableMetadata<'_>>, + > = Default::default(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let inputs = BTreeSet::new(); let var_sizes = whole_variable_extents(&metadata, &model_name); @@ -3017,10 +3027,14 @@ fn test_get_implicit_subscript_off_translates_through_mapping_parent() { Dimension::from(&sub_a), Dimension::from(&dim_b), ]; - let metadata: HashMap, HashMap, VariableMetadata<'_>>> = - HashMap::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let metadata: crate::common::IdentMap< + Ident, + crate::common::IdentMap, VariableMetadata<'_>>, + > = Default::default(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let inputs = BTreeSet::new(); let model_name = Ident::new("main"); let ident = Ident::new("test_var"); @@ -3095,7 +3109,7 @@ fn test_positional_fallback_ignores_unrelated_mapping() { unit_errors: vec![], }; - let mut model_metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut model_metadata: IdentMap, VariableMetadata<'_>> = Default::default(); model_metadata.insert( Ident::new("source_var"), VariableMetadata { @@ -3105,13 +3119,15 @@ fn test_positional_fallback_ignores_unrelated_mapping() { }, ); - let mut metadata: HashMap, HashMap, VariableMetadata<'_>>> = - HashMap::new(); + let mut metadata: IdentMap, IdentMap, VariableMetadata<'_>>> = + Default::default(); let model_name = Ident::new("main"); metadata.insert(model_name.clone(), model_metadata); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let inputs = BTreeSet::new(); let ident = Ident::new("test_var"); let var_sizes = whole_variable_extents(&metadata, &model_name); diff --git a/src/simlin-engine/src/compiler/dimensions.rs b/src/simlin-engine/src/compiler/dimensions.rs index 1a8b18099..eafe30ff4 100644 --- a/src/simlin-engine/src/compiler/dimensions.rs +++ b/src/simlin-engine/src/compiler/dimensions.rs @@ -321,14 +321,12 @@ mod tests { .iter() .map(|e| crate::common::CanonicalElementName::from_raw(e)) .collect(); - let indexed_elements: std::collections::HashMap< - crate::common::CanonicalElementName, - usize, - > = canonical_elements - .iter() - .enumerate() - .map(|(i, elem)| (elem.clone(), i + 1)) - .collect(); + let indexed_elements: crate::common::IdentMap = + canonical_elements + .iter() + .enumerate() + .map(|(i, elem)| (elem.clone(), i + 1)) + .collect(); Dimension::Named( CanonicalDimensionName::from_raw(name), NamedDimension { diff --git a/src/simlin-engine/src/compiler/mod.rs b/src/simlin-engine/src/compiler/mod.rs index bfc316cb4..ba974f897 100644 --- a/src/simlin-engine/src/compiler/mod.rs +++ b/src/simlin-engine/src/compiler/mod.rs @@ -77,8 +77,10 @@ pub struct Var { #[test] fn test_fold_flows() { let inputs = &BTreeSet::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let dummy_var = Variable::Var { ident: Ident::new(""), ast: None, @@ -92,7 +94,8 @@ fn test_fold_flows() { errors: vec![], unit_errors: vec![], }; - let mut metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); metadata.insert( Ident::new("a"), VariableMetadata { @@ -125,7 +128,7 @@ fn test_fold_flows() { var: &dummy_var, }, ); - let mut metadata2 = HashMap::new(); + let mut metadata2 = crate::common::IdentMap::default(); let main_ident = Ident::new("main"); let test_ident = Ident::new("test"); metadata2.insert(main_ident.clone(), metadata); @@ -181,9 +184,11 @@ fn test_module_var_new_missing_input_source_returns_error() { let model_name_ident: Ident = Ident::new("sub_model"); // module_models maps "main" -> { "my_module" -> "sub_model" } - let mut module_models: HashMap, HashMap, Ident>> = - HashMap::new(); - let mut main_modules = HashMap::new(); + let mut module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); + let mut main_modules = crate::common::IdentMap::default(); main_modules.insert(module_ident.clone(), model_name_ident.clone()); let main_ident = Ident::new("main"); module_models.insert(main_ident.clone(), main_modules); @@ -202,7 +207,8 @@ fn test_module_var_new_missing_input_source_returns_error() { }; // metadata only contains "my_module" -- NOT "missing_source" - let mut metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); metadata.insert( module_ident.clone(), VariableMetadata { @@ -211,7 +217,7 @@ fn test_module_var_new_missing_input_source_returns_error() { var: &module_var, }, ); - let mut metadata2 = HashMap::new(); + let mut metadata2 = crate::common::IdentMap::default(); metadata2.insert(main_ident.clone(), metadata); let dims_ctx = DimensionsContext::default(); @@ -240,8 +246,10 @@ fn test_module_var_new_missing_input_source_returns_error() { #[test] fn test_build_stock_update_expr_inflows_only() { let inputs = &BTreeSet::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let stock_var = Variable::Stock { ident: Ident::new("stock"), init_ast: None, @@ -266,7 +274,8 @@ fn test_build_stock_update_expr_inflows_only() { errors: vec![], unit_errors: vec![], }; - let mut metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); metadata.insert( Ident::new("stock"), VariableMetadata { @@ -283,7 +292,7 @@ fn test_build_stock_update_expr_inflows_only() { var: &dummy_var, }, ); - let mut metadata2 = HashMap::new(); + let mut metadata2 = crate::common::IdentMap::default(); let main_ident = Ident::new("main"); let test_ident = Ident::new("test"); metadata2.insert(main_ident.clone(), metadata); @@ -339,8 +348,10 @@ fn test_build_stock_update_expr_inflows_only() { #[test] fn test_build_stock_update_expr_outflows_only() { let inputs = &BTreeSet::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let stock_var = Variable::Stock { ident: Ident::new("stock"), init_ast: None, @@ -365,7 +376,8 @@ fn test_build_stock_update_expr_outflows_only() { errors: vec![], unit_errors: vec![], }; - let mut metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); metadata.insert( Ident::new("stock"), VariableMetadata { @@ -382,7 +394,7 @@ fn test_build_stock_update_expr_outflows_only() { var: &dummy_var, }, ); - let mut metadata2 = HashMap::new(); + let mut metadata2 = crate::common::IdentMap::default(); let main_ident = Ident::new("main"); let test_ident = Ident::new("test"); metadata2.insert(main_ident.clone(), metadata); @@ -437,8 +449,10 @@ fn test_build_stock_update_expr_outflows_only() { #[test] fn test_build_stock_update_expr_no_flows() { let inputs = &BTreeSet::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let stock_var = Variable::Stock { ident: Ident::new("stock"), init_ast: None, @@ -463,7 +477,8 @@ fn test_build_stock_update_expr_no_flows() { errors: vec![], unit_errors: vec![], }; - let mut metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); metadata.insert( Ident::new("stock"), VariableMetadata { @@ -472,7 +487,7 @@ fn test_build_stock_update_expr_no_flows() { var: &dummy_var, }, ); - let mut metadata2 = HashMap::new(); + let mut metadata2 = crate::common::IdentMap::default(); let main_ident = Ident::new("main"); let test_ident = Ident::new("test"); metadata2.insert(main_ident.clone(), metadata); @@ -522,8 +537,10 @@ fn test_build_stock_update_expr_no_flows() { #[test] fn test_build_stock_update_expr_multiple_flows() { let inputs = &BTreeSet::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let stock_var = Variable::Stock { ident: Ident::new("stock"), init_ast: None, @@ -548,7 +565,8 @@ fn test_build_stock_update_expr_multiple_flows() { errors: vec![], unit_errors: vec![], }; - let mut metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); for (name, off) in [ ("stock", 0), ("in1", 1), @@ -565,7 +583,7 @@ fn test_build_stock_update_expr_multiple_flows() { }, ); } - let mut metadata2 = HashMap::new(); + let mut metadata2 = crate::common::IdentMap::default(); let main_ident = Ident::new("main"); let test_ident = Ident::new("test"); metadata2.insert(main_ident.clone(), metadata); @@ -690,7 +708,8 @@ fn test_arrayed_default_equation_applies_to_missing_elements() { unit_errors: vec![], }; - let mut model_metadata: HashMap, VariableMetadata<'_>> = HashMap::new(); + let mut model_metadata: crate::common::IdentMap, VariableMetadata<'_>> = + Default::default(); model_metadata.insert( Ident::new("x"), VariableMetadata { @@ -699,13 +718,15 @@ fn test_arrayed_default_equation_applies_to_missing_elements() { var: &var, }, ); - let mut metadata = HashMap::new(); + let mut metadata = crate::common::IdentMap::default(); let model_name = Ident::new("main"); metadata.insert(model_name.clone(), model_metadata); let inputs = BTreeSet::new(); - let module_models: HashMap, HashMap, Ident>> = - HashMap::new(); + let module_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let dims_ctx = DimensionsContext::from(std::slice::from_ref(&datamodel_dim)); let ident = Ident::new("test"); let var_sizes = whole_variable_extents(&metadata, &model_name); @@ -2829,9 +2850,14 @@ pub struct Module { pub(crate) fn calc_module_model_map( project: &Project, model_name: &Ident, -) -> HashMap, HashMap, Ident>> { - let mut all_models: HashMap, HashMap, Ident>> = - HashMap::new(); +) -> crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, +> { + let mut all_models: crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = Default::default(); let model = Arc::clone(&project.models[model_name]); let var_names: Vec<&str> = { @@ -2840,7 +2866,8 @@ pub(crate) fn calc_module_model_map( var_names }; - let mut current_mapping: HashMap, Ident> = HashMap::new(); + let mut current_mapping: crate::common::IdentMap, Ident> = + Default::default(); for ident in var_names.iter() { let canonical_ident = Ident::new(ident); @@ -2865,7 +2892,10 @@ pub(crate) fn build_metadata<'p>( project: &'p Project, model_name: &Ident, is_root: bool, - all_offsets: &mut HashMap, HashMap, VariableMetadata<'p>>>, + all_offsets: &mut crate::common::IdentMap< + Ident, + crate::common::IdentMap, VariableMetadata<'p>>, + >, ) { use std::sync::LazyLock; @@ -2929,8 +2959,8 @@ pub(crate) fn build_metadata<'p>( var_names }; let var_count = var_names.len() + if is_root { IMPLICIT_VAR_COUNT } else { 0 }; - let mut offsets: HashMap, VariableMetadata<'p>> = - HashMap::with_capacity(var_count); + let mut offsets: crate::common::IdentMap, VariableMetadata<'p>> = + crate::common::IdentMap::with_capacity_and_hasher(var_count, Default::default()); let mut i = 0; if is_root { @@ -2999,7 +3029,10 @@ pub(crate) fn build_metadata<'p>( #[cfg(test)] fn calc_n_slots( - all_metadata: &HashMap, HashMap, VariableMetadata<'_>>>, + all_metadata: &crate::common::IdentMap< + Ident, + crate::common::IdentMap, VariableMetadata<'_>>, + >, model_name: &Ident, ) -> usize { let metadata = &all_metadata[model_name]; @@ -3063,7 +3096,11 @@ impl Module { } let model_name: &Ident = &model.name; - let mut metadata = HashMap::with_capacity(project.models.len()); + let mut metadata: crate::common::IdentMap<_, _> = + crate::common::IdentMap::with_capacity_and_hasher( + project.models.len(), + Default::default(), + ); build_metadata(project, model_name, is_root, &mut metadata); let n_slots = calc_n_slots(&metadata, model_name); diff --git a/src/simlin-engine/src/db/analysis.rs b/src/simlin-engine/src/db/analysis.rs index ef828543f..e74572db2 100644 --- a/src/simlin-engine/src/db/analysis.rs +++ b/src/simlin-engine/src/db/analysis.rs @@ -3500,7 +3500,6 @@ mod emit_edges_for_reference_tests { use super::*; use crate::common::{CanonicalDimensionName, CanonicalElementName}; use crate::dimensions::{Dimension, NamedDimension}; - use std::collections::HashMap as StdHashMap; /// Build a single-dim `Named` dimension from raw element names. /// Mirrors `make_named_dimension` in `ltm_augment.rs::tests` -- inlined @@ -3510,7 +3509,7 @@ mod emit_edges_for_reference_tests { .iter() .map(|e| CanonicalElementName::from_raw(e)) .collect(); - let indexed: StdHashMap = canonical_elements + let indexed: crate::common::IdentMap = canonical_elements .iter() .enumerate() .map(|(i, e)| (e.clone(), i + 1)) @@ -4804,7 +4803,6 @@ mod classify_cycle_tests { use super::*; use crate::common::{CanonicalDimensionName, CanonicalElementName}; use crate::dimensions::{Dimension, NamedDimension}; - use std::collections::HashMap as StdHashMap; /// Helper: build a single-dim Named dimension whose elements are /// `["a", "b"]`. The `name` is the canonical dimension name. @@ -4813,7 +4811,7 @@ mod classify_cycle_tests { CanonicalElementName::from_raw("a"), CanonicalElementName::from_raw("b"), ]; - let indexed: StdHashMap = elements + let indexed: crate::common::IdentMap = elements .iter() .enumerate() .map(|(i, e)| (e.clone(), i + 1)) diff --git a/src/simlin-engine/src/db/assemble.rs b/src/simlin-engine/src/db/assemble.rs index 124100abc..2e36bbde9 100644 --- a/src/simlin-engine/src/db/assemble.rs +++ b/src/simlin-engine/src/db/assemble.rs @@ -198,9 +198,9 @@ pub(crate) fn build_submodel_metadata<'arena>( db: &dyn Db, sub_model: SourceModel, project: SourceProject, - all_metadata: &mut HashMap< + all_metadata: &mut crate::common::IdentMap< Ident, - HashMap, crate::compiler::VariableMetadata<'arena>>, + crate::common::IdentMap, crate::compiler::VariableMetadata<'arena>>, >, ) { let sub_model_name: Ident = Ident::new(sub_model.name(db)); @@ -213,8 +213,10 @@ pub(crate) fn build_submodel_metadata<'arena>( let source_vars = sub_model.variables(db); let project_models = project.models(db); - let mut sub_metadata: HashMap, crate::compiler::VariableMetadata<'arena>> = - HashMap::new(); + let mut sub_metadata: crate::common::IdentMap< + Ident, + crate::compiler::VariableMetadata<'arena>, + > = Default::default(); let mut sorted_names: Vec<&String> = source_vars.keys().collect(); sorted_names.sort_unstable(); diff --git a/src/simlin-engine/src/db/fragment_compile.rs b/src/simlin-engine/src/db/fragment_compile.rs index 761c0a10b..2039c7872 100644 --- a/src/simlin-engine/src/db/fragment_compile.rs +++ b/src/simlin-engine/src/db/fragment_compile.rs @@ -604,8 +604,10 @@ pub(crate) fn compile_implicit_var_phase_bytecodes( // The minimal symbol table for this helper: itself plus its dependencies, // carrying no offsets. See `lower_var_fragment` for why a fragment must not // know where its own model's variables live. - let mut mini_metadata: HashMap, crate::compiler::VariableMetadata<'_>> = - HashMap::new(); + let mut mini_metadata: crate::common::IdentMap< + Ident, + crate::compiler::VariableMetadata<'_>, + > = Default::default(); let project_models = project.models(db); let self_size = if meta.is_module { @@ -877,10 +879,10 @@ pub(crate) fn compile_implicit_var_phase_bytecodes( } } - let mut all_metadata: HashMap< + let mut all_metadata: crate::common::IdentMap< Ident, - HashMap, crate::compiler::VariableMetadata<'_>>, - > = HashMap::new(); + crate::common::IdentMap, crate::compiler::VariableMetadata<'_>>, + > = Default::default(); all_metadata.insert(model_name_ident.clone(), mini_metadata); for sub_model in extra_submodels.values() { @@ -939,7 +941,7 @@ pub(crate) fn compile_implicit_var_phase_bytecodes( model_module_map(db, model, project).clone() } else { - HashMap::new() + Default::default() }; // Reference extents in the per-variable form BOTH halves borrow: lowering's diff --git a/src/simlin-engine/src/db/ltm/compile.rs b/src/simlin-engine/src/db/ltm/compile.rs index d3eea2f02..72b9783ef 100644 --- a/src/simlin-engine/src/db/ltm/compile.rs +++ b/src/simlin-engine/src/db/ltm/compile.rs @@ -898,8 +898,10 @@ pub(crate) fn compile_ltm_equation_fragment( // Arena for sub-model stub variables allocated by build_submodel_metadata let arena = bumpalo::Bump::new(); - let mut mini_metadata: HashMap, crate::compiler::VariableMetadata<'_>> = - HashMap::new(); + let mut mini_metadata: crate::common::IdentMap< + Ident, + crate::compiler::VariableMetadata<'_>, + > = Default::default(); // Add implicit time/dt/initial_time/final_time variables { @@ -1525,10 +1527,10 @@ pub(crate) fn compile_ltm_equation_fragment( } // Build the all_metadata map - let mut all_metadata: HashMap< + let mut all_metadata: crate::common::IdentMap< Ident, - HashMap, crate::compiler::VariableMetadata<'_>>, - > = HashMap::new(); + crate::common::IdentMap, crate::compiler::VariableMetadata<'_>>, + > = Default::default(); all_metadata.insert(model_name_ident.clone(), mini_metadata); // Populate sub-model metadata for implicit module sub-models @@ -1545,20 +1547,22 @@ pub(crate) fn compile_ltm_equation_fragment( // function runs once per LTM synthetic var, ~6.7k times on C-LEARN). let base_module_models = model_module_map(db, model, project); let merged_module_models; - let module_models: &HashMap, HashMap, Ident>> = - if implicit_module_refs.is_empty() { - base_module_models - } else { - merged_module_models = { - let mut merged = base_module_models.clone(); - let current_model_modules = merged.entry(model_name_ident.clone()).or_default(); - for (var_ident, (sub_model_name, _input_set)) in &implicit_module_refs { - current_model_modules.insert(var_ident.clone(), sub_model_name.clone()); - } - merged - }; - &merged_module_models + let module_models: &crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = if implicit_module_refs.is_empty() { + base_module_models + } else { + merged_module_models = { + let mut merged = base_module_models.clone(); + let current_model_modules = merged.entry(model_name_ident.clone()).or_default(); + for (var_ident, (sub_model_name, _input_set)) in &implicit_module_refs { + current_model_modules.insert(var_ident.clone(), sub_model_name.clone()); + } + merged }; + &merged_module_models + }; // The LTM synthetic var's fragment goes through the SAME emission entry // point the explicit and implicit paths use. This site used to carry a @@ -2047,8 +2051,10 @@ pub(crate) fn compile_ltm_implicit_var_fragment( // Arena for sub-model stub variables let arena = bumpalo::Bump::new(); - let mut mini_metadata: HashMap, crate::compiler::VariableMetadata<'_>> = - HashMap::new(); + let mut mini_metadata: crate::common::IdentMap< + Ident, + crate::compiler::VariableMetadata<'_>, + > = Default::default(); // Add implicit time/dt/initial_time/final_time { @@ -2424,10 +2430,10 @@ pub(crate) fn compile_ltm_implicit_var_fragment( } } - let mut all_metadata: HashMap< + let mut all_metadata: crate::common::IdentMap< Ident, - HashMap, crate::compiler::VariableMetadata<'_>>, - > = HashMap::new(); + crate::common::IdentMap, crate::compiler::VariableMetadata<'_>>, + > = Default::default(); all_metadata.insert(model_name_ident.clone(), mini_metadata); // Build sub-model metadata for module-type implicit vars and any @@ -2465,23 +2471,25 @@ pub(crate) fn compile_ltm_implicit_var_fragment( let base_module_models = model_module_map(db, model, project); let ltm_module_refs = model_ltm_implicit_module_refs(db, model, project); let merged_module_models; - let module_models: &HashMap, HashMap, Ident>> = - if module_refs.is_empty() && ltm_module_refs.is_empty() { - base_module_models - } else { - merged_module_models = { - let mut merged = base_module_models.clone(); - let current_model_modules = merged.entry(model_name_ident.clone()).or_default(); - for (var_ident, (sub_model_name, _input_set)) in &module_refs { - current_model_modules.insert(var_ident.clone(), sub_model_name.clone()); - } - for (im_ident, sub_model_name) in ltm_module_refs.iter() { - current_model_modules.insert(im_ident.clone(), sub_model_name.clone()); - } - merged - }; - &merged_module_models + let module_models: &crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + > = if module_refs.is_empty() && ltm_module_refs.is_empty() { + base_module_models + } else { + merged_module_models = { + let mut merged = base_module_models.clone(); + let current_model_modules = merged.entry(model_name_ident.clone()).or_default(); + for (var_ident, (sub_model_name, _input_set)) in &module_refs { + current_model_modules.insert(var_ident.clone(), sub_model_name.clone()); + } + for (im_ident, sub_model_name) in ltm_module_refs.iter() { + current_model_modules.insert(im_ident.clone(), sub_model_name.clone()); + } + merged }; + &merged_module_models + }; // Same single emission entry point as every other fragment site. This was // the second hand-copied duplicate of that body; it differed from the diff --git a/src/simlin-engine/src/db/query.rs b/src/simlin-engine/src/db/query.rs index 1b9420142..2c0ce734c 100644 --- a/src/simlin-engine/src/db/query.rs +++ b/src/simlin-engine/src/db/query.rs @@ -20,7 +20,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use super::*; -use crate::common::{Canonical, Ident}; +use crate::common::{Canonical, Ident, IdentMap}; /// Result of parsing a single variable, including any implicit variables /// generated by builtin expansion (e.g., DELAY1, SMTH create internal stocks). @@ -608,14 +608,14 @@ pub fn model_module_map( db: &dyn Db, model: SourceModel, project: SourceProject, -) -> HashMap, HashMap, Ident>> { +) -> IdentMap, IdentMap, Ident>> { let source_vars = model.variables(db); let project_models = project.models(db); let model_name_ident: Ident = Ident::new(model.name(db)); - let mut all_models: HashMap, HashMap, Ident>> = - HashMap::new(); - let mut current_mapping: HashMap, Ident> = HashMap::new(); + let mut all_models: IdentMap, IdentMap, Ident>> = + IdentMap::default(); + let mut current_mapping: IdentMap, Ident> = IdentMap::default(); let mut sorted_names: Vec<&String> = source_vars.keys().collect(); sorted_names.sort_unstable(); diff --git a/src/simlin-engine/src/db/var_fragment.rs b/src/simlin-engine/src/db/var_fragment.rs index 42d4954bf..01ac02bfb 100644 --- a/src/simlin-engine/src/db/var_fragment.rs +++ b/src/simlin-engine/src/db/var_fragment.rs @@ -571,7 +571,10 @@ pub(crate) fn lower_var_fragment( converted_dims: &[crate::dimensions::Dimension], dim_context: &crate::dimensions::DimensionsContext, model_name_ident: &Ident, - module_models: &HashMap, HashMap, Ident>>, + module_models: &crate::common::IdentMap< + Ident, + crate::common::IdentMap, Ident>, + >, inputs: &BTreeSet>, ) -> LoweredVarFragment { let var_ident = var.ident(db).clone(); @@ -826,8 +829,10 @@ pub(crate) fn lower_var_fragment( // implicit globals are absent for a separate reason: they lower to // `LoadGlobalVar` at fixed absolute slots and never go through a symbol // table at all.) - let mut mini_metadata: HashMap, crate::compiler::VariableMetadata<'_>> = - HashMap::new(); + let mut mini_metadata: crate::common::IdentMap< + Ident, + crate::compiler::VariableMetadata<'_>, + > = Default::default(); // Add self mini_metadata.insert( @@ -885,10 +890,10 @@ pub(crate) fn lower_var_fragment( } // Build the all_metadata map (model_name -> var_name -> metadata) - let mut all_metadata: HashMap< + let mut all_metadata: crate::common::IdentMap< Ident, - HashMap, crate::compiler::VariableMetadata<'_>>, - > = HashMap::new(); + crate::common::IdentMap, crate::compiler::VariableMetadata<'_>>, + > = Default::default(); all_metadata.insert(model_name_ident.clone(), mini_metadata); // Populate sub-model metadata for implicit and explicit module sub-models diff --git a/src/simlin-engine/src/dimensions.rs b/src/simlin-engine/src/dimensions.rs index 8d23692f3..2fb07074d 100644 --- a/src/simlin-engine/src/dimensions.rs +++ b/src/simlin-engine/src/dimensions.rs @@ -2,19 +2,18 @@ // Use of this source code is governed by the Apache License, // Version 2.0, that can be found in the LICENSE file. -use std::collections::HashMap; use std::sync::Mutex; use smallvec::SmallVec; -use crate::common::{CanonicalDimensionName, CanonicalElementName}; +use crate::common::{CanonicalDimensionName, CanonicalElementName, IdentMap}; use crate::datamodel; #[cfg_attr(feature = "debug-derive", derive(Debug))] #[derive(Clone, PartialEq, Eq, salsa::Update)] pub struct NamedDimension { pub elements: Vec, - pub indexed_elements: HashMap, + pub indexed_elements: IdentMap, /// If this dimension maps to another (e.g., DimA -> DimB), the target dimension name. /// Elements correspond positionally: elements[i] of this dimension corresponds to /// elements[i] of the target dimension. @@ -86,7 +85,7 @@ impl SubdimensionRelation { #[derive(Default)] struct RelationshipCache { cache: Mutex< - HashMap<(CanonicalDimensionName, CanonicalDimensionName), Option>, + IdentMap<(CanonicalDimensionName, CanonicalDimensionName), Option>, >, } @@ -256,7 +255,7 @@ impl From<&datamodel::Dimension> for Dimension { .iter() .map(|e| CanonicalElementName::from_raw(e)) .collect(); - let indexed_elements: HashMap = canonical_elements + let indexed_elements: IdentMap = canonical_elements .iter() .enumerate() // system dynamic indexes are 1-indexed @@ -310,9 +309,9 @@ pub struct DimensionsContext { /// sharing is safe. The `relationship_cache` memo stays per-instance /// (cloned cold), which only costs re-deriving subdimension relations on /// first use. - dimensions: std::sync::Arc>, + dimensions: std::sync::Arc>, /// For indexed subdimensions, maps child dimension name to its declared parent. - indexed_parents: std::sync::Arc>, + indexed_parents: std::sync::Arc>, relationship_cache: RelationshipCache, } @@ -384,7 +383,7 @@ impl DimensionsContext { } } - let indexed_parents: HashMap = dimensions + let indexed_parents: IdentMap = dimensions .iter() .filter_map(|dim| { dim.parent.as_ref().map(|parent_name| { diff --git a/src/simlin-engine/src/ltm_augment_tests.rs b/src/simlin-engine/src/ltm_augment_tests.rs index 34bc349d8..797c8b088 100644 --- a/src/simlin-engine/src/ltm_augment_tests.rs +++ b/src/simlin-engine/src/ltm_augment_tests.rs @@ -14,12 +14,11 @@ use crate::db::ltm_ir::OccurrenceRef; use crate::dimensions::{Dimension, NamedDimension}; fn make_named_dimension(name: &str, elements: &[&str]) -> Dimension { - use std::collections::HashMap; let canonical_elements: Vec = elements .iter() .map(|e| CanonicalElementName::from_raw(e)) .collect(); - let indexed: HashMap = canonical_elements + let indexed: crate::common::IdentMap = canonical_elements .iter() .enumerate() // 1-indexed, matching `impl From<&datamodel::Dimension>` -- production From 915985dfeabecf5088e36bb10a0cb1626c2fa4f0 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 28 Jul 2026 22:09:23 -0700 Subject: [PATCH 09/10] test: un-ignore the heavy gates the compile speedups made affordable 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. --- docs/dev/rust.md | 16 +++++ .../tests/integration/clearn_unit_errors.rs | 4 +- .../tests/integration/metasd_macros.rs | 22 +++--- .../tests/integration/simulate.rs | 72 ++++++++++--------- 4 files changed, 70 insertions(+), 44 deletions(-) diff --git a/docs/dev/rust.md b/docs/dev/rust.md index a37d75540..c3f4e3fb9 100644 --- a/docs/dev/rust.md +++ b/docs/dev/rust.md @@ -73,6 +73,22 @@ fn my_expensive_test() { ... } Prefer `--release` for expensive tests -- enumeration, simulation, and layout code can be 10-50x faster than debug. +**`#[ignore]` for runtime is a judgement about today's engine, so re-take it after the engine gets faster.** An ignored gate does not run in pre-commit or CI, which means it catches nothing until someone remembers to ask for it -- `clearn_ltm_var_count_guardrail`'s own doc comment recorded a regression that slipped through for exactly that reason. After a compile-time improvement, time the ignored set and un-ignore what now fits: + +```bash +cargo test -p simlin-engine --test integration --no-run +RUSTC_BOOTSTRAP=1 cargo test -p simlin-engine --test integration -- --ignored \ + -Z unstable-options --report-time 2>&1 | grep 'ok <' | sort -t'<' -k2 -rn +``` + +Then check the whole suite against CI's budget rather than trusting a developer machine, which has far more cores than a runner: + +```bash +RUST_TEST_THREADS=4 taskset -c 0-3 cargo test --workspace # approximates a CI runner +``` + +When a test stays ignored, say WHY in the attribute or the doc comment, and make the reason falsifiable: "runtime class" goes stale, while "executing C-LEARN under the non-JIT wasm interpreter, which no compiler speedup touches" or "a strict subset of a test that now runs by default" does not. + ## Code Quality - No placeholder comments ("this is a placeholder"). Use `todo!()` or `unimplemented!()` macros for stubbed-out code, but generally continue working until the implementation is complete. diff --git a/src/simlin-engine/tests/integration/clearn_unit_errors.rs b/src/simlin-engine/tests/integration/clearn_unit_errors.rs index f6c04a393..127d08330 100644 --- a/src/simlin-engine/tests/integration/clearn_unit_errors.rs +++ b/src/simlin-engine/tests/integration/clearn_unit_errors.rs @@ -194,8 +194,10 @@ fn dump_clearn_unit_diagnostics() { /// IF-branch unit difference) plus one umbrella inference warning -- so we /// also assert a coarse total bound to catch gross regressions without pinning /// the exact residual. +/// +/// Runs by default: loading the 1.4MB C-LEARN model, which is what this used to +/// be `#[ignore]`d for, is now under two seconds on a debug build. #[test] -#[ignore = "loads the 1.4MB C-LEARN model; run explicitly with --ignored"] fn clearn_unit_error_flood_is_cleared() { let project = load_clearn(); let macro_models: Vec = project diff --git a/src/simlin-engine/tests/integration/metasd_macros.rs b/src/simlin-engine/tests/integration/metasd_macros.rs index c691c33aa..492391aaf 100644 --- a/src/simlin-engine/tests/integration/metasd_macros.rs +++ b/src/simlin-engine/tests/integration/metasd_macros.rs @@ -467,10 +467,13 @@ fn metasd_expansion_tier() { /// macros.AC6.4 (expansion tier, the heavy real-world models). Same /// assertion as `metasd_expansion_tier` for the large models whose -/// compile exceeds the per-test time budget (beer-game ~1.2s, FREE ~1.2s, -/// covid19, scirev7 ~2.5s, scirev8 ~3.6s). `#[ignore]`d with a documented -/// opt-in per `docs/dev/rust.md`. -// Run with: cargo test -p simlin-engine --test integration --release -- --ignored metasd_expansion_tier_heavy +/// compile used to exceed the per-test time budget. +/// +/// Still `#[ignore]`d, but no longer for time: `metasd_expansion_tier_full` +/// now runs by default and is a strict superset of this, so running both in +/// the default suite would buy nothing. Kept as the focused subset to reach for +/// when the full tier fails and the light models are not the culprit. +// Run with: cargo test -p simlin-engine --test integration -- --ignored metasd_expansion_tier_heavy #[test] #[ignore] fn metasd_expansion_tier_heavy() { @@ -479,12 +482,13 @@ fn metasd_expansion_tier_heavy() { /// The full expansion tier over ALL 17 macro-using files in one run /// (light + heavy), the AC6.4 "all 14 macro-using metasd models pass the -/// expansion tier" check. `#[ignore]`d (sum of compiles ~10s, over the -/// per-test budget); the default `metasd_expansion_tier` covers the light -/// subset on every build. -// Run with: cargo test -p simlin-engine --test integration --release -- --ignored metasd_expansion_tier_full +/// expansion tier" check. +/// +/// Runs by default. The "sum of compiles ~10s" that put it over the per-test +/// budget is now under three seconds on a debug build, and this is the +/// assertion the acceptance criterion is actually about -- the light-subset +/// `metasd_expansion_tier` was the compromise, not the goal. #[test] -#[ignore] fn metasd_expansion_tier_full() { run_expansion_tier(CORPUS.iter()); } diff --git a/src/simlin-engine/tests/integration/simulate.rs b/src/simlin-engine/tests/integration/simulate.rs index fa791a6b0..4c68e9963 100644 --- a/src/simlin-engine/tests/integration/simulate.rs +++ b/src/simlin-engine/tests/integration/simulate.rs @@ -2777,11 +2777,13 @@ fn partition_places_invariant_vars_before_dynamic() { /// Heavy soundness oracle on C-LEARN: assert ZERO of the invariant-classified /// slots vary, and report the count for comparison against the prototype's -/// 678/1368. `#[ignore]`d for runtime class (C-LEARN is ~53k lines). +/// 678/1368. /// -/// Run with: cargo test --release -- --ignored oracle_clearn +/// Runs by default. It was `#[ignore]`d for runtime class, which the compile +/// speedups of the `engine-compile-perf` branch retired: C-LEARN's debug-build +/// compile now fits the per-test budget. An `#[ignore]`d soundness oracle only +/// runs when someone remembers to ask for it. #[test] -#[ignore] fn oracle_clearn() { let datamodel = clearn_datamodel(); let (n_invariant, violations) = invariance_oracle(&datamodel); @@ -2796,11 +2798,9 @@ fn oracle_clearn() { ); } -/// Heavy soundness oracle on WORLD3-03. -/// -/// Run with: cargo test --release -- --ignored oracle_wrld3 +/// Heavy soundness oracle on WORLD3-03. Runs by default (sub-second even on a +/// debug build); see [`oracle_clearn`] for why its heavy twin does too. #[test] -#[ignore] fn oracle_wrld3() { let mdl_path = "../../test/metasd/WRLD3-03/wrld3-03.mdl"; let contents = std::fs::read_to_string(mdl_path) @@ -2845,8 +2845,7 @@ fn midpoint_save_time(specs: &Specs) -> f64 { /// available parity check for this model (both backends consume the same /// `CompiledSimulation`, so any divergence is a wasm lowering bug). A /// `WasmGenError::Unsupported` would be a hard failure: WORLD3 is a -/// core-simulation model the VM handles. `#[ignore]`d for runtime class, like -/// the other heavy models. +/// core-simulation model the VM handles. /// /// In addition to the single-`run` vs VM parity check, this twin re-runs the /// same blob through a TWO-SEGMENT `run_to` (split at the midpoint save time) and @@ -2855,9 +2854,10 @@ fn midpoint_save_time(specs: &Specs) -> f64 { /// Comparing the full final series (both segments run through to `stop`) is exact /// regardless of where the cursor paused mid-run. /// -/// Run with: cargo test --release -- --ignored simulates_wrld3_03_wasm +/// Runs by default: about a second on a debug build. It was `#[ignore]`d for +/// runtime class alongside the other heavy models, which the compile speedups +/// of the `engine-compile-perf` branch retired for this one. #[test] -#[ignore] fn simulates_wrld3_03_wasm() { let mdl_path = "../../test/metasd/WRLD3-03/wrld3-03.mdl"; @@ -3026,9 +3026,13 @@ const EXPECTED_VDF_RESIDUAL: &[&str] = &[ // nondeterminism, was fixed in `e24b0080`). The exclusion is a transparent, // documented, tracked carve-out -- NOT a tolerance loosening; the hard 1% gate // holds for every non-excluded variable and the matched floor is checked after -// exclusion. Run with: cargo test --release -- --ignored simulates_clearn +// exclusion. +// +// Runs by default. This was `#[ignore]`d for runtime class, which the compile +// speedups of the `engine-compile-perf` branch retired: the whole test is a few +// seconds on a debug build. It is the engine's cross-simulator ground-truth +// gate, so it earns its place in the default run more than any other test here. #[test] -#[ignore] fn simulates_clearn() { let (vdf_results, results) = run_clearn_vs_vdf(); @@ -3103,9 +3107,10 @@ fn run_clearn_vs_vdf() -> (Results, Results) { /// `CompiledSimulation` produced by `compile_project_incremental`, so the wasm /// output must clear the gate exactly as the VM does (a divergence is a wasm /// lowering bug); the residual carve-out is identical because it is a property -/// of the model + reference data, not the execution engine. `#[ignore]`d for -/// runtime class -- C-LEARN under the non-JIT interpreter is slow -- exactly -/// like `simulates_clearn`. +/// of the model + reference data, not the execution engine. Still `#[ignore]`d +/// for runtime class, unlike its VM twin `simulates_clearn`: what is slow here +/// is not compilation but EXECUTING C-LEARN under the non-JIT DLR-FT +/// interpreter, which no compiler speedup touches (~34s on a debug build). /// /// A `WasmGenError::Unsupported` here would be a hard failure: C-LEARN is a /// core-simulation model the VM handles, so the wasm backend must too. @@ -3169,12 +3174,9 @@ fn simulates_clearn_wasm() { /// symmetric difference if the residual GREW (a regression -- new divergence to /// investigate, file under #590/#591) or SHRANK (an engine fix -- prune the now- /// passing base from the exclusion). Reuses `classify_vdf_ident` (no comparator -/// duplication). `#[ignore]`d for runtime class like `simulates_clearn`. -/// -/// Run with: -/// cargo test --release -- --ignored clearn_residual_exactness +/// duplication). Runs by default like `simulates_clearn`, whose fixture it +/// shares. #[test] -#[ignore] fn clearn_residual_exactness() { use std::collections::BTreeSet; @@ -5476,10 +5478,9 @@ fn corpus_theil_multi_output_materializes_and_simulates() { /// and produced no macro-specific compile diagnostics"; the unrelated /// GET-DIRECT-data blocker is reported for Phase-7 tiered-harness scope. /// -/// `#[ignore]` (large COVID model). -// Run with: cargo test --release -- --ignored corpus_sstats_multi_output_materializes +/// Runs by default -- a tenth of a second on a debug build, despite the large +/// COVID model, since it only imports. #[test] -#[ignore] fn corpus_sstats_multi_output_materializes() { let path = "../../test/metasd/covid19-us-homer/homer v8/Covid19US v8.mdl"; let contents = @@ -5834,11 +5835,12 @@ fn macro_attributable_classifier_separates_macro_from_nonmacro() { /// `CircularDependency`). It deliberately does NOT assert that all of /// C-LEARN compiles -- C-LEARN's non-macro blockers (#552, #553, #363, /// model-logic deps) remain out of scope -- only that no macro-specific -/// error from #554 fires. `#[ignore]` (C-LEARN is ~53k lines / 1.4 MB; -/// ~4s just to parse). -// Run with: cargo test --release -- --ignored corpus_clearn_macros_import +/// error from #554 fires. +/// +/// Runs by default. The "~4s just to parse C-LEARN" that justified `#[ignore]` +/// is no longer true -- the `engine-compile-perf` branch cut MDL import by a +/// third -- and an import regression guard is worth having in the default run. #[test] -#[ignore] fn corpus_clearn_macros_import() { let path = "../../test/xmutil_test_models/C-LEARN v77 for Vensim.mdl"; let contents = @@ -6023,17 +6025,15 @@ fn corpus_clearn_macros_import() { /// /// Also folded into this number: `-4`, PRE-EXISTING. Some earlier /// `conveyor-engine` commit already moved the count to 6,709 without updating -/// this pin. This gate is `#[ignore]`d, so neither pre-commit nor CI caught it -/// (tracked separately -- an `#[ignore]`d exact-value pin that nothing runs is -/// not a guardrail). +/// this pin. This gate USED to be `#[ignore]`d, so neither pre-commit nor CI +/// caught it -- an exact-value pin that nothing runs is not a guardrail. It now +/// runs by default; that is the fix for the problem this paragraph describes. /// /// Layout impact (the resource this gate protects -- #654's VM limit of 65,536 /// u16 result slots, NOT `wasmgen::lower`'s unrelated `MAX_UNROLL_UNITS`): the /// per-step result-row width goes 30,800 -> 31,198 slots (+398, +1.29%), still /// far below the ceiling. -// Run with: cargo test --release -- --ignored clearn_ltm_var_count_guardrail #[test] -#[ignore] fn clearn_ltm_var_count_guardrail() { use simlin_engine::db::{model_ltm_variables, set_project_ltm_enabled}; @@ -6110,7 +6110,11 @@ fn clearn_ltm_partials_all_parse() { ); } -// Run with: cargo test --release -- --ignored compiles_and_runs_clearn_structural +// Still `#[ignore]`d, but no longer for time: `simulates_clearn` now runs by +// default and checks C-LEARN's numbers against `Ref.vdf`, which subsumes +// "compiles and runs". Kept as the structural-only probe to reach for when the +// numeric gate fails and the question is whether compilation itself broke. +// Run with: cargo test -- --ignored compiles_and_runs_clearn_structural #[test] #[ignore] fn compiles_and_runs_clearn_structural() { From f1417afde10ac9ddbf398dbf49eb860b8f24ccd8 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Wed, 29 Jul 2026 10:09:51 -0700 Subject: [PATCH 10/10] engine: firewall the single-variable reconstruction so a link score is 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. --- src/simlin-engine/src/db/analysis.rs | 34 ++++++++++- src/simlin-engine/src/db/ltm/compile.rs | 32 +++++++++++ src/simlin-engine/src/db/ltm_tests.rs | 76 +++++++++++++++++++++++++ 3 files changed, 141 insertions(+), 1 deletion(-) diff --git a/src/simlin-engine/src/db/analysis.rs b/src/simlin-engine/src/db/analysis.rs index e74572db2..d232f2d8a 100644 --- a/src/simlin-engine/src/db/analysis.rs +++ b/src/simlin-engine/src/db/analysis.rs @@ -3434,15 +3434,47 @@ pub(crate) fn reconstruct_model_variables( /// to, which they could when each searched the model its own way. /// /// Returns None if the name doesn't match any variable in the model. +/// +/// A `&str`-taking wrapper over the tracked query below, so that no caller has +/// to own its name to ask. pub(super) fn reconstruct_single_variable( db: &dyn Db, model: SourceModel, project: SourceProject, var_name: &str, +) -> Option { + reconstruct_named_variable(db, model, project, var_name.to_string()) +} + +/// The salsa FIREWALL over [`reconstruct_model_variables`], and the reason +/// this is a tracked query rather than a plain map lookup. +/// +/// The map is whole-model: any variable's equation edit changes it, because it +/// holds every variable's LOWERED form. A caller that reads it directly +/// therefore depends on every variable in the model -- which for +/// `link_score_equation_text_shaped` (tracked per `(from, to, shape)`, and +/// documented as "recomputed only when the involved variables change") meant +/// one unrelated edit regenerated EVERY link score. On C-LEARN that is 6,721 +/// of them per keystroke. +/// +/// This query still reads the whole map and so 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 the same reason, as +/// `db::query::model_variable_by_name` over `SourceModel::variables`. +/// +/// Pinned by `db::ltm_tests::an_unrelated_equation_edit_does_not_regenerate_ +/// every_link_score`, which counts query-body entries -- pointer equality +/// cannot see this, since backdating leaves the memo in place either way. +#[salsa::tracked] +fn reconstruct_named_variable( + db: &dyn Db, + model: SourceModel, + project: SourceProject, + var_name: String, ) -> Option { reconstruct_model_variables(db, model, project) .get(&crate::common::Ident::::new( - var_name, + &var_name, )) .cloned() } diff --git a/src/simlin-engine/src/db/ltm/compile.rs b/src/simlin-engine/src/db/ltm/compile.rs index 72b9783ef..b0d5d6aa9 100644 --- a/src/simlin-engine/src/db/ltm/compile.rs +++ b/src/simlin-engine/src/db/ltm/compile.rs @@ -103,6 +103,35 @@ pub fn compile_ltm_var_fragment( compile_ltm_equation_fragment(db, &lsv.name, &equation, model, project) } +// How many times `link_score_equation_text_shaped`'s body has run on this +// thread (test-only). +// +// The query documents a per-involved-variable incrementality claim, and only a +// body-entry count can check it: salsa BACKDATES a re-executed query whose +// value compares equal, so the memo neither moves nor changes and pointer +// equality reads identical whether the body ran or not. Thread-local for the +// same reasons `db::stages`' counters are -- see the note there, including the +// warning about what happens if this subtree is ever parallelized. +#[cfg(test)] +thread_local! { + static SHAPED_LINK_SCORE_EXECUTIONS: std::cell::Cell = const { + std::cell::Cell::new(0) + }; +} + +/// Zero the counter (test-only). Call it after the fixture is synced and +/// first compiled, so setup work is not charged to the measured edit. +#[cfg(test)] +pub(crate) fn reset_shaped_link_score_executions() { + SHAPED_LINK_SCORE_EXECUTIONS.with(|c| c.set(0)); +} + +/// Read the counter (test-only). +#[cfg(test)] +pub(crate) fn shaped_link_score_executions() -> usize { + SHAPED_LINK_SCORE_EXECUTIONS.with(|c| c.get()) +} + /// Outcome of [`link_score_equation_text_shaped`] for one /// `(from, to, shape)` tuple. /// @@ -191,6 +220,9 @@ pub fn link_score_equation_text_shaped<'db>( use crate::db::LtmSyntheticVar; use crate::db::module_link_score_equation; + #[cfg(test)] + SHAPED_LINK_SCORE_EXECUTIONS.with(|c| c.set(c.get() + 1)); + let from_name = link_id.link_from(db); let to_name = link_id.link_to(db); let from_ident = Ident::::new(from_name); diff --git a/src/simlin-engine/src/db/ltm_tests.rs b/src/simlin-engine/src/db/ltm_tests.rs index 5a0485a13..897372a83 100644 --- a/src/simlin-engine/src/db/ltm_tests.rs +++ b/src/simlin-engine/src/db/ltm_tests.rs @@ -1357,3 +1357,79 @@ fn an_index_naming_the_axis_own_element_stays_a_static_selector() { "a static element selector must never be frozen; got: {arm}" ); } + +/// `link_score_equation_text_shaped` documents that "a per-shape link score is +/// recomputed only when the involved variables (and their shape-classifying +/// dimensions) change". This is that claim, measured. +/// +/// It has to be measured as a body-entry COUNT, not as memo identity: salsa +/// backdates a re-executed query whose value compares equal, so every link +/// score's memo looks untouched whether its body ran or not. The counter lives +/// in `db::ltm::compile` beside the query. +/// +/// The failure this pins is not hypothetical. Routing +/// `reconstruct_single_variable` straight through the whole-model +/// `reconstruct_model_variables` map -- which is what a naive "read the cached +/// map" refactor does -- makes every link score depend on every variable's +/// lowered form, so ONE unrelated equation edit regenerates all of them. On a +/// model with thousands of links that is a full LTM rebuild per keystroke. +#[test] +fn an_unrelated_equation_edit_does_not_regenerate_every_link_score() { + use crate::db::{ + compile_project_incremental, set_project_ltm_enabled, sync_from_datamodel_incremental, + }; + + // Two independent feedback loops sharing no variable, so an edit inside + // one provably cannot change the other's scores. `untouched_*` is the + // half the assertion is about. + let project = TestProject::new("ltm_link_score_incrementality") + .stock("edited_stock", "10", &["edited_in"], &[], None) + .flow("edited_in", "edited_stock * edited_rate", None) + .aux("edited_rate", "0.1", None) + .stock("untouched_stock", "10", &["untouched_in"], &[], None) + .flow("untouched_in", "untouched_stock * untouched_rate", None) + .aux("untouched_rate", "0.2", None) + .build_datamodel(); + + let mut db = SimlinDb::default(); + let state = sync_from_datamodel_incremental(&mut db, &project, None); + set_project_ltm_enabled(&mut db, state.project, true); + compile_project_incremental(&db, state.project, "main").expect("first compile"); + + // Count the whole cold build, so the edit's count has something to be a + // fraction OF -- an assertion against a bare number would pass just as + // happily on a model that emitted no link scores at all. + let cold = super::compile::shaped_link_score_executions(); + assert!( + cold >= 4, + "fixture must emit link scores for both loops to be measuring anything, got {cold}" + ); + + // Edit ONE variable's equation, in the other loop. + let mut edited = project.clone(); + for var in &mut edited.models[0].variables { + if let datamodel::Variable::Aux(aux) = var + && aux.ident == "edited_rate" + { + aux.equation = datamodel::Equation::Scalar("0.15".to_string()); + } + } + + super::compile::reset_shaped_link_score_executions(); + let state2 = sync_from_datamodel_incremental(&mut db, &edited, Some(&state)); + compile_project_incremental(&db, state2.project, "main").expect("recompile after edit"); + let after_edit = super::compile::shaped_link_score_executions(); + + // The bound is "strictly fewer than the cold build", not an exact number: + // which scores touch `edited_rate` is a property of the LTM emitter that + // may legitimately change, while "an edit in one loop must not regenerate + // the other loop's scores" is the contract. An exact pin would fail on + // every unrelated emitter change and teach nothing. + assert!( + after_edit < cold, + "a one-variable equation edit regenerated {after_edit} of {cold} link scores -- \ + the per-involved-variable incrementality this query documents is gone. \ + The usual cause is a dependency on a whole-model query (every variable's \ + lowered form) where a per-variable one would do." + ); +}