feat(core): compile board graph topology on first execution#572
feat(core): compile board graph topology on first execution#572SBALAVIGNESH123 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a caching layer for board graph topologies to improve execution performance by skipping expensive graph construction phases. A new CompiledBoard struct handles the compilation and hashing of board structures, while InternalRun is updated with a fast-path initialization method. However, the review highlights a critical concurrency issue where mutable runtime state, such as pin values and execution counters, is shared across concurrent runs using the same cached topology. There is also significant code duplication between the new caching logic and existing initialization paths that should be refactored to ensure maintainability. Other feedback includes replacing println! with the tracing crate for diagnostics and removing an unused variable in the compilation logic.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 14 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
8513a3d to
0c1917f
Compare
|
Thanks for the thorough review! All valid points — I've pushed a v2 that addresses everything: Concurrency bug (critical) — You're absolutely right, sharing Arc across concurrent runs would corrupt pin values. Fixed by restructuring CompiledBoard to only cache the stateless Arc instances (which are Send + Sync by trait contract and take &self in run()). Each execution now creates its own fresh pins, nodes, and wiring — no mutable state is shared between runs. Code duplication — Removed the standalone compile() method entirely. from_compiled() now follows the same phase structure as new_with_run_id() (pin creation → wiring → node instantiation → dependency resolution), just swapping registry.instantiate() for a cached logic lookup. Happy to extract shared helpers in a follow-up if you'd prefer less duplication, but wanted to keep this PR's diff minimal against the existing code. Unused variable & println — Removed pin_to_node from the old compile path, replaced println! with tracing::info!. The main perf win is skipping registry.instantiate() on subsequent runs — especially useful for WASM nodes where that can involve module loading. Graph wiring still runs fresh each time, which is the right tradeoff for correctness. |
) Cache stateless NodeLogic instances from the expensive registry.instantiate() call so subsequent executions of the same board version skip it entirely. Design: only Arc<dyn NodeLogic> (stateless, Send+Sync) is cached. Each run creates its own fresh InternalPins, InternalNodes, variables, and cache to guarantee full isolation between concurrent executions. Key changes: - Add CompiledBoard struct that stores cached NodeLogic instances with content-hash-based invalidation via HighwayHash - Add InternalRun::from_compiled() fast path that creates fresh graph topology per run but reuses cached NodeLogic (skips registry lookups) - Add compiled_boards cache (DashMap) to FlowLikeState - First execution compiles and caches NodeLogic; subsequent runs hit fast path - Zero changes to node APIs -- completely transparent to all catalog nodes Concurrency safety: - No mutable runtime state (pin values, exec counters) is shared between runs - Each InternalRun gets its own InternalPin, InternalNode, Run, variables - NodeLogic::run() takes &self (not &mut self) -- stateless by contract Closes Rheosoph#71
0c1917f to
f459cda
Compare
|
Thank you very much for the PR! I will review it later :) |
Summary
Resolves #71 — Graphs should be compiled on first execution or version creation to speed-up future executions and loading times.
Problem
Every call to
InternalRun::new_with_run_id()performs ~370 lines of O(N²+E) graph construction:All of this is deterministic for a given board version and repeated identically every execution.
Solution
Cache the compiled graph topology (pins, nodes, dependencies) on first execution, keyed by a content hash (HighwayHash) of the board. Subsequent runs reuse the cached topology and only reset mutable runtime state.
Key design insight:
InternalPinusesOnceLockfor graph wiring (connected_to,depends_on,node), making the topology immutable after construction. Onlypin.value: RwLock<Option<Value>>is mutable — andpin.reset()already clears it.Changes
compiled.rsCompiledBoardstruct withcompile(),reset_for_new_run(),compute_board_hash(),is_valid_for()execution.rsnew_with_run_id()+from_compiled()fast path + cache store after slow pathstate.rscompiled_boards: DashMapfield onFlowLikeStateFast Path Performance
Run,variables,cachefor full isolationSafety
new_with_run_id()preserved as fallback (slow path)