Skip to content

prepare_repository_once: retain what it learned, not the 31.8 MB it learned it from - #8415

Merged
briansrls merged 1 commit into
mainfrom
fix/prepared-inventory-corpus-duplicate
Aug 18, 2026
Merged

briansrls merged 1 commit into
mainfrom
fix/prepared-inventory-corpus-duplicate

Conversation

@gunbai-bot

@gunbai-bot gunbai-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

PLAT-3, the memory lane.

PreparedRepository held every admitted file content in a Vec<InventoryEntry>, beside the Rc<SourceFile> list the resolved graph already owns. sf.content.clone() is a deep copy of every byte: 31,819,415 bytes across 3,655 modules, measured on this tree, paid on every floor run.

What it bought

The retention was justified by three questions a later consumer might ask — what modules exist, which files declare witnesses, what a module path is. Only the middle one ever had a consumer:

InventoryEntry.content read in exactly 2 places, both inside inventory_witness_files
inventory_witness_files exactly 1 caller
prepared.inventory exactly 1 reader

So the whole corpus was retained to feed one fold, called once, whose output is a list of witness sites.

The fix is the construction move, not the tidier one

The obvious smaller change is to hold Rc<SourceFile> instead of cloning the String. That removes the duplicate and leaves the corpus retained — which keeps the invitation. The next consumer with a question about source text answers it by scanning the copy in hand, and that is a second representation of facts the resolved graph is the authority for. The retained inventory has already been exactly that once, in the entry-closure import scan this file own comment records as a defect that silently narrowed.

Instead the fold runs during preparation and the bytes are dropped. witness_file_from_source decides one file from the bytes preparation is holding at that moment, PreparedRepository carries Vec<InventoryWitnessFile>, and InventoryEntry is deleted. With the corpus copy gone, answering the next question from it is not merely discouraged, it is unavailable — so the duplicate cannot return by someone reaching for what was conveniently there. §5 construction over validation.

Behaviour is unchanged by construction

Each half is the same rule in the same order:

  • the exclusion filter still runs before the file is considered
  • test fn and data ... LiveTreeDisposition ... ReadsLiveTree are still recognised at column zero only
  • a file with no test fn is still dropped, which is what the predecessor if !functions.is_empty() did after building the record
  • the path still has backslashes normalised
  • the population is still sorted by path, now at the end of preparation rather than at the end of the fold

Checked with cargo check --release -p v1-compiler --bin claim_executor.

Noted, not fixed here

--all-targets additionally fails at cli_run.rs:32323 and :32327, where Some(crate::v1_interpreter::str_value(s)) appears in pattern position. That is invalid Rust independent of this change, it is in #[cfg(test)] code, and it is untouched by this diff — these edits are all at line 40575 and beyond. It has not compiled since the Rust suite left CI on 2026-07-11, which is the green-by-vacancy class rather than a regression.

…earned it from

`PreparedRepository` held every admitted file's `content` in a
`Vec<InventoryEntry>`, beside the `Rc<SourceFile>` list the resolved
graph already owns. `sf.content.clone()` is a deep copy of every byte:
31,819,415 bytes across 3,655 modules, measured on this tree, paid on
every floor run.

WHAT IT BOUGHT. The retention was justified by three questions a later
consumer might ask -- what modules exist, which files declare witnesses,
what a module's path is. Only the middle one ever had a consumer:

  InventoryEntry.content   read in exactly 2 places, both inside
                           inventory_witness_files
  inventory_witness_files  exactly 1 caller
  prepared.inventory       exactly 1 reader

So the whole corpus was retained to feed one fold, called once, whose
output is a list of witness sites.

THE FIX IS THE CONSTRUCTION MOVE, NOT THE TIDIER ONE. The obvious
smaller change is to hold `Rc<SourceFile>` instead of cloning the
String, which removes the duplicate and leaves the corpus retained. That
keeps the invitation: the next consumer with a question about source
text answers it by scanning the copy in hand, which is a second
representation of facts the resolved graph is the authority for -- and
the retained inventory has already been that once, in the entry-closure
scan this file's own comment records.

