Skip to content

Commit ba9df67

Browse files
Your Nameclaude
andcommitted
refactor(pipeline): PR#7 slice 8 -- extract cache.rs (issue #67 hotspot split)
Move-only extraction of the formal-resolver cache and resolution-maps cache from pipeline.rs into the new pipeline/cache.rs: cached_formal_resolver, cached_resolution_maps, invalidate_resolution_maps_cache, is_manifest_path (plus their backing FORMAL_RESOLVER/RESOLUTION_MAPS_CACHE statics, RESOLUTION_MAPS_TTL const, and the private CachedResolutionMaps struct). ResolutionMaps itself stays in pipeline.rs (already pub, shared with resolve_import_targets/rebuild_graph/incremental_graph_update). Visibility: the 4 moved functions are pub(super) in cache.rs -- driver.rs and graph.rs are siblings of cache.rs, not descendants, so plain private wouldn't reach them. pipeline.rs pulls them back in via a plain `use cache::{...}` (non-pub), which both driver.rs and graph.rs already consume via their existing `use super::{...}` blocks -- same "child sees ancestor's use imports regardless of original visibility" pattern used since slice 4, so neither sibling file's imports needed to change. Verified via callers() before the move: real callers are driver.rs (reindex_all_cancellable_with_phase/reindex_changed_cancellable/ reindex_paths) and graph.rs::rebuild_graph_from_index, plus 2 in-pipeline.rs test calls to cached_resolution_maps (resolve automatically via the test module's own `use super::*;`). Also fixes a real slice-7 regression found while re-reading pipeline.rs's raw bytes before this move (same discipline that caught slice 7's own two boundary bugs): reindex_changed_cancellable's doc comment had been left orphaned in pipeline.rs instead of moving with the function in slice 7 -- it silently merged (no blank-line separator) into what was this cache's own doc comment, since nothing sat between them. Restored onto driver.rs::reindex_changed_cancellable verbatim in this same commit, and driver.rs's own module doc comment updated to reflect slice 8 landing (only slice 9's needs_call_site_identity_baseline/ rebuild_call_site_identity_baseline remain as pipeline.rs-private). Zero net logic diff: edit_lines zero-expected_hash PREVIEW confirmed the deleted range byte-for-byte before writing cache.rs; a first preview attempt undershot by one line (missed invalidate_resolution_maps_cache's own closing brace) and a first edit attempt over-deleted ResolutionMaps without reinstating it -- both caught by the tool's own gates (EDIT_CONTEXT_REQUIRED) before anything wrong was written to disk. Verified: cargo build -p calm-core clean (one unused PathBuf import fixed), cargo clippy -p calm-core --all-targets 100% clean, cargo test -p calm-core 1247 passed/0 failed/12 ignored including both golden_equivalence_{continued,incremental}_vs_fresh_across_mutation_rounds, cargo fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent f41700e commit ba9df67

3 files changed

Lines changed: 213 additions & 146 deletions

File tree

crates/calm-core/src/indexer/pipeline.rs

Lines changed: 22 additions & 143 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use rusqlite::Connection;
22
use std::collections::{HashMap, HashSet};
3-
use std::path::{Path, PathBuf};
3+
use std::path::Path;
44

55
use crate::indexer::chunker::CodeChunk;
66
use crate::indexer::parser::ParsedSymbol;
@@ -361,38 +361,27 @@ struct ResolutionCtx<'a> {
361361
inheritance_closure: HashMap<String, Vec<Vec<String>>>,
362362
}
363363

