Skip to content

Commit 47bfcd0

Browse files
claude[bot]claude
andauthored
perf(pm): discover each tree once per process — the dispatch-gates self-test now reaches a verdict inside the foreground cap (#18255)
Fixes #18201 ## The profile, which is what the card asked for first `pnpm check:pm-dispatch-gates` was measured end to end on `b3b43b6e`, detached, with every case line timestamped. It does not merely run past the container's ~10-minute foreground cap — **it completes in 760 s (12m40s)**, so the card's "9m0s then exit 124" was the kill, never the battery's real length. 1723 cases, exit 0. Where the 760 s went, measured three ways on the same tree: | reading | value | |:--|:--| | total wall clock, 1723 cases | **760 s** | | `spawnSync` calls from the battery | 253 (46 `dispatch-gates` CLI children, 198 `git`, 2 `bash`) | | time inside those children | 500.7 s = **66.2 %** | | of which: 24 children doing a FULL derivation | ~20.7 s each = **496 s** (the other 22 are argv/`--tier` probes at ~0.1 s) | | parent's own in-process `discoverFamilies()` | **157.1 s = 20.8 %** | | one plain derivation, timed alone | **21.2 s** | Then a V8 CPU profile of that single 21.2 s derivation, which is where the root cause is: ``` discoverFamilies — 2 profile node(s) incl=6.86s <- derive:12935 incl=7.06s <- gateFamilyFiles:9455 <- matches:9918 <- changeKindGates:9972 ``` **The same whole-tree discovery runs twice per derivation**, over a tree that cannot have changed between them — once from `derive()`, once from `gateFamilyFiles()` a whole call chain away under `changeKindGates()`. 13.9 s of the 21.2 s, 66 %. And the self-test's live cases call the same function a further twenty-odd times in one process, each one a fresh pass re-reading every workflow and re-masking every gate source. No hot spot, no quadratic: just the identical derivation, run N times. ## The fix: one discovery pass per tree, per process Exactly the shape — and the argument — the source maskers in this same file already landed: *"the same derivation run once instead of a dozen times, which is the only kind of speed-up this tool may take."* - `repoCorpus()` lists the checkout once per process and hands every caller the SAME listing and the SAME tree bundle. `derive()` now takes both halves from it, which also strengthens the "one read, N answers" invariant its own comment states — the pair is now the same two objects, not two equal copies. - `discoverFamilies` is memoised **on the tree OBJECT**. A caller that means a different tree hands a different object and gets its own pass; `null` — the deliberate no-tree probe — is not an object and is never memoised. The answer is a pure function of the tree plus the checkout on disk, and nothing in this process writes to the checkout: the CLI is one-shot and every write the self-test makes goes to a `mkdtemp` directory under the system temp root. - `check-dispatch-gates.mjs`: the kill branch now spends its one chance on the **remedy** rather than on naming the signal, and every run prints what the battery cost **on that box** — a reading that belongs to the run that took it, which is the one shape that cannot rot. No figure is frozen in the header; the header's convention is kept. ## Before / after | | before (`b3b43b6e`) | after (`0f901d56`) | |:--|:--|:--| | `--self-test` wall clock | **760 s** | **466 s** (−38.7 %) | | cases | 1723 pass, exit 0 | **1730 pass, exit 0** | | one plain derivation | 21.2 s | **13.2 s** (−38 %) | | longest single gap between case lines | 42.9 s | 27.2 s | 466 s = 7m46s, inside the cap. **The 13.2 s derivation is the half every dispatch pays**, not only this gate: every `os-dev` running `dispatch-gates.mjs --commands` on its own card gets the same 38 % back. ## ⛔ No gate weakened, and nobody else has to make the assertions Every one of the 1723 cases is still made by this same battery, in this same CI step. **Zero cases removed** — verified by diffing the case NAMES of both runs, not their counts: ``` before cases: 1724 after cases: 1731 (the extra line is the verdict line) REMOVED (3): - the tier constant's VALUE is spelled in exactly ONE site ... the definition at scripts/pm/dispatch-gates.mjs:10689 ... <- same case, line number moved by the insert - ⭐ 49 case(s) really handed this harness a diagnostic reading ... <- same case, 49 -> 53 - dispatch-gates self-test: 1723 cases pass. <- the verdict line itself ADDED (10): the 7 new cases below, plus the updated spellings of those same 3 ``` All three "removed" names carry a live reading inside the name; all three are present with the reading updated. Nothing was shrunk, skipped, `timeout`-wrapped or marked `continue-on-error`. Seven cases were ADDED, because the memo's whole symptom is work that does **not** happen, and absent work is invisible to every other case here — each of them asks what discovery ANSWERS, and the answer is identical either way: - the default tree is discovered ONCE per process — two further calls compute no pass - and those calls hand back the SAME object, so an entry ablated and restored is one entry - the corpus behind it is listed once too — one bundle, so the sweep and the discovery cannot describe different revisions - and that listing is the tracked corpus itself, not a trimmed copy of it - the tree `derive` hands down and the default `gateFamilyFiles` asks for are ONE pass, not two - a tree object that is not the corpus's is never served its answer — same content, its own pass - the no-tree probe is never memoised — it is not an object, and it still answers ## Byte-identity: the output did not move Five invocations (`plain`, `--commands`, `--json`, `--residue`, and a `scripts/pm` card), stdout **and** stderr, run against the unmodified `b3b43b6e` checkout and against this branch: ``` byte-identical after path normalisation: 10 stream(s); differing: 0 225585 total bytes compared ``` The only raw difference was the checkout directory in the provenance banner, which is what that banner prints by design. ## Reverse verification Direction predicted before running: **red**. Committed first, mutated on disk with the blob hash proving the bytes moved, `trap ... EXIT INT TERM` restoring, and the restore proven by `git hash-object` matching the HEAD blob with `git diff HEAD` empty. **Ablation A — the memo never hits** (`const hit = discoveryMemo.get(tree)` becomes `const hit = undefined`): ``` on-disk proof: anchor lines 1 -> 0 ; injected lines 0 -> 1 ; blob 52d58ee... -> 1f8b91d1... ✗ the default tree is discovered ONCE per process — two further calls compute no pass ✗ and those calls hand back the SAME object, so an entry ablated and restored is one entry ✗ the tree `derive` hands down and the default `gateFamilyFiles` asks for are ONE pass, not two ✗ dispatch-gates self-test: 3 of 1730 case(s) failed. SELFTEST_EXIT=1 wall clock back to 807 s restored: hash matches HEAD blob and git diff HEAD is empty ``` The other four new cases stay green under A, correctly: a memo that never hits breaks the collapse, not the tree-keying. ## What this PR deliberately does NOT do The card and the dispatch both floated classifying the family as CI-only so a dev "is not owed it locally". **Measured first, and then not taken.** The derivation reaches a card by **gate-script identity** — the change kind *"adds or edits a GATE SCRIPT (a file some discovered check family runs)"* — which is wider than the dispatch's hypothesis of `scripts/pm` paths: ``` scripts/pm/check-clause2-carriers.mjs -> pnpm check:pm-dispatch-gates IS in the 35 commands packages/spec/src/data/filter.zod.ts -> NOT in the 73 commands scripts/pm/dispatch-gates.mjs -> IS in the 31 commands ``` So it is not in every roster, and the cards it does reach are cards editing a gate script — exactly the population that should read this verdict. With the battery now inside the cap, taking it off their roster would remove a reading they can finally get, which is a weakening dressed as a scoping fix. The reconciler is untouched: exit 3 still reads NOT MEASURED and a 124/143 kill is still refused as a run. ## Acceptance notes Observed, not filed, not fixed here: - `lint.yml`'s own note for this step records a CI median of **597 s** over ten merge-group runs (from #16496). That reading predates this change and is a runner reading, not a container one — it will move, and this PR deliberately does not edit that note, which refuses to carry a figure anyone has not re-measured on a real run of the step. - `deadHintSweep` is the next-largest single stage in a derivation (~5.7 s of the remaining 13.2 s). It runs once per derivation, so there is nothing redundant to collapse; making it cheaper would be an optimisation of what it computes, which is a different card and a different risk. --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01HZfg2AwVX191qCizp88gQr --- _Generated by [Claude Code](https://claude.ai/code/session_01HZfg2AwVX191qCizp88gQr)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 917b87e commit 47bfcd0

2 files changed

Lines changed: 212 additions & 8 deletions

File tree

‎scripts/pm/check-dispatch-gates.mjs‎

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,28 @@
3131
* for this same file) — see #14281 for the reading that motivated this
3232
* section.
3333
*
34+
* ## What #18201 changed about that, and what it did NOT
35+
*
36+
* The section above was written against a battery that spent most of its wall
37+
* clock discovering the SAME tree over and over: a profile of it found every
38+
* child derivation running the discovery twice, and the live cases in the
39+
* parent driving it a further twenty-odd times, each pass re-reading every
40+
* workflow and re-masking every gate source for bytes that had not changed.
41+
* #18201 collapsed those to one pass per tree per process — ⛔ not one case
42+
* fewer, ⛔ not one assertion weaker, and the tool's output byte-identical.
43+
* The battery now fits inside a quiet container's cap with room to spare, and
44+
* the figures for it belong to that card and its PR rather than to this
45+
* paragraph.
46+
*
47+
* ⛔ That is NOT a licence to drop the detached form. The cap is a property of
48+
* the CALLER's container and the margin is a property of how contended it is,
49+
* neither of which this file can see — and an agent box runs several agents
50+
* at once. So: detached is still the form that cannot be cut off, and the
51+
* `result.signal` branch below now spends its one chance on the remedy rather
52+
* than on naming the signal. What the collapse bought is that a foreground run
53+
* on a quiet box reaches a verdict at all, where before it could only ever be
54+
* killed.
55+
*
3456
* ## Why the gate exists
3557
*
3658
* scripts/pm/dispatch-gates.mjs derives the "local gates for this card" line of
@@ -201,14 +223,41 @@ const SURFACE_MODULE = 'scripts/i18n-bundle-surface.mjs';
201223
*/
202224
const FRAME_MODULE = 'scripts/check-skill-frame-sync.mjs';
203225

