docs(knowledge): split the load-performance record from the measurement protocol - #194
Merged
Conversation
…nt protocol The previous attempt at this record accumulated thirty-six commits and sixty-plus review findings without the rate falling, for two structural reasons. First it grew an executable benchmark harness — credential stripping, whole-home cloning, symlink materialization, destructive cleanup, signal handling, source mutation — inside a public Markdown file with no fixtures and nothing CI could run. Second, and the one that kept it going: it was allowed to become the acceptance evidence for a vendor pin advance, and an empirical validity claim has no natural floor. Each round could correctly ask about the next confounder, and the supply of those questions is not finite. This replaces it with two documents that do not share a role. `history/dashboard-load-performance.md` records what was measured: the three symptoms and their real causes, six disproved hypotheses each with the premises its conclusion depends on, the cold and warm profiles, the three shipped changes, and the engine batching that is not in the app. It states in its opening that it is not the pin's acceptance evidence, which is what stops a protocol change from forcing a recomputation of history. `measurement.md` is new and canonical: oracle design, the timing and correctness protocols, corpus preconditions, and the self-deceptions that produced them. It can be tightened without invalidating a single recorded number. The claims are graded rather than uniformly asserted. Correctness is the strong result — four arms, frozen corpus, NEW sampled twice from the same revision, all equal, under an oracle proven sensitive by four mutations and a negative control. The speedup is stated as direction-certain and magnitude-approximate, with the thermal carryover bias quantified by order block and the conservative reading named. Peak RSS is stated as having no usable data, since the only two complete pairs show NEW higher. Both are wired into the knowledge index and the history index. `make check-docs` passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5bb2608e8
ℹ️ 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".
Three findings with one root cause: each requirement was written down without being written strongly enough to bind. Fixed together rather than one at a time. **Schedule stability is not the benchmark's job.** The protocol asked for `NEW-a == NEW-b` as evidence that a parallel implementation is stable. Two samples only lower the odds of a lucky pass; they do not change its kind, and a benchmark over a static corpus cannot manufacture the scheduling divergence it is supposed to detect. The protocol now assigns the work explicitly: deterministic fixtures in the engine carry ordering and dedup behaviour at batch boundaries and must be shown to fail under mutation; the digest comparison carries output equivalence on real data; the no-op pair carries the oracle. `NEW-a == NEW-b` remains as a sanity check and is labelled as one. **Presenting order blocks is not controlling for thermal carryover.** An even split makes the two orders equal in count, but the second arm in each block still inherits treatment-dependent heat, so both block estimates stay biased — the bias is merely visible. The protocol now requires a washout to an equivalent thermal baseline, and keeps block presentation as a diagnostic: a large gap between blocks means the washout is too short, not that a median should be taken across it. **Peak RSS had no protocol at all**, while the history document made re-measuring it an obligation for the pin PR. Without a procedure that obligation could be satisfied by any incomparable number, which matters here because every complete pair measured so far shows the batched engine using more memory. The new section fixes the arm (same run as timing, cold invocation), the instrument (`/usr/bin/time -l` maximum resident set size rather than polling, which misses the peak), the estimator (paired, report median and worst pair), and a `≤ +25%` bound tied to the retention trade-off the batching design accepts. It states that passing a bound is not the same as not rising, and requires the actual percentage in reports. `make check-docs` passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 186a251bc6
ℹ️ 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".
…SS statistic Three findings, two of which are the same gap: a bound with no rule for applying it lets one dataset support acceptance or rejection depending on which statistic gets quoted. The timing protocol rejected single comparisons but never said how many pairs are enough or which number decides. With same-configuration runs varying by 9.4 seconds, two pairs and a median would have been enough to claim a speedup. The rule is now: **minimum four pairs, even, and the bound applies to the worst pair rather than the median.** Median passing while the worst pair fails is not a marginal pass — it is no result, and means the washout is too short or the effect is unstable. Both numbers must be reported, since quoting one leaves a reader unable to tell which case they are in. The RSS bound inherits the same rule and now says so; it previously named `≤ +25%` without saying whether the median or the worst pair had to meet it, while requiring both to be reported. Deliberately deterministic rather than a significance test. It needs no distributional assumption, and it cannot be steered after the fact by choosing an estimator. The history document still said two equal `NEW` executions prove the parallel implementation stable, which the protocol now contradicts directly. That is the duplicate-claim failure again, this time across the two documents I had just split — I changed the protocol and did not sweep its companion. The history now calls it a sanity check and points at the deterministic fixtures that carry the real proof. `make check-docs` passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7df1e76f83
ℹ️ 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".
…warm output Four findings. Two are defects in the decision rule added last commit, one is a real gap in the correctness protocol, and one is a reference I broke during the rewrite. **The rule referred to a limit the protocol never defines**, and did not require one to be fixed before sampling. The determinism it promised was therefore hollow: the same measurements could still be accepted or rejected by choosing a threshold afterwards. Limits must now be declared before sampling, in the PR or plan for that change, and the defaults this project uses for engine work are named so a reader is not inventing them. **"Add more pairs" cannot rescue a failing worst pair.** When the statistic is the worst retained pair, extra samples can only leave it unchanged or worsen it, so that instruction either left an experiment permanently inconclusive or invited quietly dropping the bad pair. The only recovery is now a complete rerun after fixing the cause, with the previous set voided and recorded as having existed. **The correctness protocol compared only the miss path.** Each arm reset the cache and ran once, so no digest ever came from a run against the cache a binary had just written. Cache-hit replay is separate code — the batching change alters exact-hit handling specifically — so a regression confined to warm replay would pass a cold-only comparison completely. Each arm now also digests its warm run. **The oracle mutations were not recorded where the history sends readers.** The history document cites four mutations plus a negative control and links here; this file had only a generic procedure. The concrete table is now here — each mutation, its expectation, its result, and which earlier version of the digest it corresponds to — so the sensitivity claim can be checked rather than taken. `make check-docs` passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5576af400f
ℹ️ 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".
…cklist The oracle section requires a positive mutation and a negative control. The numbered correctness protocol — the list a future pin actually follows — asked only for a mutation that should change the digest. The checklist was weaker than the section explaining why the checklist exists, which is the shape a reader following steps rather than prose walks straight into. An oracle that wrongly folds an unordered container into its hash passes every step in that list whenever the traversal order happens to be stable across the run, and then reports a false regression the first time an equivalent order appears. The positive half cannot detect that; only the negative control can. Step 9 now requires both halves and says what the missing one catches. Swept the sibling statements while fixing it: the one-line summary in the oracle table also described sensitivity as "output changed, so the value must change", which is the same half-definition. It now carries both directions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b037532353
ℹ️ 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".
…cannot carry Two findings, both one-clause corrections with real consequences, and this closes the fixing rounds on this PR. "Minimum four pairs, even" does not produce balanced order blocks. Four `OLD→NEW` pairs satisfy both that rule and the interleaving requirement while yielding no `NEW→OLD` block at all, so order-dependent thermal carryover stays invisible and biases the worst-pair decision unobserved. The rule now requires equal counts of the two orders, with the sequence predeclared like the limits. The history document then read a conservative magnitude out of the same bias it had just documented. It offered the `OLD`-first block median as a safer figure and a 20–30% true range — but the protocol says both block estimates remain biased without a washout, and that run had none. Picking one block does not yield a conservative-but-correct number, only a differently biased one. The data support the direction; the magnitude bound is removed rather than hedged, with a pointer to re-measuring under the protocol if a magnitude is wanted. That claim is worth naming: it survived every earlier round because it sounded cautious. Choosing the smaller of two biased estimates reads as conservatism and is not — it is the same error with a humbler tone.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e54aae7abc
ℹ️ 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".
The rule said any failed arm discards its pair. That is right for preparation and environment failures and wrong for an execution failure of the binary being tested: if NEW crashes, times out or is killed intermittently under benchmark load, discarding every affected pair keeps only the successful NEW runs and still certifies the change. The failures were the reliability evidence, and the rule deleted them. Now split. Preparation and environment failures discard the pair and are recorded. An execution failure of either tested binary fails acceptance. Tolerating a bounded number requires declaring the bound in advance and reporting every attempt, so the discards are visible rather than implicit. This is a correction to a rule that is wrong as written, not an addition, which is why it lands after I had closed the fixing rounds on this PR. The sibling finding in the same round — that a warm run inherits heat from its own arm's cold run, which differs in length between the two binaries, so a washout at the arm boundary cannot equalize the warm baseline — is correct and deferred to a follow-up. It changes the structure of an arm rather than a clause.
Owner
Author
|
Deferred one finding from the latest round to #196: the warm run inherits heat from its own arm's cold invocation, and the cold runs differ in length between binaries, so an arm-boundary washout cannot equalize the warm baseline. Real, correct, and a change to the structure of an arm rather than a clause — recorded with the direction of its bias (it flatters NEW, so the warm results on #195 are not endangered by it) rather than fixed here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6eeb836c2
ℹ️ 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".
The previous commit introduced "environment failure discards the pair" as an escape hatch and did not define it observably. Under the worst-pair rule, a slow NEW arm can then be classified as "the machine was busy" after it has been seen, and the retained set passes through selective invalidation. The mechanism added to close a finding opened the next one, which is the pattern this document warns about, one commit after writing that warning. Written to close the class rather than the instance, because enumerating escape hatches is how this regress continues: **any** invalidation must be decidable without looking at the result. Conditions are declared in advance and rest on signals observable independently of the measurement — system load, temperature, other processes during sampling — not on the number that came out. Every invalidation is reported with the signal that justified it, because the count of discarded attempts is itself data. If it cannot be decided that way, the attempt stays in the set. That formulation also covers escape hatches nobody has invented yet, which is the property the previous version lacked. `make check-docs` passes; CI green on the previous head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce39edc929
ℹ️ 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".
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.
Replaces #193, which was closed rather than merged. Same investigation, restructured so that review can terminate.
Why the previous attempt did not converge
#193 reached 36 commits and 62 findings with the finding rate flat. Two structural causes, both fixed here:
It grew an executable benchmark harness inside public Markdown — credential stripping, whole-home cloning, symlink materialization, destructive cleanup, signal traps, source mutation — with no fixtures and nothing CI could run. About forty findings landed there. Corpus construction is now explicitly out of scope and lives in a private machine-local overlay.
It became the acceptance evidence for a vendor pin advance. When review showed the timing set had not used an immutable corpus, the scope-minimal answer was to record those results as directional and measure acceptance in the pin PR. Instead the measurements were re-run to make the history document itself protocol-valid — and an empirical validity claim has no natural floor. Every subsequent round could correctly ask about the next confounder: estimator, corpus identity, binary identity, arm ordering, thermal carryover, hardware premises. All legitimate; the supply is not finite.
The split
history/dashboard-load-performance.mddocs/knowledge/measurement.md(new)The history document states in its opening that it is not the pin's acceptance evidence. That single sentence is what stops a protocol tightening from voiding historical numbers.
Claims are graded, not uniformly asserted
NEWsampled twice from the same revision, all four digests equal, under an oracle proven sensitive by four mutations plus a negative control. A digest is discrete: bytes match or they do not, so machine speed, thermal state and corpus size cannot confound it.The disproved-hypotheses table now carries a per-row column naming the premises each conclusion depends on — implementation, corpus, environment — so a maintainer knows which rows a pin advance invalidates. The Rayon row is bound to
5546bd5and requires re-measurement once batching is pinned.Review
Reviewed locally by Codex gpt-5.6-sol at high effort as a whole-document structural review rather than a line-level pass. Verdict: the split holds, no document recreates the acceptance-report role, all three gradings are correct in both directions, no contradictions or divergent duplicates, and no defects to fix. That was a read-only document review; it did not re-run the benchmarks.
make check-docspasses. Both documents are wired into the knowledge index and the history index.