364-
/// Same as `reindex_changed`, but checked against `cancel` between parse
365-
/// batches — see `run_indexing_pipeline_cancellable`'s doc comment for why
366-
/// this matters on the shutdown path (a large changed-file set, e.g. a git
367-
/// branch switch, can take long enough to matter even inside the already
368-
/// per-event-cancellable watch loop). Bailing mid-loop drops `tx` without
369-
/// committing — same rollback guarantee as the full-index cancellable path.
370-
/// Process-wide cache for the stack-graph rule sets `FormalResolver` loads
371-
/// (`load_python`/`load_typescript`/`load_javascript`/`load_java`) — these
372-
/// compile `.tsg` rule files via tree-sitter at construction time, which
373-
/// measured live (this repo's own daemon, release build) is the single
374-
/// most expensive step in every reindex call: ~5s, dwarfing the O(repo)
375-
/// file-walk that Plan 3 §3.1 Phase A's `reindex_paths` removes — found
376-
/// while dogfooding Phase A's own latency win and confirming it barely
377-
/// moved end-to-end (see the plan doc's acceptance table). The rule sets
378-
/// never change during a process's lifetime (nothing reconfigures them),
379-
/// and `FormalResolver::resolve_file` takes `&self` only — read-only after
380-
/// construction — so one shared instance, built once and reused by every
381-
/// reindex call for the rest of the process's life, is both safe and the
382-
/// actual dominant win here, bigger than Phase A's own file-walk removal.
383-
static FORMAL_RESOLVER: std::sync::OnceLock<crate::resolver::formal::FormalResolver> =
384-
std::sync::OnceLock::new();
385-
386-
fn cached_formal_resolver() -> &'static crate::resolver::formal::FormalResolver {
387-
FORMAL_RESOLVER.get_or_init(|| {
388-
let mut formal = crate::resolver::formal::FormalResolver::new();
389-
let _ = formal.load_python();
390-
let _ = formal.load_typescript();
391-
let _ = formal.load_javascript();
392-
let _ = formal.load_java();
393-
formal
394-
})
395-
}
364+
// PR#7 slice 8: move-only extraction of the formal-resolver cache and the
365+
// resolution-maps cache (shared FormalResolver instance, per-project_root
366+
// ResolutionMaps TTL/manifest-mtime cache, manifest-path predicate, force-
367+
// evict helper) into pipeline/cache.rs (issue #67 hotspot split).
368+
// ResolutionMaps itself stays in pipeline.rs (already pub, shared with
369+
// resolve_import_targets/rebuild_graph/incremental_graph_update).
370+
// cached_formal_resolver/cached_resolution_maps/
371+
// invalidate_resolution_maps_cache/is_manifest_path are pub(super) in
372+
// cache.rs (not plain private, unlike slices 1-6) since driver.rs and
373+
// graph.rs -- siblings of cache.rs, not descendants -- both call into them;
374+
// verified via callers() before the move. Also fixes a slice-7 regression
375+
// found while researching this slice: reindex_changed_cancellable's doc
376+
// comment had been orphaned in pipeline.rs (left behind, silently merged
377+
// into this comment block) instead of moving to driver.rs with the
378+
// function itself -- restored onto driver.rs::reindex_changed_cancellable
379+
// in the same commit.
380+
mod cache;
381+
use cache::{
382+
cached_formal_resolver, cached_resolution_maps, invalidate_resolution_maps_cache,
383+
is_manifest_path,
384+
};
396385

397386
/// Bundle of the 6 per-project-root, ecosystem-specific import/module
398387
/// resolvers that `resolve_module_to_path` (and everything upstream of it —
@@ -414,116 +403,6 @@ pub struct ResolutionMaps {
414403
go: crate::indexer::go_module::GoModule,
415404
}
416405