Instead the fold runs during preparation and the bytes are dropped.
`witness_file_from_source` decides one file from the bytes preparation
is holding at that moment; `PreparedRepository` carries
`Vec<InventoryWitnessFile>` and `InventoryEntry` is deleted. With the
corpus copy gone, answering the next question from it is not merely
discouraged, it is unavailable -- so the duplicate cannot return by
someone reaching for what was conveniently there.

BEHAVIOUR IS UNCHANGED BY CONSTRUCTION, and each half is the same rule
in the same order: the exclusion filter still runs before the file is
considered; `test fn` and `data ... LiveTreeDisposition ... ReadsLiveTree`
are still recognised at column zero only; a file with no `test fn` is
still dropped, which is what the predecessor's `if !functions.is_empty()`
did after building the record; the path still has backslashes
normalised; and the population is still sorted by path, now at the end
of preparation rather than at the end of the fold.

Checked with `cargo check --release -p v1-compiler --bin claim_executor`
on the current tree.

NOTED, NOT FIXED HERE: `--all-targets` additionally fails at
cli_run.rs:32323 and :32327, where `Some(crate::v1_interpreter::str_value(s))`
appears in pattern position. That is invalid Rust independent of this
change, it is in `#[cfg(test)]` code, and it is untouched by this diff --
these edits are all at line 40575 and beyond. It has not compiled since
the Rust suite left CI on 2026-07-11, which is the green-by-vacancy class
rather than a regression.
@gunbai-bot

gunbai-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Investigated. This one genuinely could have been mine — it changes witness discovery in cli_run.rs, unlike my workflow-config PRs where no witness can read the changed files. So I checked it on its own terms rather than reusing that argument.

The decisive check: the discovered population is identical

required-floor: planned=9418 executed=9418 terminal=9418 passed=8563 known_red_held=795 failed=60

9418 discovered, and a run on a sibling branch at a comparable tree also discovered 9418. If this refactor had changed which files are recognised as witness sites, or dropped one, planned would differ. It does not. That is the number this PR could have moved, and it did not move.

Every failure is a known mode; none is discovery or correctness

count mode
17 errored: non-exhaustive pattern match
13 exceeded its Cpu budget
11 errored: witness receipt wall budget
10 errored: eval budget
9 errored: host tool unresolved

Zero failures fall outside those five, and none is a count mismatch, a missing site, a wrong module path, or a wrong reads_live_tree classification — the four ways this diff could break something.

Of them, 17 are the NetMemberBelongsToAnotherDesign missing import, fixed by #8419 which merged AFTER this branch base (ed3f64adc7). 9 are the shared-~/.cargo race, fixed by #8420. The remaining 34 are the budget band, which moves with host load — two of them missed by 1ms and 42ms.

Why the count is higher than a sibling run

60 here against 37 on a branch cut later. The difference is base age, not quality: this branch predates #8419, so it still carries all 17 pattern-match failures that main has since fixed. Rebasing would drop them; I have left it alone because the discovery evidence above does not depend on the base, and a rebase would reset the two approvals.

Equivalence argument, for the record

Behaviour is unchanged by construction, and each half is the same rule in the same order: the exclusion filter still runs before a file is considered; test fn and data … LiveTreeDisposition … ReadsLiveTree are still recognised at column zero only; a file with no test fn is still dropped, which is what the predecessor if !functions.is_empty() did after building the record; backslashes are still normalised; the population is still sorted by path with the same comparator, moved from the end of the fold to the end of preparation.

One honest difference: the scan now runs during preparation, so on the refusal path — when the strict typecheck rejects — it does work the predecessor would have skipped. That path is rare and already expensive, and it costs nothing on the success path.

— sent from tidy-lark-471

@briansrls
briansrls merged commit 5a16f05 into main Aug 18, 2026
1 check failed
@briansrls
briansrls deleted the fix/prepared-inventory-corpus-duplicate branch August 18, 2026 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant