Repository navigation
Incremental compilation via salsa framework - #289
Conversation
|
@codex review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
==========================================
+ Coverage 77.06% 78.45% +1.38%
==========================================
Files 118 121 +3
Lines 28019 29656 +1637
==========================================
+ Hits 21593 23266 +1673
+ Misses 6426 6390 -36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3052447306
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (compiled, vm, vm_error) = if let Some(compiled) = cached { | ||
| match Vm::new(compiled.clone()) { | ||
| Ok(vm) => (Some(compiled), Some(vm), None), | ||
| Err(err) => (Some(compiled), None, Some(err)), |
There was a problem hiding this comment.
simlin_sim_new unconditionally consumes cached_compilation for any non-LTM model, but the cache is populated in gather_error_details by compiling only "main" (compile_simulation(project, "main")). In multi-model projects, calling sim_new for a non-main model after a successful patch can run with bytecode for the wrong model, yielding incorrect series/offset behavior instead of compiling the requested model. This is user-visible whenever patches are used and simulations are started for models other than main.
Useful? React with 👍 / 👎.
Code Review: Incremental compilation via salsa frameworkReviewed all 38 changed files (+7054/-2029). The SimFloat generic removal is mechanically correct throughout, and the salsa integration architecture is well thought out. Found three bugs in the new code. [P1] Unsorted
|
2359fd5 to
ddb49da
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddb49da329
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let canonical_var_name = canonicalize(dm_var.get_ident()).into_owned(); | ||
| let var_id = VariableId::new(db, canonical_var_name.clone()); | ||
|
|
||
| let source_var = source_variable_from_datamodel(db, dm_var); |
There was a problem hiding this comment.
sync_from_datamodel creates fresh SourceVariable inputs each time it is called, and patch application calls this sync on every committed edit, so the same project accumulates new input identities on every patch. Salsa memoization is keyed by input identity, so this pattern forces cache misses for previously-computed tracked functions and causes DB state to grow with each edit, which defeats the incremental-compilation goal and can degrade long-running editor sessions. Keep stable input handles and update fields via setters during resync.
Useful? React with 👍 / 👎.
| for source_var in source_model.variables(salsa_db).values() { | ||
| let _ = db::variable_direct_dependencies(salsa_db, *source_var, source_project); | ||
| } |
There was a problem hiding this comment.
set_dependencies_cached precomputes variable_direct_dependencies for every source variable but then ignores those results and still rebuilds dependencies via all_deps from lowered variables. That adds an extra full dependency-extraction pass on every compile without changing outputs, so the cached path does more work than the previous implementation unless another caller happens to hit those entries later. Either wire these cached deps into runlist construction or drop the priming loop.
Useful? React with 👍 / 👎.
Code ReviewFindings[P3] Tautological assertion in
|
Code ReviewReviewed all 38 changed files (+7098/-2035 lines) across three main workstreams: SimFloat generic removal, salsa incremental compilation infrastructure, and libsimlin FFI patch/error/simulation improvements. FindingsNo bugs found that meet the review criteria. The code is well-structured, systematically tested, and correctly implements the described functionality. ObservationsSalsa caching granularity within
Overall Correctness VerdictCorrect. All existing tests and the new test suites validate the changes. The SimFloat generic removal is mechanical and consistent. The symbolic bytecode roundtrip tests verify the symbolize→resolve pipeline. The salsa tracked functions are properly defined with appropriate caching boundaries. The libsimlin FFI changes maintain proper null-pointer validation, error handling, and thread safety through Mutex-guarded state. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e309d9412
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| // Set up salsa database for incremental per-variable caching. | ||
| let salsa_db = SimlinDb::default(); |
There was a problem hiding this comment.
Project::compile allocates a fresh SimlinDb each time, so every compilation starts from an empty salsa cache and all tracked per-variable queries are recomputed. That removes the intended incremental behavior across edits (the expensive parse/dependency work is redone for unchanged variables on every compile), which can keep edit-compile latency proportional to full model size instead of the changed subset.
Useful? React with 👍 / 👎.
| let cached = if !enable_ltm && model_ref.model_name.as_str() == "main" { | ||
| project_ref.cached_compilation.lock().unwrap().take() | ||
| } else { |
There was a problem hiding this comment.
simlin_sim_new consumes cached_compilation whenever the model is main, but this cache is not versioned against project mutations outside apply_patch. After any other mutating path (for example simlin_project_add_model, which rebuilds the project), the first simulation can run stale bytecode from the previous project state and return incorrect results before the cache is emptied.
Useful? React with 👍 / 👎.
Code ReviewFiles ReviewedAll 38 changed files, with particular focus on:
FindingsNo bugs found. Overall Correctness VerdictCorrect. The patch introduces the salsa infrastructure alongside the existing compilation pipeline without breaking any existing behavior. The SimFloat generic removal, salsa tracked functions, symbolic bytecode scaffolding, and libsimlin compilation caching are all implemented correctly. The var_names sorting fix in |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8020b8ac97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| use std::sync::OnceLock; | ||
| static PORTS: OnceLock<crate::ltm_augment::CompositePortMap> = OnceLock::new(); | ||
| PORTS.get_or_init(|| { | ||
| std::thread::spawn(|| { |
There was a problem hiding this comment.
get_stdlib_composite_ports() unconditionally uses std::thread::spawn, but this helper is called by LTM tracked queries (model_ltm_synthetic_variables / model_ltm_all_link_synthetic_variables), so LTM execution in the WASM engine can panic at runtime on wasm32 builds where thread spawning is unavailable. This makes LTM simulation/analysis fail in browser contexts even though other engine paths already provide wasm-specific fallbacks; add a cfg-gated non-threaded initialization path.
Useful? React with 👍 / 👎.
Code Review: PR #289Findings[P3] Silent
VerdictCorrect. The SimFloat generic removal is clean and mechanically consistent across all affected files. The new salsa database infrastructure is well-structured with thorough test coverage (~1400 lines of tests in |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31f0a34ae5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let source_project = SourceProject::new( | ||
| db, | ||
| project.name.clone(), | ||
| SourceSimSpecs::from(&project.sim_specs), | ||
| project |
There was a problem hiding this comment.
sync_from_datamodel allocates a brand-new SourceProject (and downstream SourceModel/SourceVariable) on every call, which means re-syncing the same long-lived SimlinDb keeps appending new input graphs rather than updating existing ones. In this commit, the libsimlin paths call re-sync repeatedly during patch/error flows, so unchanged models cannot benefit from stable tracked IDs and the database grows over time with stale inputs. Please keep and update prior input IDs (via setters) so repeated edits actually reuse cached query state.
Useful? React with 👍 / 👎.
| // Set up salsa database for incremental per-variable caching. | ||
| let salsa_db = SimlinDb::default(); | ||
| let sync_result = sync_from_datamodel(&salsa_db, &project_datamodel); |
There was a problem hiding this comment.
Project::base_from constructs a fresh SimlinDb for each rebuild and drops it at the end of the function, so memoized salsa results never survive from one project edit to the next. Since patch/compile workflows rebuild Project repeatedly, this defeats cross-edit incremental compilation and forces cold-cache recomputation after each change. Threading a persistent DB through rebuilds is needed for the intended incremental behavior.
Useful? React with 👍 / 👎.
Code Review: PR #289 — Incremental compilation via salsa frameworkSummaryThis is a large PR (+7103/-2035) implementing incremental compilation infrastructure using salsa. The changes break down into:
FindingsNo bugs introduced by this PR were identified. Specific areas verified:
VerdictOverall correctness: Correct. The patch is free of blocking issues. Existing tests pass, and the new code is well-covered by unit tests and integration tests. |
Review cycle summaryFive iterations of automated review from both claude[bot] and codex. Three fix commits were made addressing actionable feedback: Bugs fixed:
Follow-up issues filed:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be742798d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let project_locked = proj.project.lock().unwrap(); | ||
| let (all_errors, _) = gather_error_details(&project_locked); | ||
| let db_locked = proj.db.lock().unwrap(); | ||
| let sync = engine::db::sync_from_datamodel(&db_locked, &project_locked.datamodel); |
There was a problem hiding this comment.
simlin_project_get_errors now calls sync_from_datamodel on every invocation, which creates new salsa inputs each time (SourceProject::new/SourceModel::new/SourceVariable::new in src/simlin-engine/src/db.rs:1828-1874) instead of updating existing handles. In workflows that poll errors frequently (for example, editor validation on each edit), this causes the long-lived per-project DB to accumulate unreachable input nodes and steadily increases memory and query cost over time. This path should reuse proj.sync_state with sync_from_datamodel_incremental (or use an ephemeral DB) to avoid unbounded growth.
Useful? React with 👍 / 👎.
Code ReviewReviewed the full diff (~12K lines): SimFloat generic removal, salsa incremental compilation infrastructure, FFI compilation cache, and extensive new tests. Areas reviewed
Specific items verified
Overall correctness verdictCorrect. The patch removes the |
|
@codex review |
Code ReviewReviewed the full diff (~8800 additions) covering:
No blocking bugs found.Specific areas verified:
Overall correctness verdict: CorrectThe patch is well-structured, the new infrastructure has thorough unit tests (sync, incremental sync, pointer-stability, diagnostic collection, symbolic bytecode roundtrips), and the mechanical refactoring preserves existing semantics. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daf6bd42bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let salsa_db = SimlinDb::default(); | ||
| let sync_result = sync_from_datamodel(&salsa_db, &project_datamodel); |
There was a problem hiding this comment.
Project::from_with_model_cb creates a brand-new SimlinDb on every call, which drops all query caches before compilation starts. Because patch validation still recompiles via engine::Project::from(...) in src/libsimlin/src/patch.rs:440, editing a single equation forces a full reparse/recompile of unchanged variables instead of reusing prior Salsa results, which defeats the incremental-compilation behavior this change is meant to provide and can cause major latency regressions on large models.
Useful? React with 👍 / 👎.
| if !existing_keys.contains(&key) { | ||
| all_errors.push(ErrorDetailBuilder::from_formatted(formatted)); |
There was a problem hiding this comment.
The dedup key set is computed once from all_errors and never updated inside the loop, so multiple accumulator diagnostics with the same (model, variable, code) can all be appended even though this block claims to deduplicate by that triple. In cases where one variable emits repeated same-code diagnostics (for example, multiple spans with the same error code), callers can receive duplicate error entries from simlin_project_get_errors.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc7d91373e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let canonical_src = canonicalize(&mr.src).into_owned(); | ||
| edges.entry(canonical_src).or_default().insert(name.clone()); |
There was a problem hiding this comment.
model_causal_edges inserts module-input edges from canonicalize(&mr.src) without passing through normalize_module_ref_str. If a module input is fed by another module output (e.g. foo.bar, canonicalized to foo·bar), this creates an edge from a phantom module·output node instead of the module node, so cycle/loop analysis can miss real feedback paths and produce incorrect LTM synthetic-variable outputs for models that chain module outputs.
Useful? React with 👍 / 👎.
Add models field to SourceProject to store SourceModel handles alongside model names, enabling assemble_simulation to look up models by name. Implement assemble_simulation to produce valid CompiledSimulation objects by enumerating module instances, assembling each module, building specs, and computing flattened offsets. Fix concatenate_fragments to strip trailing Ret opcodes from per-variable fragments before concatenation, adding a single Ret at the end. Without this, the VM would stop executing flows after the first variable. Fix compute_layout to handle module variables by looking up the sub-model and using its total n_slots instead of variable_size. Populate dimension metadata in assemble_module from project dimensions. Fix canonical name mismatch: sync functions stored display names (with spaces) in variable_names but canonical names in variables map, causing XMILE-loaded models to produce empty bytecodes via the incremental path. compile_var_fragment also compared display names against canonical runlist entries, silently skipping all compilation phases. Split db.rs test module to db_tests.rs to stay under 6000-line threshold.
…_compilation The salsa DB is now the sole compilation cache. apply_project_patch_internal eagerly syncs the DB with the staged datamodel for accumulator diagnostics, restoring the previous state on rejection or dry-run. simlin_sim_new for non-LTM simulations tries compile_project_incremental first (a salsa cache hit when nothing changed since the last patch), falling back to the monolithic path when the incremental path does not yet support the model configuration. The explicit cached_compilation field and its consume-on-read protocol are no longer needed.
Decompose monolithic link score generation into per-link tracked functions so that equation edits only regenerate link scores for affected links. New tracked functions: - link_score_equation_text: computes a single link's score equation, keyed by an interned LtmLinkId. Only reads parse_source_variable for the specific from/to variables, so salsa skips recomputation when unrelated variables change. - module_ilink_equation_text: same pattern for stdlib module internal links. Refactored model_ltm_synthetic_variables, model_ltm_all_link_synthetic_variables, and module_ltm_synthetic_variables to iterate links and delegate to the per-link functions instead of bulk-generating all equations. Added reconstruct_single_variable helper for targeted variable lookup without rebuilding the full model variable map.
…lation AC1.3/AC1.4: verify compile_var_fragment produces identical symbolic bytecodes for existing variables when a new variable is added or removed, while compute_layout changes to reflect the new variable set. AC1.5: verify that changing a dimension definition only recompiles variables using that dimension (sales with ApplyToAll), while scalar variables (price) produce identical fragments. AC1.6: verify cross-model isolation -- changing module connections in model B does not invalidate model A's dependency graph. AC2.4: verify stdlib module_ltm_synthetic_variables returns a cached (pointer-equal) result on unchanged inputs. AC4.3 (strengthened): compile a model with stocks, flows, and lookups both incrementally and monolithically, run through the VM, and assert identical time-series output for every variable at every timestep. AC3.1: verify apply_patch + sim_new succeeds end-to-end through the shared salsa DB path and produces correct post-patch results. AC3.3: verify two sequential patches each produce correct simulation results, confirming incremental recomputation works across patches. AC3.4: verify snapshot isolation -- a sim created before a patch runs with pre-patch values while a post-patch sim sees the new values.
Criterion benchmarks comparing monolithic compile_project against incremental compile_project_incremental on a 100-variable chain model. Three scenarios: equation edit (v50), variable addition (v101), and variable removal (v100). On this machine, equation edit and variable removal show ~10x speedup; variable addition is ~1.3x faster since it changes the variable list and forces broader re-evaluation. Track two deferred items from the incremental compilation design: - Legacy error fields on Variable/ModelStage (design Step 13) - Dimension-granularity invalidation optimization (design Step 12) Neither blocks any acceptance criterion.
Five fixes from automated code review feedback: 1. Resource ID mismatch in assemble_module: flows and stocks phases were independently concatenated with resource IDs starting at 0, but the shared ByteCodeContext used all-phases numbering. Now each phase's concatenation receives context resource base offsets from preceding phases, ensuring GF/module/view/temp/dimlist IDs are consistent with the shared context. 2. Fragment compilation errors silently dropped: when compile_var_fragment returned None, the variable was silently skipped. Now assemble_module tracks missing fragments and returns an error listing all variables that failed to compile. 3. Model-specific sim specs not used: assemble_simulation always used project-level sim_specs, ignoring per-model overrides. Added model_sim_specs field to SourceModel, synced it in both fresh and incremental paths, and used it in assemble_simulation (matching the monolithic compile_project behavior). 4. Lock ordering inconsistency: apply_project_patch_internal's revert path acquired project.lock() while holding db.lock(), creating an AB/BA inversion with simlin_project_get_errors (which locks project then db). Fixed by cloning the datamodel before acquiring the db lock. 5. Silent truncation in renumber_opcode: temp_off (u32) and gf_off (u16) were truncated to u8 via bare `as` casts. Now renumber_opcode returns Result and checks bounds before casting.
Fix temp ID counting: from_fragments and db.rs per-initial renumbering used .max() across fragments that each start temp IDs at 0. Since concatenate_fragments renumbers sequentially, the correct count is the sum of each fragment's (max_id + 1), not the global max. Fix static view Temp(id) renumbering: concatenate_fragments copied SymbolicStaticView objects as-is, but Temp(id) bases still referred to fragment-local numbering. Now offsets Temp bases by temp_offset. Fix u8 checked arithmetic: renumber_opcode validated that offsets fit in u8 but didn't check that base + offset fits. Use checked_add for all u8 arithmetic to surface overflow as an error instead of silent wrapping. Fix variable_names nondeterminism: variable_names built from HashMap keys had nondeterministic order, causing spurious set_variable_names invalidation on every sync. Sort at all three construction sites.
When model_dependency_graph detects a circular dependency, set has_cycle flag on the result. assemble_module checks this flag and returns an error, preventing the incremental path from producing bytecode with invalid execution order for cyclic models. This ensures simlin_sim_new falls back to the monolithic path which properly rejects such models. Also sort variable_names after collecting from HashMap keys to ensure deterministic ordering and prevent spurious salsa cache invalidation.
The root-model flag in assemble_simulation compared raw name strings, which could fail when the model name preserves user casing (e.g. "Main" vs canonical "main"). Use canonicalize() for the comparison. Also fix calc_flattened_offsets_incremental to accept is_root as a parameter instead of hardcoding model_name == "main", ensuring correct implicit slot reservation for projects with non-"main" root models.
Use clone() instead of take() for sync_state during patch validation, so concurrent readers still see valid state. Previously, take() would clear the sync_state, leaving a window where concurrent sim_new or get_errors calls would see None and resync from scratch. Derive Clone on PersistentSyncState and its inner types to support this. Also simplify the revert path: since we no longer take the state, the None case is handled uniformly by passing prev_state.as_ref() to the resync call.
compile_var_fragment used filter_map with .ok() on symbolize_static_view and symbolize_module_decl, silently dropping entries that fail. Since bytecodes reference these by index, dropping an entry would shift all subsequent indices, causing wrong resource lookups. Use collect::<Result> to propagate errors consistently with the bytecode symbolization path.
apply_project_patch_internal was dropping the db lock after syncing staged state but before deciding whether to commit or rollback. During that window, concurrent simlin_sim_new or simlin_project_get_errors calls could observe patch effects for dry_run or rejected patches. The fix saves the original datamodel before patching (so rollback doesn't need to re-acquire the project lock, which would violate the project -> db -> sync_state lock ordering) and holds the db lock through the entire sync-evaluate-decide cycle. On commit, sync_state is also updated while holding the db lock so the incremental path (db + sync_state) is atomically consistent for concurrent readers.
c83e406 to
7e5b044
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e5b0441f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| })?; | ||
|
|
||
| let is_root = canonicalize(name.as_str()) == main_model_canonical; | ||
| let compiled = assemble_module(db, *source_model, project, is_root)?; |
There was a problem hiding this comment.
The assemble_simulation loop iterates each distinct inputs set for a module instance, but the compilation call ignores inputs and always builds the same module body (assemble_module(db, *source_model, project, is_root)). This means different instantiations of the same submodel are keyed differently in compiled_modules but contain identical bytecode, so models that rely on per-instantiation module inputs (e.g., isModuleInput-gated behavior) can simulate incorrectly.
Useful? React with 👍 / 👎.
| // incremental path (db + sync_state) is atomically consistent for | ||
| // concurrent readers. Then release db before updating project | ||
| // to maintain lock ordering (project -> db -> sync_state). | ||
| *project_ref.sync_state.lock().unwrap() = Some(staged_sync_state); |
There was a problem hiding this comment.
This publishes sync_state for the staged patch before updating project, then drops the DB lock and only afterward acquires the project lock to commit. A concurrent reader can therefore observe a mixed snapshot (old project + new DB/sync state), because simlin_project_get_errors reads project first and DB/sync second, which can merge diagnostics from different revisions and return inconsistent results.
Useful? React with 👍 / 👎.
Code ReviewThis is a large, well-structured PR introducing salsa-based incremental compilation infrastructure and a symbolic bytecode layer. After analyzing the new Findings[P3] Misleading doc comment on
|
Code ReviewThis is a large, well-structured PR that adds salsa-based incremental compilation and removes the FindingsNo blocking bugs found. [P3] Brief atomicity gap during patch commit — In Observations (non-blocking)
Overall Correctness VerdictCorrect. The patch should not break existing code or tests. The float de-genericization is mechanical and verified by passing all existing tests. The incremental compilation path produces identical results to the monolithic path for supported models, and gracefully falls back for unsupported ones. Lock ordering is consistently maintained across all FFI entry points with no deadlock risk. |
Code ReviewThis is a large, well-structured PR that adds salsa-based incremental compilation and removes the SimFloat generic parameter. I reviewed all 38 changed files, focusing on the new db.rs (3599 lines), symbolic.rs (2626 lines), the libsimlin integration (patch.rs, simulation.rs, project.rs), and the de-genericization of vm.rs, bytecode.rs, results.rs, float.rs, and the compiler modules. FindingsNo blocking bugs found. [P3] Brief atomicity gap during patch commit — src/libsimlin/src/patch.rs:521-525 In apply_project_patch_internal, the commit path updates sync_state (line 521) and then drops the db lock (line 522) before acquiring the project lock (line 524) to write the new project. During this window, a concurrent reader (e.g. simlin_sim_new or simlin_project_get_errors) could observe the new sync_state paired with the old project. In practice this window is extremely brief (microseconds) and the incremental path would produce correct results from the new sync_state while the monolithic fallback path would produce a consistent-but-stale result from the old project, so this is unlikely to cause issues. This is a consequence of the documented lock ordering constraint (project -> db -> sync_state) and the existing comments already explain the reasoning. Noting it for completeness. Observations (non-blocking)
Overall Correctness VerdictCorrect. The patch should not break existing code or tests. The float de-genericization is mechanical and verified by passing all existing tests. The incremental compilation path produces identical results to the monolithic path for supported models, and gracefully falls back for unsupported ones. Lock ordering is consistently maintained across all FFI entry points with no deadlock risk. |
The comment claimed the function does not depend on model-wide data, but it reads model.variables and model_dependency_graph. Salsa's backdating still ensures fragments are only invalidated when the values they actually read change, so the caching behavior is correct; just the documentation was wrong.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c62697d078
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
|
|
||
| // Stock phase: stocks get compiled with is_initial=false for updates | ||
| let stock_bytecodes = if is_stock && dep_graph.runlist_stocks.contains(&var_ident_str) { |
There was a problem hiding this comment.
model_dependency_graph puts module variables in runlist_stocks ("stocks and modules"), but compile_var_fragment only emits stock bytecode when is_stock is true, so module vars always produce None for the stock phase. In module-containing models this makes assemble_module treat valid module entries as missing fragments and fail incremental assembly, which forces the runtime to drop back to monolithic compilation for every sim creation instead of using the salsa cache.
Useful? React with 👍 / 👎.
| let table_results: Vec<crate::compiler::Table> = gf_tables | ||
| .iter() | ||
| .filter_map(|t| crate::compiler::Table::new(&var_ident, t).ok()) | ||
| .collect(); |
There was a problem hiding this comment.
This path silently discards Table::new errors via filter_map(... .ok()), which can compact the table list and shift table indices instead of rejecting the model. For arrayed lookup tables with one malformed element (e.g., mismatched x/y lengths), incremental compilation can succeed with reordered/missing tables, causing lookups to read the wrong table or return NaN at runtime, whereas the monolithic compiler would correctly fail with BadTable.
Useful? React with 👍 / 👎.
Code Review: Incremental compilation via salsa frameworkSummaryReviewed ~13,000 lines of additions across 42 files. This PR adds salsa-based incremental compilation alongside the existing monolithic pipeline, removes the Detailed AnalysisLock ordering (libsimlin): Verified that all paths acquire locks in a consistent order: SimFloat removal: Mechanical type substitution throughout ( Symbolic bytecode layer (symbolic.rs): The symbolize → resolve roundtrip is correct. Dependency graph (db.rs): Transitive dependency computation, cycle detection, and topological sorting follow the same logic as the monolithic path. The Incremental fallback: Models with sub-modules will fail in Layout consistency: Both FindingsNo bugs found that would break existing code, tests, or introduce regressions. The patch is well-structured with appropriate fallback mechanisms and comprehensive test coverage. Overall Correctness VerdictCorrect. The patch preserves all existing behavior (tests pass), introduces the incremental compilation as a safe optimization layer with graceful fallback, and the SimFloat removal is a clean mechanical change. No blocking issues identified. |
The incremental path used filter_map(.ok()) to silently drop Table::new failures. For a variable with a malformed graphical function (e.g. mismatched x/y point lengths), this would skip the bad table and potentially produce bytecode that references a nonexistent lookup index. The monolithic path correctly propagates these errors. Now returns None from compile_var_fragment on any table conversion failure, which causes the incremental path to fall back to the monolithic compiler for that model.
|
@codex review |
Code Review: Incremental compilation via salsa frameworkSummaryReviewed all 42 changed files (~13k additions, ~2k deletions). The PR introduces salsa-based incremental compilation, removes the Files reviewed
FindingsNo bugs found that meet the review criteria. Specific areas examined closely:
Observations (non-blocking)
Overall Correctness VerdictCorrect. The patch is free of bugs and blocking issues. Existing code and tests will not break. The design is well-layered with clear separation between symbolic bytecode (layout-independent) and concrete bytecode (layout-resolved), and the salsa integration follows established patterns. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfaa0c1ee4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(svar) = source_vars.get(var_name.as_str()) | ||
| && let Some(result) = compile_var_fragment(db, *svar, model, project, is_root) | ||
| && let Some(ref bc) = result.fragment.initial_bytecodes | ||
| { | ||
| initial_frags.push((var_name.clone(), bc)); |
There was a problem hiding this comment.
model_dependency_graph can include implicit DELAY/SMOOTH helper variables in runlist_initials/runlist_flows, but assembly only looks up fragment inputs in source_vars (explicit model variables). When an implicit name is encountered, this branch marks it missing and the module assembly fails, so compile_project_incremental rejects otherwise valid models that use builtin expansions and libsimlin falls back to monolithic compilation every time.
Useful? React with 👍 / 👎.
Review cycle summaryThree iterations of automated review (claude + codex) addressed all actionable feedback from the latest round of changes. Two fix commits were made: Correctness fixes:
Documentation fix:
|
The Initials runlist was a function of the per-process HashMap RandomState seed, not of the model: `model_dependency_graph_impl` materialized its candidate set (`init_list`) from a `HashSet` and handed it straight to `topo_sort_str`, which emits `names` in visit order and breaks ties -- variables with no ordering dependency between them, e.g. independent constants, stocks, and the lagged-input-stripped `PREVIOUS()`/`INITIAL()` helpers -- by exactly that order. Two compiles of the SAME model therefore produced different init orderings, and for any unordered pair that feeds the init value of a `PREVIOUS()`/`INITIAL()` variable, different (one possibly wrong) initial values. The Flows and Stocks runlists were already deterministic because they filter the pre-sorted `var_names`; only the initials phase leaked hash order. Fix: sort `init_list` before `topo_sort_str`, matching the flows/stocks contract. With a sorted `names` argument and BTreeSet-stored dependency edges, `topo_sort_str`'s entire traversal -- including how it breaks any residual ordering cycle -- is a deterministic function of the model. This is general (deterministic simulation for all models), not C-LEARN specific, and changes nothing about NA-arithmetic or tolerances. Pre-existing, not introduced by this branch: the `init_set.into_iter().collect()` pattern dates to the original salsa incremental-compilation work (PR #289), long before the C-LEARN residual branch. It only became observable now because C-LEARN's residual cells sit near the 1% cross-simulator tolerance, where a tiny order-induced floating-point delta flips a verdict. Empirically: across 12 fresh-DB compiles of C-LEARN the initials runlist had 12 distinct orderings before this change and 1 after (flows/stocks were 1 both times), and the live residual set is now byte-identical run to run. Relation to GH #595: this resolves the HashMap-iteration-order nondeterminism #595 identifies as the proximate mechanism (the arbitrary, hash-dependent init tie-break is now a deterministic function of the model). It does NOT close #595's deeper soundness gap -- the detection-vs-ordering edge-set inconsistency that lets an init ordering cycle through a synthetic-module stock output exist undetected; such a cycle is now ordered deterministically rather than reported. No in-repo model triggers that shape today. A regression test pins the property directly (no probability): `initials_runlist_is_deterministic_across_fresh_databases` asserts a byte-identical runlist across 32 freshly-seeded databases, and `initials_runlist_is_sorted_topological_order` asserts the order equals the stable (sorted-name tie-break) topological sort reconstructed from the engine's own `initial_dependencies`. Both fail if the sort is removed.
1 participant
Summary
This PR implements incremental compilation for simlin-engine using the salsa framework for demand-driven incremental computation. The core idea is to decompose each compilation stage into fine-grained, per-variable tracked functions whose results salsa automatically caches and selectively invalidates.
When a user edits one equation in a 100-variable model, only that variable's compilation stages re-execute; everything else is served from cache.
Changes by phase
Remove SimFloat generic (
d157184d) -- Hardcode f64 throughout, eliminating the generic parameter that complicates salsa integration. -1884 lines.Salsa database and interned identifiers (
e3d6955c) -- Add salsa dependency, SimlinDb, VariableId/ModelId interned types, SourceProject/SourceModel/SourceVariable input types, sync_from_datamodel function.Per-variable parsing and lowering (
aa37244a) --parse_source_variabletracked function memoizes per-variable parsing. Editing one variable's equation does NOT re-parse other variables.Dependency analysis (
15cc40e2) --variable_direct_dependenciesandmodel_dependency_graphtracked functions. Changinga + btoa * b(same deps) skips dependency graph recomputation via salsa's backdating.Symbolic bytecode and layout separation (
df05ce11) -- 1630-line symbolic bytecode module decouples per-variable compilation from global variable layout. Adding/removing variables doesn't invalidate cached bytecode fragments.LTM integration (
7d71c6db) -- Causal graph, loop detection, and score equation generation as tracked functions. Equation edits with unchanged deps skip loop redetection.Shared compilation in libsimlin (
9057efea) -- apply_patch caches CompiledSimulation; sim_new reuses it instead of recompiling. Eliminates double compilation across FFI calls.Error accumulator migration (
30524473) -- CompilationDiagnostic salsa accumulator collects parse/compilation errors as a side channel, parallel to existing struct fields for backward compatibility.Key design decisions
model_dependency_graphreads dependency sets, not equation text. When deps don't change, downstream graph computation is skipped entirely.Stats
Test plan
tests/simulate*.rs)tests/simulate_ltm.rs)cargo test -p simlin-engine)Implements the design from doc/design/2026-02-21-incremental-compilation.md.