Scan a command per invocation and report a rule per object - #402
Merged
Conversation
A command whose invocations were all compiles got one scan, taking flags from the first invocation and sources from all of them, and a command with any non-compile invocation got none. Both halves cost real builds. A rule like `gcc -c a.c -o a.o && gcc -Iinc_b -c b.c -o b.o` produced `gcc -M a.c b.c` without -Iinc_b, so the scan could not find b.c's header and the build failed outright. And a rule like `gcc -c a.c -o a.o && cd sub && gcc -c b.c -o b.o` was refused whole, so a.o -- perfectly reproducible from the rule's directory -- went unscanned and was reported as if nothing about it could be recovered. The unit was wrong in both places. Scan per invocation, over the longest leading run of compiles the scanner recognizes: each scan carries that invocation's own flags and its own sources, and scanning stops at the first invocation that could change the directory or the environment, because past it the scan would preprocess in a state the compile never had. An empty prefix still refuses everything, which is the bound that keeps the rule from becoming scan-everything. Reporting follows the same move, from rule to object: an object is covered when some generated scan names it, and every object no scan names is reported. That dissolves "a rule scanned in part" rather than encoding it -- per object the answer is binary again -- and it fixes a case that predates this change, where a rule declaring an object no invocation writes was silent because a scan existed for the rule. Coverage is decided by match_and_generate, never a re-derivation, so the existence-keyed map the check used is gone. Carrying that needed the scanner interface widened. build_dep_scans returns Vec<DepScan>, and DepScan carries the scan command with the object word the invocation writes, so the mapping from scan to object is constructed at the only site able to construct it -- inside the scanner as it reads the -o word -- and travels as one value rather than being re-derived by re-parsing the command text. Empty is refusal: "scanned nothing" and "refused" were never two states, and an optional<Vec> would have spelled one state twice. GeneratedRule gains covered_object to forward it, because the reporter consumes match_and_generate rather than the scanner. RulePattern::generate widens with them; it is a second singular surface that a name-based grep for the scanner does not reach. The old name is deleted rather than aliased, so every stale call is a compile error instead of a silently adapted one. That widening was designed to land first, as its own behaviour-preserving commit, so this one would show the decision rather than 76 sites of plumbing. It is not separable in the end: each scanner's builder body was re-indented into the per-invocation loop, so the rename and the prefix law occupy one indivisible hunk, and splitting it by line yields a half-rewritten function rather than two commits that build. Landing it whole is the declared divergence, not an oversight. The object comparison is the part to be careful with, and it took two attempts. The scan's output word is relative to the rule's own directory, exactly as the compile resolves it, so joining it there is what puts it in the declared output's space -- %o one directory down expands to ../../build/src/lib/a.o and lands on the declared path. An intermediate version of this work stripped the variant prefix off the declared side instead, which looked right against a rule written `-o a.o` in an out-of-tree build and was backwards: that rule is malformed, writing into the source directory while declaring an object under the variant, and stripping guaranteed a mismatch for every correct rule not at the project root. A cross-model review caught it with the %o repro; the pin added for it uses a subdirectory rule, because a root-level rule has an empty source directory and never exercises the round trip. Both drivers also learn the joined output spelling: -oa.o left the object unrecorded and, worse, rode into the scan command, which would have made the scan write an object. No index change is needed. Two invocations can render byte-identical scan text and share one node, which is correct rather than a collision: identical text requires identical sources and identical flags, so the two scans have identical dependency sets and the nodes are interchangeable by construction. Verified: whole suite green in one process -- 832 cases, 170196 assertions -- plus make check at exit 0, and format, tidy, iwyu and spec-check clean. Each flipped requirement was observed red first; the reporter's red needed a control tree built from 0e79076, where the same rule names a.o -- the object that is reproducible -- and never names b.o. Left alone deliberately: reports_own_deps still tests the whole command text, so one depfile flag anywhere suppresses reporting for every object of that command. Under a per-object reporter that is an asymmetry, and it belongs to the separate issue #357 rather than here. Closes #355. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjcxNmyFfoTJYSZKKgjrCp
PR metricsPerformance (gcc example, Linux)
Deterministic signals: instructions (cachegrind-simulated instruction reads — exact across runs, no PMU needed), page faults, peak RSS, and the cachegrind D1/LL miss rates. CPU time is user+sys from time(1). Internal statistics (gcc example, up-to-date dry run)
Counters from Binary size (Linux)
Code churn (whole codebase, last 30d)
Of the lines written across the codebase in the last 30 days, how many are already gone — work that was written and then discarded or rewritten inside the same window. This is the state of the tree including this PR, not a measure of the PR itself. Only code we write is counted: tests, examples, vendored and generated files, CI plumbing and prose are excluded. 5413 lines were deleted in the window in total, most of them older than it. Where the churn is
Test coverage (lines)
102 files · 16582/18823 lines covered Deltas vs main@0e7907602. Updated for 6cf813a |
Per-object reporting made the reporter ask the production predicate about every command carrying an object, where before a single scan node stood as proof its whole rule was covered. The answer got more correct and the dry run got slower: check_unscanned_compiles went from 2.2M instructions to 250.6M on the gcc example — more than the entire measured regression, everything else netting slightly cheaper. The cost was not the predicate running. It was that one run tokenized the same command four times: each scanner's matches, gcc's build_dep_scans, clang-cl's has_dep_flags, with both scanners keeping their own copy of command_words and each re-splitting invocations the other had already split. Deleting the redundant matches() guard inside match_and_generate looks like the fix and is not. It was applied and measured: the work relocates rather than disappearing, because build_dep_scans then runs for every command matches() used to reject, and the tokenizer count does not move. It recovers 0.5%. A command is now tokenized once and the words and the invocation split travel together as CommandTokens, built by the outermost per-command caller and passed to every scanner surface. The split is shared, not only the words: splitting is scanner-independent, so sharing the words alone would leave each scanner re-splitting. Both copies of command_words are gone. The value is derived, so it shares down the stack and never crosses the build boundary. Persisting the covered object in the index would have closed the same number, but parse installs no scan nodes and must recompute regardless — leaving two encodings of coverage obliged to agree forever, for half the paths. Sharing within a frame cannot go stale, because it cannot outlive its input. Four fences keep the shared value from becoming a second source of truth about the command: tokenize_command is the only way to obtain one, and it embeds the text it derived from; the scanner virtuals lose their raw-text forms in this same commit, so no scanner can tokenize independently and drift; copy construction and copy assignment are deleted, because invocations span into the words buffer and a copy would dangle silently where a move is safe; and the type holds views, so persisting it is structurally hostile rather than merely forbidden. Shape A's output side is untouched — build_dep_scans still returns Vec<DepScan>, empty still means refusal, and the object still rides into GeneratedRule. Reporting also stops making a whole-command claim per object. Under a covered prefix the old sentence was false: "gcc -c a.c -o a.o && cd sub && gcc -c b.c -o b.o" named sub/b.o while a.o had a live scan, and the message asserted the command contained no reproducible compile with one standing next to it. A report's sentence carries the report's unit; this one had not moved with it. Measured on the same callgrind pair and workload as the regression. Tokenizer calls per command: 1.53 before #401, 3.45 after, 1.006 now — the absolute tokenizer cost is below the pre-#401 baseline. check_unscanned_compiles 250.6M to 170.7M; the whole dry run 2,037.9M to 1,900.6M, so +13.7% against main becomes +6.0%. The remaining ~108M is the per-object work itself: one build_dep_scans walk and one expand_instruction per object-bearing command. Whole suite green in one process — 832 cases, 170,199 assertions — with make check at exit 0 and format, tidy, iwyu and spec-check clean. The message change was observed red first. The copy deletion is proven by a compile error rather than asserted, and the profile on the final tree is byte-identical to the one measured before it, so every number above describes the code as it stands. A cross-model review found the copy hazard independently, on the same line, while the tree was held frozen. It also priced one accepted cost: the free matches_*_compile entry points now intern the command text per call, on a path that reaches production only through RulePatternRegistry, which the shipped CLI never populates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjcxNmyFfoTJYSZKKgjrCp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A command whose invocations were all compiles got one dependency scan, taking
flags from the first invocation and sources from all of them; a command with any
non-compile invocation got none at all. Both halves cost real builds.
gcc -c a.c -o a.o && gcc -Iinc_b -c b.c -o b.oproducedgcc -M a.c b.cwithno
-Iinc_b, so the scan could not findb.c's header and the build failedoutright.
gcc -c a.c -o a.o && cd sub && gcc -c b.c -o b.owas refused whole, soa.o— perfectly reproducible from the rule's directory — went unscanned and was
reported as if nothing about it could be recovered.
The unit was wrong in both places.
Scanning is now per invocation, over the longest leading run of compiles the
scanner recognizes. Each scan carries that invocation's own flags and its own
sources. Scanning stops at the first invocation that could change the directory
or the environment, because past it the scan would preprocess in a state the
compile never had. An empty prefix still refuses everything — the bound that
keeps the rule from becoming scan-everything.
Reporting is now per object. An object is covered when some generated scan
names it; every object no scan names is reported. That dissolves "a rule scanned
in part" rather than encoding it — per object the answer is binary again — and it
fixes a case that predates this change, where a rule declaring an object no
invocation writes stayed silent because a scan existed for the rule. Coverage is
decided by the production predicate, never a re-derivation.
Carrying that needed the scanner interface widened:
build_dep_scansreturnsVec<DepScan>, and the element carries the object word the invocation writes, sothe scan-to-object mapping is built at the only site able to build it — inside
the scanner, as it reads the
-oword — and travels as one value instead ofbeing re-derived by re-parsing the command text. Empty is refusal, so there is no
second spelling of one state.
One declared divergence
The interface widening was designed to land first as its own behaviour-preserving
commit, so the behaviour commit would show the decision rather than 76 sites of
plumbing. It turned out not to be separable: each scanner's builder body was
re-indented into the per-invocation loop, so the rename and the prefix law occupy
one indivisible hunk, and splitting it by line yields a half-rewritten function
rather than two commits that build. Landing it whole is a deliberate call, not an
oversight.
Worth a reviewer's eye
The object comparison took two attempts. The scan's output word is relative to
the rule's own directory, exactly as the compile resolves it, so joining it there
is what puts it in the declared output's space —
%oone directory down expandsto
../../build/src/lib/a.oand lands on the declared path. An intermediateversion stripped the variant prefix off the declared side instead, which looked
right against a rule written
-o a.oin an out-of-tree build and was backwards:that rule is malformed, writing into the source directory while declaring an
object under the variant, and stripping guaranteed a mismatch for every correct
rule not at the project root. A cross-model review caught it with the
%orepro.The pin added for it uses a subdirectory rule, because a root-level rule has an
empty source directory and never exercises the round trip.
Both drivers also learn the joined output spelling:
-oa.oleft the objectunrecorded and, worse, rode into the scan command, which would have made the scan
write an object.
No index change is needed. Two invocations can render byte-identical scan text
and share one node, which is correct rather than a collision: identical text
requires identical sources and identical flags, so the two scans have identical
dependency sets and the nodes are interchangeable by construction.
Verification
Whole suite green in one process — 832 cases, 170,196 assertions — plus
make checkat exit 0, andformat,tidy,iwyuandspec-checkclean. Eachflipped requirement was observed red first. The reporter's red needed a control
tree built from
0e7907602, where the same rule namesa.o— the object thatis reproducible — and never names
b.o.One asymmetry is left alone deliberately: the self-depfiling suppression still
tests the whole command text, so one depfile flag anywhere silences reporting for
every object of that command. That belongs to its own separate issue, not here.
Closes #355.