417-
/// Cache entry for `cached_resolution_maps` — one per `project_root` (a
418-
/// single-slot cache would return the wrong project's maps whenever more
419-
/// than one `project_root` is used within the same process, which the test
420-
/// suite does constantly via per-test temp dirs).
421-
struct CachedResolutionMaps {
422-
built_at: std::time::Instant,
423-
cargo_toml_mtime: Option<std::time::SystemTime>,
424-
cargo_lock_mtime: Option<std::time::SystemTime>,
425-
composer_json_mtime: Option<std::time::SystemTime>,
426-
maps: ResolutionMaps,
427-
}
428-
429-
static RESOLUTION_MAPS_CACHE: std::sync::OnceLock<
430-
std::sync::Mutex<HashMap<PathBuf, CachedResolutionMaps>>,
431-
> = std::sync::OnceLock::new();
432-
433-
/// Fallback for the part `CrateMap`/`Psr4Map` genuinely can't cover by
434-
/// mtime alone: neither `NamespaceMap::build` nor `PySysPathMap::build` is
435-
/// manifest-driven at all — they walk every `.cs` / `.py` file in the repo
436-
/// and read each one's content (see their own doc comments) — so there is no
437-
/// single file whose mtime tracks "did those maps change". A pure TTL is the
438-
/// honest answer here, not a gap: any edit to a `.cs`/`.py` file is already
439-
/// at most this old before the next reindex sees a corrected map.
440-
const RESOLUTION_MAPS_TTL: std::time::Duration = std::time::Duration::from_secs(60);
441-
442-
/// Plan 3 §3.1 Phase D: `CrateMap`/`Psr4Map`/`NamespaceMap` were each
443-
/// rebuilt from scratch on every single reindex call (3 call sites) —
444-
/// `CrateMap::build` alone spawns a `cargo metadata` subprocess when
445-
/// `cargo` is available. Cached per-`project_root`, invalidated on either
446-
/// `Cargo.toml`/`Cargo.lock`/`composer.json`'s mtime changing (covers
447-
/// `CrateMap`/`Psr4Map`, whose real inputs — verified by reading
448-
/// `from_cargo_metadata`/`from_toml_scan`/`from_composer_json` — are
449-
/// exactly these files, not the `*.csproj` the plan doc originally
450-
/// guessed) or `RESOLUTION_MAPS_TTL` elapsing (the only correct answer for
451-
/// `NamespaceMap`, see its doc comment above). All three maps are cheap to
452-
/// `Clone` (small `HashMap`/`Vec` of strings) — cloned out of the lock
453-
/// rather than holding it for the caller's `rebuild_graph` pass.
454-
fn cached_resolution_maps(project_root: &Path) -> ResolutionMaps {
455-
let file_mtime = |name: &str| {
456-
std::fs::metadata(project_root.join(name))
457-
.and_then(|m| m.modified())
458-
.ok()
459-
};
460-
let cargo_toml_mtime = file_mtime("Cargo.toml");
461-
let cargo_lock_mtime = file_mtime("Cargo.lock");
462-
let composer_json_mtime = file_mtime("composer.json");
463-
464-
let cache_lock = RESOLUTION_MAPS_CACHE.get_or_init(|| std::sync::Mutex::new(HashMap::new()));
465-
let mut cache = cache_lock
466-
.lock()
467-
.unwrap_or_else(std::sync::PoisonError::into_inner);
468-
if let Some(c) = cache.get(project_root) {
469-
let fresh_enough = c.built_at.elapsed() < RESOLUTION_MAPS_TTL;
470-
let manifests_unchanged = c.cargo_toml_mtime == cargo_toml_mtime
471-
&& c.cargo_lock_mtime == cargo_lock_mtime
472-
&& c.composer_json_mtime == composer_json_mtime;
473-
if fresh_enough && manifests_unchanged {
474-
return c.maps.clone();
475-
}
476-
}
477-
478-
let maps = ResolutionMaps {
479-
crate_map: crate::indexer::crate_map::CrateMap::build(project_root),
480-
psr4: crate::indexer::psr4::Psr4Map::build(project_root),
481-
namespace_map: crate::indexer::csharp_namespace::NamespaceMap::build(project_root),
482-
pysys: crate::indexer::pysyspath::PySysPathMap::build(project_root),
483-
jvm: crate::indexer::jvm_package::JvmPackageMap::build(project_root),
484-
go: crate::indexer::go_module::GoModule::build(project_root),
485-
};
486-
cache.insert(
487-
project_root.to_path_buf(),
488-
CachedResolutionMaps {
489-
built_at: std::time::Instant::now(),
490-
cargo_toml_mtime,
491-
cargo_lock_mtime,
492-
composer_json_mtime,
493-
maps: maps.clone(),
494-
},
495-
);
496-
maps
497-
}
498-
499-
/// Phase B plan T4b: the 3 manifest filenames `cached_resolution_maps`
500-
/// tracks mtimes for (see its doc comment) — a standalone predicate so both
501-
/// `reindex_paths` and `reindex_changed_cancellable` check the same thing.
502-
/// Root-relative exact match only, matching that function's own
503-
/// `project_root.join(name)` checks — a nested workspace member's manifest
504-
/// doesn't affect this cache.
505-
fn is_manifest_path(rel: &str) -> bool {
506-
matches!(rel, "Cargo.toml" | "Cargo.lock" | "composer.json")
507-
}
508-
509-
/// Phase B plan T4b (Risk Abductive-1 mitigation): force-evict
510-
/// `project_root`'s `cached_resolution_maps` entry. Belt-and-suspenders on
511-
/// top of that function's own mtime comparison, which is correct only to
512-
/// the filesystem's mtime resolution — some filesystems round to 1s, so two
513-
/// edits to the same manifest inside one second would otherwise look
514-
/// "unchanged" and keep serving the first edit's stale maps. Called only
515-
/// when this pass's own `changed_paths` already proves a manifest was
516-
/// touched (a hard fact, not a heuristic), so an unconditional evict here is
517-
/// free insurance rather than a guess. A no-op if nothing has been cached
518-
/// yet for this `project_root`.
519-
fn invalidate_resolution_maps_cache(project_root: &Path) {
520-
if let Some(lock) = RESOLUTION_MAPS_CACHE.get() {
521-
lock.lock()
522-
.unwrap_or_else(std::sync::PoisonError::into_inner)
523-
.remove(project_root);
524-
}
525-
}
526-
527406
/// Whether an existing database still contains CallSites whose line-only
528407
/// identity predates D4. Incremental indexing cannot repair these rows because
529408
/// their file hashes are unchanged, so it must take the full transactional
Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
//! PR#7 (docs/plans/2026-08-19-evidence-architecture-execution-plan.md Part E,
2+
//! Wave 1 slice 8): behavior-preserving extraction from `pipeline.rs` (issue
3+
//! #67 hotspot). The formal-resolver cache (one process-wide `FormalResolver`
4+
//! instance, expensive to build) and the resolution-maps cache (per-
5+
//! `project_root` `ResolutionMaps`, TTL + manifest-mtime invalidated), plus
6+
//! the manifest-path predicate and the force-evict helper both caches share.
7+
//! Move-only -- no logic changed, only relocated.
8+
//!
9+
//! `ResolutionMaps` itself stays defined in `pipeline.rs` (not moved,
10+
//! already `pub`, shared with `resolve_import_targets`/`rebuild_graph`/
11+
//! `incremental_graph_update`) -- pulled in via `super::ResolutionMaps`.
12+
//!
13+
//! Sibling-module wrinkle (not present in slices 1-6, first predicted in the
14+
//! slice-7 handoff doc, confirmed live here): `driver.rs` and `graph.rs` are
15+
//! both SIBLINGS of this new `cache` module, not ancestors -- Rust privacy
16+
//! only grants ancestor/descendant visibility (a `pub(in path)` item is
17+
//! visible in `path` and `path`'s descendants), so `cached_formal_resolver`/
18+
//! `cached_resolution_maps`/`invalidate_resolution_maps_cache`/
19+
//! `is_manifest_path` are `pub(super)` here -- `super` from `cache`'s own
20+
//! perspective is `pipeline`, which both `driver` and `graph` descend from,
21+
//! so `pub(super)` is exactly sufficient. Both sibling files' `use
22+
//! super::{...}` import blocks were updated to pull these four names from
23+
//! `super::cache::{...}` instead. Verified via `callers()` before the move:
24+
//! real callers are `driver.rs` (`reindex_all_cancellable_with_phase`/
25+
//! `reindex_changed_cancellable`/`reindex_paths`) and
26+
//! `graph.rs::rebuild_graph_from_index`, plus 2 in-`pipeline.rs` test calls
27+
//! to `cached_resolution_maps` (resolve automatically via the test module's
28+
//! own `use super::*;`, same as every prior slice).
29+
//!
30+
//! Also fixes a slice-7 regression found while researching this slice:
31+
//! `reindex_changed_cancellable`'s doc comment had been left behind in
32+
//! `pipeline.rs` instead of moving with the function -- it silently merged
33+
//! (no blank-line separator) into what was this cache's own doc comment,
34+
//! since nothing was between them. Restored onto
35+
//! `driver.rs::reindex_changed_cancellable` in the same commit as this move.
36+
37+
use std::collections::HashMap;
38+
use std::path::{Path, PathBuf};
39+
40+
use super::ResolutionMaps;
41+
42+
/// Process-wide cache for the stack-graph rule sets `FormalResolver` loads
43+
/// (`load_python`/`load_typescript`/`load_javascript`/`load_java`) — these
44+
/// compile `.tsg` rule files via tree-sitter at construction time, which
45+
/// measured live (this repo's own daemon, release build) is the single
46+
/// most expensive step in every reindex call: ~5s, dwarfing the O(repo)
47+
/// file-walk that Plan 3 §3.1 Phase A's `reindex_paths` removes — found
48+
/// while dogfooding Phase A's own latency win and confirming it barely
49+
/// moved end-to-end (see the plan doc's acceptance table). The rule sets
50+
/// never change during a process's lifetime (nothing reconfigures them),
51+
/// and `FormalResolver::resolve_file` takes `&self` only — read-only after
52+
/// construction — so one shared instance, built once and reused by every
53+
/// reindex call for the rest of the process's life, is both safe and the
54+
/// actual dominant win here, bigger than Phase A's own file-walk removal.
55+
static FORMAL_RESOLVER: std::sync::OnceLock<crate::resolver::formal::FormalResolver> =
56+
std::sync::OnceLock::new();
57+
58+
pub(super) fn cached_formal_resolver() -> &'static crate::resolver::formal::FormalResolver {
59+
FORMAL_RESOLVER.get_or_init(|| {
60+
let mut formal = crate::resolver::formal::FormalResolver::new();
61+
let _ = formal.load_python();
62+
let _ = formal.load_typescript();
63+
let _ = formal.load_javascript();
64+
let _ = formal.load_java();
65+
formal
66+
})
67+
}
68+
69+
/// Cache entry for `cached_resolution_maps` — one per `project_root` (a
70+
/// single-slot cache would return the wrong project's maps whenever more
71+
/// than one `project_root` is used within the same process, which the test
72+
/// suite does constantly via per-test temp dirs).
73+
struct CachedResolutionMaps {
74+
built_at: std::time::Instant,
75+
cargo_toml_mtime: Option<std::time::SystemTime>,
76+
cargo_lock_mtime: Option<std::time::SystemTime>,
77+
composer_json_mtime: Option<std::time::SystemTime>,
78+
maps: ResolutionMaps,
79+
}
80+
81+
static RESOLUTION_MAPS_CACHE: std::sync::OnceLock<
82+
std::sync::Mutex<HashMap<PathBuf, CachedResolutionMaps>>,
83+
> = std::sync::OnceLock::new();
84+
85+
/// Fallback for the part `CrateMap`/`Psr4Map` genuinely can't cover by
86+
/// mtime alone: neither `NamespaceMap::build` nor `PySysPathMap::build` is
87+
/// manifest-driven at all — they walk every `.cs` / `.py` file in the repo
88+
/// and read each one's content (see their own doc comments) — so there is no
89+
/// single file whose mtime tracks "did those maps change". A pure TTL is the
90+
/// honest answer here, not a gap: any edit to a `.cs`/`.py` file is already
91+
/// at most this old before the next reindex sees a corrected map.
92+
const RESOLUTION_MAPS_TTL: std::time::Duration = std::time::Duration::from_secs(60);
93+
94+
/// Plan 3 §3.1 Phase D: `CrateMap`/`Psr4Map`/`NamespaceMap` were each
95+
/// rebuilt from scratch on every single reindex call (3 call sites) —
96+
/// `CrateMap::build` alone spawns a `cargo metadata` subprocess when
97+
/// `cargo` is available. Cached per-`project_root`, invalidated on either
98+
/// `Cargo.toml`/`Cargo.lock`/`composer.json`'s mtime changing (covers
99+
/// `CrateMap`/`Psr4Map`, whose real inputs — verified by reading
100+
/// `from_cargo_metadata`/`from_toml_scan`/`from_composer_json` — are
101+
/// exactly these files, not the `*.csproj` the plan doc originally
102+
/// guessed) or `RESOLUTION_MAPS_TTL` elapsing (the only correct answer for
103+
/// `NamespaceMap`, see its doc comment above). All three maps are cheap to
104+
/// `Clone` (small `HashMap`/`Vec` of strings) — cloned out of the lock
105+
/// rather than holding it for the caller's `rebuild_graph` pass.
106+
pub(super) fn cached_resolution_maps(project_root: &Path) -> ResolutionMaps {
107+
let file_mtime = |name: &str| {
108+
std::fs::metadata(project_root.join(name))
109+
.and_then(|m| m.modified())
110+
.ok()
111+
};
112+
let cargo_toml_mtime = file_mtime("Cargo.toml");
113+
let cargo_lock_mtime = file_mtime("Cargo.lock");
114+
let composer_json_mtime = file_mtime("composer.json");
115+
116+
let cache_lock = RESOLUTION_MAPS_CACHE.get_or_init(|| std::sync::Mutex::new(HashMap::new()));
117+
let mut cache = cache_lock
118+
.lock()
119+
.unwrap_or_else(std::sync::PoisonError::into_inner);
120+
if let Some(c) = cache.get(project_root) {
121+
let fresh_enough = c.built_at.elapsed() < RESOLUTION_MAPS_TTL;
122+
let manifests_unchanged = c.cargo_toml_mtime == cargo_toml_mtime
123+
&& c.cargo_lock_mtime == cargo_lock_mtime
124+
&& c.composer_json_mtime == composer_json_mtime;
125+
if fresh_enough && manifests_unchanged {
126+
return c.maps.clone();
127+
}
128+
}
129+
130+
let maps = ResolutionMaps {
131+
crate_map: crate::indexer::crate_map::CrateMap::build(project_root),
132+
psr4: crate::indexer::psr4::Psr4Map::build(project_root),
133+
namespace_map: crate::indexer::csharp_namespace::NamespaceMap::build(project_root),
134+
pysys: crate::indexer::pysyspath::PySysPathMap::build(project_root),
135+
jvm: crate::indexer::jvm_package::JvmPackageMap::build(project_root),
136+
go: crate::indexer::go_module::GoModule::build(project_root),
137+
};
138+
cache.insert(
139+
project_root.to_path_buf(),
140+
CachedResolutionMaps {
141+
built_at: std::time::Instant::now(),
142+
cargo_toml_mtime,
143+
cargo_lock_mtime,
144+
composer_json_mtime,
145+
maps: maps.clone(),
146+
},
147+
);
148+
maps
149+
}
150+
151+
/// Phase B plan T4b: the 3 manifest filenames `cached_resolution_maps`
152+
/// tracks mtimes for (see its doc comment) — a standalone predicate so both
153+
/// `reindex_paths` and `reindex_changed_cancellable` check the same thing.
154+
/// Root-relative exact match only, matching that function's own
155+
/// `project_root.join(name)` checks — a nested workspace member's manifest
156+
/// doesn't affect this cache.
157+
pub(super) fn is_manifest_path(rel: &str) -> bool {
158+
matches!(rel, "Cargo.toml" | "Cargo.lock" | "composer.json")
159+
}
160+
161+
/// Phase B plan T4b (Risk Abductive-1 mitigation): force-evict
162+
/// `project_root`'s `cached_resolution_maps` entry. Belt-and-suspenders on
163+
/// top of that function's own mtime comparison, which is correct only to
164+
/// the filesystem's mtime resolution — some filesystems round to 1s, so two
165+
/// edits to the same manifest inside one second would otherwise look
166+
/// "unchanged" and keep serving the first edit's stale maps. Called only
167+
/// when this pass's own `changed_paths` already proves a manifest was
168+
/// touched (a hard fact, not a heuristic), so an unconditional evict here is
169+
/// free insurance rather than a guess. A no-op if nothing has been cached
170+
/// yet for this `project_root`.
171+
pub(super) fn invalidate_resolution_maps_cache(project_root: &Path) {
172+
if let Some(lock) = RESOLUTION_MAPS_CACHE.get() {
173+
lock.lock()
174+
.unwrap_or_else(std::sync::PoisonError::into_inner)
175+
.remove(project_root);
176+
}
177+
}

0 commit comments

Comments
 (0)