226+
const started = Date.now();
204227
const result = spawnSync(process.execPath, [join(ROOT, TOOL), '--self-test'], { stdio: 'inherit' });
228+
/**
229+
* What the battery cost on THIS box, printed rather than frozen anywhere.
230+
*
231+
* The header above refuses to carry a figure and says why: a reading belongs
232+
* to a named commit, not to a comment. A reading taken at RUN TIME belongs to
233+
* the run that took it, which is the one shape that cannot rot — and it is
234+
* what a caller needs, because the cap this file's first section is about is
235+
* a property of the caller's container and not of this battery.
236+
*/
237+
const seconds = ((Date.now() - started) / 1000).toFixed(1);
205238

206239
if (result.error) {
207240
console.error(`✗ check:pm-dispatch-gates: could not run ${TOOL} — ${result.error.message}`);
208241
process.exit(2);
209242
}
210243
if (result.signal) {
211-
console.error(`✗ check:pm-dispatch-gates: ${TOOL} --self-test was killed by ${result.signal}.`);
244+
// ⛔ A kill is NOT a verdict, and this branch is the only place that can
245+
// say so before a reader reaches for the cases that did print. It answers
246+
// the question a killed caller actually has — what do I do now — rather
247+
// than naming the signal and stopping, which is what it used to do.
248+
console.error(
249+
`✗ check:pm-dispatch-gates: ${TOOL} --self-test was killed by ${result.signal} after ${seconds}s — NOTHING was measured.`,
250+
);
251+
console.error(
252+
' Every case decided before the kill is in the output above and every case after it is unjudged, so this run' +
253+
' grades neither the tool nor your diff. ⛔ Do not record it as a run.',
254+
);
255+
console.error(
256+
" Remedy — detach it and read the log, the invocation this file's header prescribes for a capped container:",
257+
);
258+
console.error(' nohup pnpm check:pm-dispatch-gates > pm-dispatch-gates.log 2>&1 &');
259+
console.error(' then tail that log until it stops growing. CI runs this step with no such cap.');
212260
process.exit(2);
213261
}
262+
console.error(`check:pm-dispatch-gates: the battery took ${seconds}s on this box.`);
214263
process.exit(result.status ?? 2);

