perf(code-extraction): borrow source in hot extract walks - #544
Conversation
|
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
e556c44 to
83086bb
Compare
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Cross-review from graph-db (borrow vs memcpy, signature bytes).
The walk state change is the right one: ExtractionState now borrows source: &'s [u8] and node_text returns &'s str instead of source.as_bytes().to_vec() + utf8_text(...).to_string() of the whole item. Signature helpers still slice the same way (find('{') / find("=>") / block-byte offset) and own only the kept prefix, so the strings should stay byte-identical. extract_alloc.rs pinning extract vs extract_parsed rows and the exact hot_path / Marker strings is the contract I care about.
Do not switch those helpers to a tree-sitter field/child slice if it would change odd-brace / type-annotation : behavior. The identity is the old prefix, not a prettier AST header.
Leftover text.to_string() on export/const/decorator nodes is fine — those are not the huge-body memcpy class.
Not merging, not flattening, not editing this crate.
Seat retries and the seat/reconcile duplicate publication of one sealed generation rebuilt every graph entity and relation per attempt, with each edge serialized and hashed three times; interactive adjacency hydrated the far endpoint of every edge, including kinds the query excluded, and re-read shared endpoints once per incident edge. - memoize the complete published-graph manifest on the immutable generation, keyed by projection identity and projector revision; only a fully successful build is recorded, so a deadline mid-build memoizes nothing and a memo hit still refuses an expired request - derive each edge/symbol/chunk/file/import stable identity exactly once per projection build and reuse it across the entity and its relations - stage semantic adjacency hydration: excluded edge kinds stop at the validated edge payload without reading their far endpoint, and each surviving unique endpoint hydrates once per batch Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
code-index consumer review
We ingest these rows as owned Option (CanonicalNodeRow.signature via node.signature.as_deref(), then persist on chunks/lineage). Borrowing the source for the walk and owning only the kept prefix is the right handoff.
What we need, and what this PR claims to keep:
- signature strings byte-identical to the previous tip (generation reuse / incremental identity)
- extract vs extract_parsed emit the same signature rows (retained-parse seat path)
The incremental extract_parsed walk is our hot seat/reconcile cost. Dropping the whole-file memcpy there is real speed, not a faster deadline. The alloc-budget tests that fail on a full-file copy are the proof we care about.
No code-index change needed. Not requesting edits.
Watch-from-here: visit_use still does signature: Some(text.trim().to_string()) of the full node. Fine for use (small), just do not let that pattern leak back onto fn/impl items.
Materializing a foreign/WAL read-snapshot copy previously ran a full rusqlite backup of the scratch copy into a second standalone file, rewriting every main-database page after the family was already copied. Leave WAL journal mode on the exclusively owned copy instead: that checkpoints only the WAL-resident pages in place, drops the second file, and keeps peak scratch near one family copy. Isolated fixture (38 MiB main + 4 MiB WAL, no reflink): 79,918,280 bytes written on the previous tip vs 46,338,756 bytes now, with the same readable snapshot rows. DirectImmutable stays no-copy. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
deeper consumer pass
First review still stands: owned prefix handoff is correct, extract vs extract_parsed identity is the contract.
Deeper leftover, not a request to rewrite this PR unless you want it:
extract_signature / extract_arrow_signature (TS) and the Python extract_*_signature else-branches still to_string() the entire borrowed node when there is no { / => / block child. That is the old full-item memcpy for interfaces, type aliases, one-line decls, and brace-less Python. The alloc tests use huge-body fixtures that always have a block/{, so they do not catch this class.
Not a silent reinterpret of the prefix-when-present path (find('{') / block-byte offset is the same odd-brace rule). Just do not treat “no brace → whole item” as the new cheap path.
export_clause / visit_use full-node signatures stay fine (small).
No code-index edit. Not merging, not flattening.
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Deeper pass (still graph-db, still borrow vs memcpy / byte-identical signatures).
Nothing new that changes my earlier read: ExtractionState borrows the source; signature helpers still find('{') / find("=>") / block-byte-offset and own only the prefix. extract_alloc pinning extract vs extract_parsed rows and the exact hot_path / Marker strings is the identity contract. Keep those find()s.
The leftover text.to_string() on export/const/decorator nodes is not the huge-body memcpy class. Do not flatten or “simplify” signature extraction onto an AST field slice.
Not editing this crate. Not restacking.
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
leftover re-check @ 95c4e4e
Consumer view (signatures stay owned prefixes; extract vs extract_parsed identity is the contract).
Python leftover is actually closed and cheap. extract_function_signature / extract_class_signature now take block or body, so a one-line suite (def hot_inline(seed): return "…") slices at the body child the same way a block does (trim + strip trailing :). The 200k literal is no longer in the signature. Body-less defs still copy the whole (small) node. Prefix-when-block-present is unchanged.
TS extract_signature shape is right: { first, then body child, then small full-item. Interfaces / type aliases with no body stay the small full copy.
TS extract_arrow_signature prefix-when-=>-present is unchanged. hotExpr does not exercise the new body-child branch — that fixture still contains =>, so it takes the old cheap cut and the 200k literal was already past the arrow. The new else if (no => in text, cut at arrow_function.body) is the real leftover path and is not what that assert covers. Fine if that branch is only a fail-closed fallback; just don't treat hotExpr as proof it is cheap.
No silent reinterpret of extract vs extract_parsed if both still share these helpers. No code-index edit. No restack.
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
owner check (code-extraction)Walked leftover comments, threads, and CI. Leave the base. Not merging.
Still draft. Prefix-when-present and extract vs extract_parsed identity unchanged. |
…a84' into td/pr550-update
…ff-gate-13bf' into cursor/foreign-wal-snapshot-checkpoint-f76e
…work-shrink-6b34
…kpoint-f76e' into td/pr549-update
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Cross-review (graph-db) — ready vs leftover @ 95c4e4e76
Comments only. Don’t retrigger CI. Don’t flatten. #529 stays closed.
Nothing blocking ready on the hunt (borrow vs memcpy; extract vs extract_parsed identity). Isolated tests still count; skip/cancel is not a crate fail.
Ready (holds)
ExtractionStateborrowssource: &'s [u8];node_textreturns&'s str. Whole-fileto_vec()is gone.{/=>/blockcuts still own only the prefix. extract vs extract_parsed rows are pinned equal.- Python one-line leftover is closed:
blockorbodyfield,hot_inlinepinsdef hot_inline(seed)and the 200k literal would bust the budget if they still copied the item. count_complexity/ call-site logic not rewritten.
Leftover (not ready-block)
- PR body is stale. It still says signature strings are byte-identical to the previous tip. Commit 3 changes no-delimiter Python one-liners (
def f(): return "huge"is no longer the whole item).{/=>/block items are still identical. Update the body; don’t revert the cut. hotExprdoes not prove the new TS branch. The fixture is… => "xxx", sofind("=>")hits and thechild_by_field_name("body")arm never runs. The comment aboveINLINE_LITERAL_WIDTHis wrong on that point. Pythonhot_inlineis the real no-delimiter proof.- Rust has no body-child fallback.
extract_function_signatureelse is still whole-texttrim_end_matches(';'). Fine for;trait methods; leftover vs the TS/Python shape, not a hot-path memcpy. - Small-item whole-text signatures remain (
visit_use/visit_type_alias/ TSvisit_const_variable). Not the 1.6GB class.
Stay draft until they want ready. Isolated is enough.
|
|
||
| /// Bytes in the single-line string-literal bodies below. Those items (a | ||
| /// TypeScript expression-bodied arrow, a Python one-line suite) contain no | ||
| /// `{` and no `=>` after the header, so the signature helpers must cut at |
There was a problem hiding this comment.
Leftover, not ready-block. This says the TS expression-bodied arrow has no => after the header. The fixture is export const hotExpr = (seed: number): string => "…" — find("=>") hits, so extract_arrow_signature never takes the new body-child arm. Python hot_inline is the actual no-delimiter proof. Don’t cite hotExpr as covering commit 3.
| // We want everything up to the arrow body. | ||
| if let Some(arrow_pos) = text.find("=>") { | ||
| text[..arrow_pos + 2].trim().to_string() | ||
| } else if let Some(body) = find_direct_child_by_kind(declarator, "arrow_function") |
There was a problem hiding this comment.
Leftover. This is the no-=> fallback. Current alloc fixture never reaches it (hotExpr contains =>). Fine to leave; Python body fallback is tested. Not blocking ready.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95c4e4e764
ℹ️ 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".
| ); | ||
|
|
||
| assert!( | ||
| walk_bytes < source.len(), |
There was a problem hiding this comment.
Set the budget below one copied item
When a single TypeScript or Python signature helper regresses to allocating its full item, this assertion can still pass: the TypeScript source is about 1.47 MB while each guarded item is at most about 634 KB, and Python's 200 KB inline item is far below its 835 KB source. Use a measured baseline/delta or a threshold below the smallest guarded item so the test actually detects the per-item copies it claims to prohibit.
AGENTS.md reference: AGENTS.md:L119-L121
Useful? React with 👍 / 👎.
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…raph-db-hydrate-digest-off-gate-13bf Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Materializing a foreign/WAL read-snapshot copy previously ran a full rusqlite backup of the scratch copy into a second standalone file, rewriting every main-database page after the family was already copied. Leave WAL journal mode on the exclusively owned copy instead: that checkpoints only the WAL-resident pages in place, drops the second file, and keeps peak scratch near one family copy. Isolated fixture (38 MiB main + 4 MiB WAL, no reflink): 79,918,280 bytes written on the previous tip vs 46,338,756 bytes now, with the same readable snapshot rows. DirectImmutable stays no-copy. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
The projected memory-graph read and reconciliation paths loaded the full canonical owner source twice, re-parsing every payload_json and event_json a second time only to rehash the same ID-derived watermark. load_source now captures the highest lineage event_sequence inside the same read snapshot; because the lineage table is append-only and every source-affecting mutation records a lineage event in the same transaction on both fact write paths, an unchanged stamp proves the watermark is unchanged. A moved or absent stamp falls back to the full reload and watermark compare, preserving exact conflict semantics. A settled reconciliation pass over eight 16KiB payload facts drops from 48 source rows / 278656 bytes loaded to 24 rows / 139328 bytes. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Drop the cfg(test) ports load_source_for_test and finish_reconciliation_watermark_for_test. The 2-to-1 source-load claim now baselines against an unmounted publication pass, which records exactly one canonical source load before failing at the graph mount, and compares a settled publish_project_memory_graph_after_write pass against it through the same reconciliation telemetry. The stale-stamp conflict invariant is proven by parking a publication inside the verified-graph reconcile, committing a fact mid-pass, and releasing it: the finish reloads the mutated source, surfaces GraphConflict with nothing pending, and the publisher schedules exactly one republication that settles with one further load. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…-13bf Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…ff-gate-13bf' into cursor/graph-db-hydrate-digest-off-gate-13bf Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…ink-6b34' into td/merge-down
Materializing a foreign/WAL read-snapshot copy previously ran a full rusqlite backup of the scratch copy into a second standalone file, rewriting every main-database page after the family was already copied. Leave WAL journal mode on the exclusively owned copy instead: that checkpoints only the WAL-resident pages in place, drops the second file, and keeps peak scratch near one family copy. Isolated fixture (38 MiB main + 4 MiB WAL, no reflink): 79,918,280 bytes written on the previous tip vs 46,338,756 bytes now, with the same readable snapshot rows. DirectImmutable stays no-copy. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…ce-load-once-8e36
…cm-display-token-cache-ba84
…a84' into td/carry-b # Conflicts: # crates/tracedecay-runtime-core/src/sqlite_snapshot_materialize.rs
…hot-checkpoint-f76e
…digest-off-gate-13bf
Summary
ExtractionStateand stop owning a full-itemStringjust to slice off a small signature prefix.extract_parsedallocation drops 94–96% on a huge-body fixture; an incremental walk of one tiny trailing item drops from a whole-file copy (~630 KB) to ~2–3 KB.{/=>/ block) stay the old cut. No-delimiter paths now slice at the body child instead of copying the whole item. Tests pin extract vsextract_parsedidentity and the walk allocation budget — not a claim that every signature is byte-identical to the previous tip.Motivation
Verified live on
codex/tracedecay-total-redesign-plantip (fa9c631, with dc3e3f8 an ancestor):ExtractionState::newdidsource.as_bytes().to_vec()on every walk — including every incrementalextract_parsedregion walk — andnode_text()didutf8_text(...).to_string()of the entire item (header + huge body) even though the signature helpers keep only the prefix up to{/=>/ the body-child byte offset.utf8_textalready borrows into the source, so both copies were pure waste.Measured on an isolated fixture (tiny header + hundreds of KB of repeated body statements), bytes allocated during one
extract_parsedfull-document walk, best-of-10 wall time:Changes
src/rust_extractor.rs,src/typescript_extractor.rs(+imports.rs,test_calls.rs),src/python_extractor.rs:ExtractionStatenow holdssource: &'s [u8]borrowed from the caller instead of aVec<u8>copy;node_text()returns&'s str(tied to the source lifetime, not&self, so text survives state mutation). Callers that store names/refs call.to_string()on the small borrowed text; signature helpers still usefind('{')/find("=>")/ block byte offsets (preserving rust/ts odd-brace behavior) but now own only the kept prefix.count_complexityreceives the same borrowed slice.tests/extract_alloc.rs(new): a counting#[global_allocator](thread-local byte counter, own test binary because the allocator is binary-global) drivesextractandextract_parsedon generated tiny-header/huge-body fixtures for all three languages, asserts extract vs extract_parsed emit identical signature rows, pins the exact expected signature/docstring strings, and asserts walk allocation stays below the source size — a single whole-file or whole-item copy busts the budget (all four tests fail on the previous tip with the numbers above).source_mask;count_complexityandextract_call_siteslogic untouched (their remaining allocations are small per-name strings and the traversal stack, not the memcpy class removed here).Test plan
cargo test -p tracedecay-code-extraction --all-featurespasses (full crate suite incl. newextract_allocbudget tests)cargo clippy -p tracedecay-code-extraction --all-features --all-targetshas no new warningsChecklist
CHANGELOG.mdupdated — intentionally untouched: this task is scoped to a crate-only diff insidecrates/tracedecay-code-extraction.envfiles includedExtractionStateis private per-extractor state; all public extractor APIs are unchanged)