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/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/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/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..dcdde85d7 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 = @@ -1529,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), @@ -1576,8 +1572,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..392bf0f96 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) } @@ -232,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); @@ -246,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), } }) @@ -272,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/common.rs b/src/simlin-engine/src/common.rs index 3e963c78a..a1402a42d 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,117 @@ 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`. +/// +/// 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) { @@ -932,7 +1042,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 +1071,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 +1549,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 +1580,90 @@ 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); + } + } + + /// 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] + 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] @@ -2667,6 +2947,56 @@ 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 +// 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/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 8e31bf843..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>, } @@ -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 { @@ -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/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) diff --git a/src/simlin-engine/src/db/analysis.rs b/src/simlin-engine/src/db/analysis.rs index 4fab5a244..d232f2d8a 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,18 +3349,28 @@ 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); 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 +3394,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,83 +3407,76 @@ 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); - 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) + }); } } - variables + std::sync::Arc::new(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. +/// +/// 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 { - use crate::common::{Canonical, Ident}; - - 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); - 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, dims, &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, dims, &scope, implicit_dm_var); - return Some(lowered_imp); - } - } - } + reconstruct_named_variable(db, model, project, var_name.to_string()) +} - None +/// 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, + )) + .cloned() } /// Reconstruct an implicit (compiler-generated) variable from its datamodel form. @@ -3483,7 +3488,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 +3518,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, @@ -3528,7 +3532,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 @@ -3538,7 +3541,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)) @@ -4832,7 +4835,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. @@ -4841,7 +4843,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 6ca3a3185..2039c7872 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, @@ -605,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 { @@ -878,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() { @@ -940,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/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..b0d5d6aa9 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}; @@ -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); @@ -785,7 +817,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 +831,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 +885,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 +896,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), ); @@ -899,8 +930,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 { @@ -1526,10 +1559,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 @@ -1546,20 +1579,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 @@ -1968,9 +2003,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 +2012,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, @@ -2049,8 +2083,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 { @@ -2426,10 +2462,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 @@ -2467,23 +2503,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/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/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/db/ltm_tests.rs b/src/simlin-engine/src/db/ltm_tests.rs index cd5dfcc8c..897372a83 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()); @@ -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." + ); +} 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..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). @@ -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 @@ -603,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/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/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 94aeb6680..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. @@ -39,8 +38,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 } } @@ -82,7 +85,7 @@ impl SubdimensionRelation { #[derive(Default)] struct RelationshipCache { cache: Mutex< - HashMap<(CanonicalDimensionName, CanonicalDimensionName), Option>, + IdentMap<(CanonicalDimensionName, CanonicalDimensionName), Option>, >, } @@ -252,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 @@ -306,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, } @@ -380,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| { @@ -414,17 +417,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/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(), } } diff --git a/src/simlin-engine/src/ltm_augment_tests.rs b/src/simlin-engine/src/ltm_augment_tests.rs index 737eb0551..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 @@ -3297,7 +3296,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 +3354,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 +3547,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 +3957,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/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) diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index 133eb6b1c..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); @@ -578,12 +574,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 +601,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 +1070,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..bdd37d46c 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, ); @@ -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`. @@ -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())) }); 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() {