‎scripts/pm/dispatch-gates.mjs‎

Lines changed: 162 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8463,6 +8463,36 @@ export function watchHintTree(files = trackedFiles()) {
84638463
return { files: new Set(files), prefixes: trackedPrefixes(files) };
84648464
}
84658465

8466+
/**
8467+
* ── The repository's OWN corpus, listed once per process (#18201) ───────────
8468+
*
8469+
* The listing and the tree built from it, for the one tree this process is
8470+
* about: the checkout `ROOT` names. Every caller that wants "this repo" takes
8471+
* the SAME two objects from here, which is the "one read, N answers"
8472+
* discipline `watchHintTree`'s own docblock states, promoted from an
8473+
* invariant each caller had to keep by hand to one the module keeps for them.
8474+
*
8475+
* ⛔ It is NOT a general tree cache. A caller that means a DIFFERENT tree — a
8476+
* temporary repository, a hand-built fixture listing, `null` — still builds
8477+
* and passes its own, and nothing here answers for it; the memo below is keyed
8478+
* on the tree object, so a fixture tree can never be served this one's answer.
8479+
*
8480+
* What makes the reuse observationally identical rather than merely cheaper:
8481+
* within one process nothing writes to `ROOT`. The CLI is one-shot, and the
8482+
* self-test's every write goes to a `mkdtemp` directory under the system temp
8483+
* root — the in-tree-fixture class its own cases refuse. A process that did
8484+
* mutate the checkout under itself would need a fresh listing, and would ask
8485+
* `trackedFiles()` for one, which is untouched.
8486+
*/
8487+
let repoCorpusMemo = null;
8488+
export function repoCorpus() {
8489+
if (repoCorpusMemo === null) {
8490+
const files = trackedFiles();
8491+
repoCorpusMemo = { files, tree: watchHintTree(files) };
8492+
}
8493+
return repoCorpusMemo;
8494+
}
8495+
84668496
/**
84678497
* The longest leading run of a hint's segments that the tree still has, or ''
84688498
* when even its first segment names nothing.
@@ -10994,7 +11024,58 @@ export function tierLines(result) {
1099411024
* is the drift this file's header refuses everywhere else. The self-test is
1099511025
* the only other caller, and it calls THIS.
1099611026
*/
10997-
export function discoverFamilies({ tree = watchHintTree() } = {}) {
11027+
/**
11028+
* ── ONE discovery pass per tree, per process (#18201) ──────────────────────
11029+
*
11030+
* PROFILED, not guessed, on the same principle as the source maskers far
11031+
* above and with the same promise: ⛔ this changes nothing about WHAT is
11032+
* discovered — it is the same derivation run once instead of N times, which
11033+
* is the only kind of speed-up this tool may take.
11034+
*
11035+
* The reading that motivated it belongs to a named commit rather than to this
11036+
* comment, so it lives on the card: a V8 CPU profile of one plain derivation
11037+
* found this function running TWICE over the identical tree — once from
11038+
* `derive`, once from `gateFamilyFiles` under `changeKindGates` — and the
11039+
* self-test driving it a further twenty-odd times in one process, every pass
11040+
* re-reading every workflow and re-masking every gate source for bytes that
11041+
* cannot have changed in between.
11042+
*
11043+
* Keyed on the TREE OBJECT, which is what makes the memo observationally
11044+
* identical rather than merely cheaper: the answer is a pure function of the
11045+
* tree it is handed plus the checkout on disk, a caller that means a
11046+
* different tree hands a different object and gets its own pass, and nothing
11047+
* in this process writes to the checkout (see `repoCorpus`). A `null` tree —
11048+
* the deliberate no-tree probe — is not an object and is never memoised.
11049+
*
11050+
* ⛔ The entries this hands back are SHARED. A caller that mutates one for an
11051+
* ablation must restore it before it returns, exactly as the one self-test
11052+
* case that does so already restores `entry.reads`.
11053+
*/
11054+
const discoveryMemo = new WeakMap();
11055+
11056+
/**
11057+
* How many discovery passes this process has really computed — the reading a
11058+
* case needs to tell "memoised" from "cheap enough that nobody noticed", and
11059+
* the only way to pin a collapse whose whole symptom is the absence of work.
11060+
*/
11061+
let discoveryPasses = 0;
11062+
export function discoveryPassCount() {
11063+
return discoveryPasses;
11064+
}
11065+
11066+
export function discoverFamilies({ tree = repoCorpus().tree } = {}) {
11067+
if (tree !== null && typeof tree === 'object') {
11068+
const hit = discoveryMemo.get(tree);
11069+
if (hit !== undefined) return hit;
11070+
const value = discoverFamiliesPass(tree);
11071+
discoveryMemo.set(tree, value);
11072+
return value;
11073+
}
11074+
return discoverFamiliesPass(tree);
11075+
}
11076+
11077+
function discoverFamiliesPass(tree) {
11078+
discoveryPasses += 1;
1099811079
const wfDir = nodePath.join(ROOT, '.github/workflows');
1099911080
const workflows = readdirSync(wfDir).filter((f) => /\.ya?ml$/.test(f));
1100011081
if (workflows.length === 0) throw new Error('no workflow files found under .github/workflows');
@@ -12941,12 +13022,16 @@ function derive(paths, { showResidue = false, mode = 'human', runRecord = [] } =
1294113022
// It is read here, above the discovery, because the extractor needs the same
1294213023
// corpus to judge a single-segment directory literal — one listing, so the
1294313024
// hints and the sweep that grades them cannot be taken from different trees.
12944-
const swept = trackedFiles();
12945-
// Held in a name rather than built inline: the artifact-roster split needs
12946-
// the SAME bundle the discovery was handed. Built twice it would be two
12947-
// readings of one listing, which is the drift `watchHintTree`'s own docblock
12948-
// refuses ("the pair is meaningless apart").
12949-
const tree = watchHintTree(swept);
13025+
// Both halves come from `repoCorpus`, which lists the checkout once per
13026+
// process. Held in names rather than built inline: the artifact-roster split
13027+
// needs the SAME bundle the discovery was handed. Built twice they would be
13028+
// two readings of one listing, which is the drift `watchHintTree`'s own
13029+
// docblock refuses ("the pair is meaningless apart") — and taking them from
13030+
// the shared corpus makes the pair the same two OBJECTS rather than two
13031+
// equal copies, which is also what lets the discovery below be the same pass
13032+
// `gateFamilyFiles` gets under `changeKindGates` instead of a second one
13033+
// over an identical tree (#18201).
13034+
const { files: swept, tree } = repoCorpus();
1295013035
const { byCheck, workflows, workflowEntries } = discoverFamilies({ tree });
1295113036
// ONE per-hint sweep feeds both readers of dead literals: the unreachable
1295213037
// listing (whole-family grain) and the residue annotations (per-hint grain,
@@ -21294,6 +21379,76 @@ function selfTest() {
2129421379
const liveGaps = checkFamilyCoverageGaps(liveWorkflowEntries);
2129521380
t(`every real paths-filtered workflow discovers a check family or declares why not (gaps: ${liveGaps.join(', ') || 'none'})`, liveGaps.length === 0);
2129621381

21382+
// ── ONE discovery pass per tree, and the collapse is OBSERVED (#18201) ────
21383+
//
21384+
// The memo's whole symptom is work that does NOT happen, and absent work is
21385+
// invisible to every other case here: each of them asks what discovery
21386+
// ANSWERS, and the answer is identical either way — which is the point of
21387+
// the memo and also the reason nothing already in this file can tell a
21388+
// collapsed pass from a repeated one. So the pass counter is read directly,
21389+
// and both directions are pinned: the default tree is discovered once, and a
21390+
// tree that is NOT that object is never served its answer.
21391+
//
21392+
// ⛔ The second half is not decoration. The cheap wrong memo is one module
21393+
// slot ignoring the argument, and under it every fixture-tree case in this
21394+
// file — the directory-landing tree, the class tree, the no-tree probe —
21395+
// would be answered about the REAL tree while still reading as a pass on the
21396+
// day their expectations happen to coincide. The pin that costs a pass is
21397+
// what makes that unbuildable.
21398+
const warmDiscovery = discoverFamilies();
21399+
const passesWarm = discoveryPassCount();
21400+
const defaultAgain = discoverFamilies();
21401+
const defaultOnceMore = discoverFamilies();
21402+
t(
21403+
'the default tree is discovered ONCE per process — two further calls compute no pass',
21404+
discoveryPassCount() === passesWarm,
21405+
`passes before ${passesWarm}, after ${discoveryPassCount()}`,
21406+
);
21407+
t(
21408+
'and those calls hand back the SAME object, so an entry ablated and restored is one entry',
21409+
defaultAgain === warmDiscovery && defaultOnceMore === warmDiscovery,
21410+
);
21411+
t(
21412+
'the corpus behind it is listed once too — one bundle, so the sweep and the discovery cannot describe different revisions',
21413+
repoCorpus() === repoCorpus() && repoCorpus().tree === repoCorpus().tree,
21414+
);
21415+
t(
21416+
"and that listing is the tracked corpus itself, not a trimmed copy of it",
21417+
repoCorpus().files.length === trackedFiles().length && repoCorpus().tree.files.size === repoCorpus().files.length,
21418+
);
21419+
// The derivation's own collapse, pinned at the seam that used to pay twice:
21420+
// `derive` takes this exact tree object and `gateFamilyFiles` — reached from
21421+
// `changeKindGates`, a whole call chain away — asks for the default one. The
21422+
// two are the same object, so the second ask is the first pass.
21423+
const passesBeforeSeam = discoveryPassCount();
21424+
const seamDiscovery = discoverFamilies({ tree: repoCorpus().tree });
21425+
const seamFiles = gateFamilyFiles();
21426+
t(
21427+
'the tree `derive` hands down and the default `gateFamilyFiles` asks for are ONE pass, not two',
21428+
discoveryPassCount() === passesBeforeSeam && seamDiscovery === warmDiscovery && seamFiles.size > 0,
21429+
`passes before ${passesBeforeSeam}, after ${discoveryPassCount()}, gate files ${seamFiles.size}`,
21430+
);
21431+
// A DIFFERENT tree object, built from the very same listing: identical
21432+
// content, and still its own pass. Content is not the key — the object is —
21433+
// because a caller that built its own bundle is asking about ITS tree.
21434+
const twinTree = watchHintTree(repoCorpus().files);
21435+
const passesBeforeTwin = discoveryPassCount();
21436+
const twinDiscovery = discoverFamilies({ tree: twinTree });
21437+
t(
21438+
'a tree object that is not the corpus’s is never served its answer — same content, its own pass',
21439+
discoveryPassCount() === passesBeforeTwin + 1 && twinDiscovery !== warmDiscovery,
21440+
`passes before ${passesBeforeTwin}, after ${discoveryPassCount()}`,
21441+
);
21442+
// The no-tree probe is not an object, so it cannot be a key at all — and a
21443+
// memo that tried would throw rather than answer. It computes every time.
21444+
const passesBeforeNull = discoveryPassCount();
21445+
const nullDiscovery = discoverFamilies({ tree: null });
21446+
t(
21447+
'the no-tree probe is never memoised — it is not an object, and it still answers',
21448+
discoveryPassCount() === passesBeforeNull + 1 && nullDiscovery !== warmDiscovery && nullDiscovery.byCheck.size > 0,
21449+
`passes before ${passesBeforeNull}, after ${discoveryPassCount()}`,
21450+
);
21451+
2129721452
// ── The reachability sweep — the third verdict (#9883) ────────────────────
2129821453
//
2129921454
// The verdict answers a question about the TREE, so both halves are pinned:

0 commit comments

Comments
 (0)