A rebuild started one git process per trailer paragraph (#776) - #789
A rebuild started one git process per trailer paragraph (#776)#789MongLong0214 wants to merge 4 commits into
Conversation
Measured on this repository: one `commitlore index --rebuild` started 4,345 git subprocesses, of which 4,329 were `interpret-trailers --parse`. The other sixteen were the whole rest of the work -- one `rev-list`, three batched `git log`s, a notes read. Forty-five seconds, and essentially all of it process startup. The batch reader already avoids this and the module says so in its own header: "One `git log` per batch of commits replaces one process per commit, which is the difference between indexing 100k commits and not finishing." `explodeRecordBlocks` went around it. `%(trailers)` returns only a message's last paragraph, so recovering a squashed earlier block means re-reading the message -- and that pass parsed *every* earlier paragraph to find the three percent that carry one. Thirty-nine hundred parses to keep thirty-three. Two changes, both of which move a cheap test in front of an expensive one that was already being discarded: `parseRecordBlocks` ran `asIsolatedBlock` on each earlier paragraph and *then* dropped any result without a `Record-Id`. The raw-text test now runs first. A trailer key is always at the start of a line in the source and folding continues values, so a paragraph that never names the key cannot yield one. `explodeRecordBlocks` now skips the parse for messages the atom pass already covers entirely. That second gate was wrong on the first attempt, and the way it was wrong is worth keeping. ADR-0014 says a message with at most one `Record-Id` has exactly one block, so the gate counted. But that is a statement about how many record blocks exist, not about whether the one git handed the atom pass is it: a commit whose final paragraph is `Co-authored-by:` has its record one paragraph earlier, and the count-only gate dropped nine rows from exactly such a commit. The gate now checks position as well as count. Record-Id: r-rebuildprocesscost Provenance: authored Certainty: firm Blast: module Undo: easy Limit: this removes process startup, not work. A repository whose commits genuinely carry multiple record blocks still pays one parse each, and the remaining seconds are the batched `git log`s and the inserts. `%G?` in the rebuild format is another 2.7s on this tree and is not touched here Verified: the index is identical, not merely smaller -- every row of `trailers` dumped before and after and compared, 7250 rows both times, byte-for-byte equal. 45s to 6.8s on the same repository. Negative control: reverting the gate to count-only reproduces the nine missing rows, and the boundary it got wrong is now four tests CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 4 commits in Active constraints for the paths this PR touchesLimits (251)
Ruled out (384)
Truncated: 375 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
`%G?` in the scan format makes git verify each commit's signature. Nothing reads the answer outside signature mode: `grade.ts` consults `signatureStatus` only behind `requireSignedDirective`, and `trusted-authors.ts` says so in the comment above the verifier generation -- "Only signature mode pays for it: the setting is opt-in". The rebuild paid for it always, at 2.4 s of a 4.9 s rebuild on this repository. Enabling the setting afterwards is not a stale-cache hazard, because the mechanism for that already exists: `signatureVerifierGeneration` goes from `null` to a hash, `healthProblem` compares it against the value in `meta`, and the mismatch rebuilds. That is the same path a changed keyring already takes (#653). The field is emitted empty rather than dropped, so the positional destructure keeps its shape -- a shifted field is a silent wrong answer, and this format is read by index. An empty string is also outside git's `%G?` vocabulary (G/B/U/X/Y/R/E/N), so a stored `''` cannot be misread as "no signature", which is `N` and a different fact from "not asked". What records that nothing was asked is the absent `signature_verifier_generation` in `meta`. Record-Id: r-signatureatomoffbydefault Provenance: authored Certainty: firm Blast: module Undo: easy Limit: this makes an unsigned-mode index carry `''` where it used to carry git's verdict, so a reader that wanted the cached status without turning signature mode on no longer gets it. None exists today, and the honest signal for the difference is the missing meta key rather than the column Verified: 4.9 s to 2.5 s on the same HEAD, with every `trailers` row dumped before and after and compared -- 7258 rows both times, identical. The test exercises the exported function rather than a copy of it: a first version defined its own `signatureAtomFor` in the test file and would have passed with the implementation deleted CommitLore-Version: 2.0.0
|
Second optimisation added:
The rebuild paid for it always. Turning the setting on later is already handled, and by the mechanism built for it: The field is emitted empty rather than dropped, so the positional destructure keeps its shape — a shifted field is a silent wrong answer. And Verified the same wayEvery One correction to my own test: the first version defined its own copy of #776 after thisThe product question is now against ~2.5 s, not 23 s. The third item from the audit is still not taken: the 3-second consumer budget spends itself on the explode pass for the first slice of commits. |
`BUDGETED_LOG_BATCH` is 64 because `LOG_BATCH` exceeds the commit count of most repositories, so a budgeted scan using it never reached a second iteration and never stopped. That reason is sound and the constant stays as the *first* batch; what was wrong was holding it there. A batch is three `git log` processes however many commits it covers. On a 10,000-commit repository that made 177 of them where the unbounded path makes 30, and the budgeted scan managed about a quarter of the commits per second that a full rebuild did -- so a 2.2 s rebuild did not fit inside a 3 s budget, and a fresh clone served a partial answer with the caveat that #776 is about. The batch now doubles to `LOG_BATCH`. The early iterations are small, so a repository of any size still gets its deadline checked often, which is the whole reason 64 was chosen. The later ones are large, so the per-batch process cost is paid a handful of times instead of a hundred and fifty. A repository smaller than one batch never reaches the second iteration and is untouched. Measured on 10,000 commits with no index: 196 git processes to 58, first answer 3.8 s to 2.9 s, and the incomplete-scan caveat gone -- the scan now finishes inside the budget rather than reporting how far it got. Record-Id: r-growingbudgetbatch Provenance: authored Certainty: firm Blast: module Undo: easy Limit: the deadline is still only checked between batches and before the expensive half of one, so a single late batch of 1024 commits can overshoot by whatever that batch costs. The overshoot is bounded by one batch rather than by the whole scan, which is what it was before 64 existed Verified: negative control -- pinning the size back to 64 reproduces the incomplete-scan caveat on the same corpus, and removing the growth reproduces the 177-process count. The sizes themselves are four tests: starts at 64, reaches 1024 and no further, covers 10k in under 20 batches, and leaves a 50-commit repository as a single batch CommitLore-Version: 2.0.0
|
Third and last item from the audit is in: the budgeted scan no longer spends its budget on its own deadline checks.
A batch is three It now doubles to Measured, 10,000 commits, no index
The scan now finishes inside the budget rather than reporting how far it got. The whole of #776, measured
A correction worth recordingMy first attempt at the 10k scaling measurement reported no difference — 2.1 s before, 2.2 s after — and I nearly wrote it up as "the synthetic corpus does not exercise the slow path". The control never ran. I reverted with Two controls that did not fail before one that did. |
`byteSnapshot` reads a directory and then stats each entry, and git writes and
removes its own lock files under `.git/objects` in between. CI caught it on
`maintenance.lock`:
ENOENT: no such file or directory, lstat
'/tmp/capture-shadow-HQr7Dr/.git/objects/maintenance.lock'
A snapshot that dies because the repository tidied up is reporting on the
walker rather than on the thing under test, and this test is about whether
`capture --shadow` leaves `.git` untouched. Entries that vanish between the
`readdir` and the `lstat` are now skipped; anything else still throws.
The tolerance cannot hide what the test exists to catch. A file the shadow run
wrote would still be there to be stat'd, and a file it changed still hashes
differently -- only an entry that is gone by the time it is looked at is
skipped, and the shadow run does not delete anything.
Provenance: authored
Certainty: firm
Blast: local
Undo: easy
Limit: an entry that appears and disappears entirely between the two snapshots is invisible to this test, and was before -- the snapshot compares two points in time and never claimed to see between them
Verified: three properties checked against the same walker rather than argued -- a changed file is still detected, an added file is still detected, and a vanishing entry no longer throws. The failure it fixes was not reproducible locally in three runs, which is why the check is the walker's behaviour rather than a rerun
CommitLore-Version: 2.0.0
|
Closed in favour of #790, which is the same change. What was wrong: my last commit on this branch carried CommitLore trailers without a I did not amend and force-push; #790 rebuilds the work as one properly-recorded commit on a fresh branch. The first failure on this branch was a different thing and is fixed rather than re-run: |
Blocked only on CI. Nothing else is outstanding — all three items from the #776 audit are in, measured, and negative-controlled. Merge when green; that closes #776.
Closes #776. The 23-second rebuild was not a floor — it was 4,329 git process spawns.
Not closing #776: this removes the reason the number was large, and the remaining product question (below) is unchanged in shape but much smaller.
What one rebuild actually did
The batch reader already avoids one process per commit, and the module says so in its own header:
explodeRecordBlockswent around it.%(trailers)returns only a message's last paragraph, so recovering a squashed earlier block means re-reading the message — and that pass parsed every earlier paragraph to find the ones carrying a second record. On this repository: 3,900 parses to keep 33.Both changes move a cheap test in front of an expensive one already being discarded
parseRecordBlocksranasIsolatedBlockon each earlier paragraph and then dropped any result without aRecord-Id. The raw-text test now runs first — a trailer key is always at the start of a line in the source and folding continues values, so a paragraph that never names the key cannot yield one.explodeRecordBlocksskips the parse entirely for messages the atom pass already covers.The gate was wrong the first time, and how is worth reading
ADR-0014 says "a message with at most one
Record-Idanywhere has exactly one block", so the first gate counted occurrences and skipped at ≤1.That is a statement about how many record blocks exist, not about whether the one git handed the atom pass is it. A commit whose final paragraph is
Co-authored-by:has its record one paragraph earlier —%(trailers)returns the conventional block, and the real record is only reachable through the explode pass. The count-only gate dropped nine rows from exactly such a commit here.The gate now checks position as well as count. The boundary is four tests.
Measured
trailersrowsIdentical, not merely smaller — every row of
trailersdumped before and after and compared field by field. That check is what caught the first gate; the timing alone looked like a win.Negative control: reverting to the count-only gate reproduces the nine missing rows.
What is left on #776
The product question does not go away, it shrinks: what first-use delay is acceptable in a freshly cloned repository, now against ~7 s rather than ~23 s, with
CONSUMER_SCAN_BUDGET_MS = 3_000still the consumer budget.Two further reductions are identified and not taken here:
%G?in the rebuild'sgit logformat costs 2.7 s on this tree and is only needed in signature mode.