perf(cli): per-session source cache for corpus loading (112s → 4.5s) - #135
Conversation
A full-corpus load benchmark over the 9,909-chunk ingest measured 111.7s and 9,909 source imports for only 400 unique sources — each tab parsed ~25×. A per-session sha256→Score cache cut it to 4.5s / 400 imports with an identical result (9,909 loaded both ways). `load_corpus_material_with` injects the importer so the count is observable. The test writes two chunks that share one source and asserts it is imported once; the loader currently imports per chunk, so it fails 2 != 1. The cache is the GREEN. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
`load_chunk` keys the parsed `Score` by the source's sha256 (filename for pre-v9 records) in a per-load `HashMap`, so a tab shared by many chunks is parsed once. The count test passes (1 import for 2 chunks), and the existing correctness test is unchanged — `prepare_chunk` slices an immutable `&Score`, so reuse is bit-identical to per-chunk parsing. The hash check lives on the cache-miss path, so a mismatched source is still a strict load failure, never bypassed by reuse. Full-corpus effect (measured): 111.7s → 4.5s, 9,909 source imports → 400, same 9,909 chunks loaded. No persisted-schema change; the cockpit's OPFS loader is a separate path and unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
📝 WalkthroughWalkthroughCorpus chunk loading now supports injected parsing, shared ChangesCorpus loading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant load_corpus_material_with
participant load_chunk
participant Importer
participant prepare_chunk
CLI->>load_corpus_material_with: load corpus with importer
load_corpus_material_with->>load_chunk: process chunk using shared cache
load_chunk->>Importer: parse source on cache miss
Importer-->>load_chunk: return Score
load_chunk->>prepare_chunk: prepare expected slice
prepare_chunk-->>load_corpus_material_with: return chunk material
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/generation_input.rs`:
- Around line 87-103: Add a regression test for the cache-loading logic around
the key construction and validation, covering a legacy filename that equals a v9
source SHA-256 value. Ensure the test confirms the identities use separate
namespaces, the hash-pinned record performs validation, and the wrong cached
Score is not reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0664694-2f58-40a5-8dc8-2ab1676ccf22
📒 Files selected for processing (1)
cli/src/generation_input.rs
| let key = meta | ||
| .source | ||
| .sha256 | ||
| .clone() | ||
| .unwrap_or_else(|| meta.source.filename.clone()); | ||
| if !cache.contains_key(&key) { | ||
| let bytes = fs::read(dir.join(&meta.source.filename)).ok()?; | ||
| // A filename is not an identity: when the record pins the source's hash | ||
| // (schema v9), a same-named but different file must not silently supply | ||
| // the notes. A mismatch is a load failure — and a cache miss, so the | ||
| // check runs for every distinct expected hash, never bypassed by reuse. | ||
| if let Some(expected) = &meta.source.sha256 { | ||
| if &source_sha256(&bytes) != expected { | ||
| return None; | ||
| } | ||
| } | ||
| cache.insert(key.clone(), import(&bytes)?); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Namespace legacy filenames and SHA-256 cache keys.
Both identities share the same String key space. A pre-v9 filename equal to a v9 hash causes a false cache hit, bypassing hash validation and preparing the chunk from the wrong Score.
Proposed fix
- let key = meta
- .source
- .sha256
- .clone()
- .unwrap_or_else(|| meta.source.filename.clone());
+ let key = match &meta.source.sha256 {
+ Some(sha256) => format!("sha256:{sha256}"),
+ None => format!("filename:{}", meta.source.filename),
+ };Add a regression test covering the cross-namespace collision.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let key = meta | |
| .source | |
| .sha256 | |
| .clone() | |
| .unwrap_or_else(|| meta.source.filename.clone()); | |
| if !cache.contains_key(&key) { | |
| let bytes = fs::read(dir.join(&meta.source.filename)).ok()?; | |
| // A filename is not an identity: when the record pins the source's hash | |
| // (schema v9), a same-named but different file must not silently supply | |
| // the notes. A mismatch is a load failure — and a cache miss, so the | |
| // check runs for every distinct expected hash, never bypassed by reuse. | |
| if let Some(expected) = &meta.source.sha256 { | |
| if &source_sha256(&bytes) != expected { | |
| return None; | |
| } | |
| } | |
| cache.insert(key.clone(), import(&bytes)?); | |
| let key = match &meta.source.sha256 { | |
| Some(sha256) => format!("sha256:{sha256}"), | |
| None => format!("filename:{}", meta.source.filename), | |
| }; | |
| if !cache.contains_key(&key) { | |
| let bytes = fs::read(dir.join(&meta.source.filename)).ok()?; | |
| // A filename is not an identity: when the record pins the source's hash | |
| // (schema v9), a same-named but different file must not silently supply | |
| // the notes. A mismatch is a load failure — and a cache miss, so the | |
| // check runs for every distinct expected hash, never bypassed by reuse. | |
| if let Some(expected) = &meta.source.sha256 { | |
| if &source_sha256(&bytes) != expected { | |
| return None; | |
| } | |
| } | |
| cache.insert(key.clone(), import(&bytes)?); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/generation_input.rs` around lines 87 - 103, Add a regression test for
the cache-loading logic around the key construction and validation, covering a
legacy filename that equals a v9 source SHA-256 value. Ensure the test confirms
the identities use separate namespaces, the hash-pinned record performs
validation, and the wrong cached Score is not reused.
![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: 4b5520bec1
ℹ️ 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".
| .sha256 | ||
| .clone() | ||
| .unwrap_or_else(|| meta.source.filename.clone()); | ||
| if !cache.contains_key(&key) { |
There was a problem hiding this comment.
When the cache already contains this sha256, this condition skips the read/hash path for the current record entirely. In a corpus where a.chunk.json loaded hash H first and b.chunk.json also declares H but b.mid is missing or has been replaced, b now gets prepared from a.mid instead of being skipped as before; that hides broken provenance even though chunks are supposed to be usable only with their named source file.
Useful? React with 👍 / 👎.
1 participant
Per-session source cache for corpus loading
First two steps of the dedup-aware curation bootstrap: measure the load path, then eliminate the one redundancy the measurement exposed — nothing more. No SQLite, no new corpus format, no persisted-schema change.
b034fc6(main)The measurement (step 1)
A reproducible load benchmark over the full
corpus/ingest/(the 401-tab bulk corpus from #134):The load path imports each chunk's source independently, so the 400 tabs are parsed ~25× each.
0 skippedis also a full-scale confirmation that the #134 P1 fixes hold across the whole corpus — every chunk finds its source, verifies its hash, and resolves its exact track.The fix (step 2)
load_corpus_materialnow keys the parsedScoreby the source's sha256 (filename for pre-v9 records) in a per-loadHashMap, so a tab shared by many chunks is parsed once. Everything else is unchanged:track_index+bar_range(schema v9);prepare_chunkslices an immutable&Score, so a shared parse produces exactly the per-chunk-parse result (the existing correctness test is unchanged, and the count test asserts the shared result matches);Real-path proof
griff generate --corpus corpus/ingest/over all 9,909 chunks: 8.8 s end to end (load + generate + export). Before the cache the load alone was ~112 s, so the same command was ~116 s. The cache is in the production path the CLI and cockpit Generate share.Test
load_corpus_material_withinjects the source importer so the parse count is observable.one_source_is_parsed_once_for_all_its_chunkswrites two chunks that share one source and asserts it is imported once (RED: the per-chunk loader imports twice). The existingload_corpus_material_reads_chunks_slices_ranges_and_skips_missing_sourcesstill passes — same result, fewer imports.Deliberately not here
Per the arbiter's order, this is only benchmark → source cache. Still ahead in the milestone: the dedup-aware persistent curation queue in the cockpit, a first ≥100 accepted-phrase corpus, and generation A/B on it — then, only if a further measurement demands it, a storage/artifact decision (SQLite / canonical
griff-score).Not self-accepted, not merged.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
Summary by CodeRabbit
Bug Fixes
Tests