feat(benchmarks): gate the layout pass on exact measurement counts - #469
Merged
Conversation
Nothing in CI could tell a slower engine from a slower machine. The timing smoke run compares against absolute millisecond ceilings set about three times above observed values, so a regression that stays under the ceiling passes, and no job compares a branch against its own base. MeasurementCountBenchmark already answered that question deterministically and nothing ran it: it wraps the text measurement system in a counting decorator and reports, as exact integers, how many width measurements a document costs. Those counts are identical on every machine and move only when the engine changes. MeasurementCountGateTest now asserts them per scenario. It lands in the module whose whole test suite CI already runs as the deterministic-gates step, so it gates every performance-relevant pull request without a workflow change. A second test requires each probe scenario to carry a recorded expectation, so a new scenario cannot be measured and silently ungated. The gate is complementary to the layout snapshots rather than a duplicate: two implementations that place fragments identically but measure a different number of times satisfy every snapshot, and only this separates them. Verified by measuring one token twice in the wrapping loop - long-text goes from 4472 to 8552 requests and the gate fails with both records printed. The probe's scenario list moves into a shared accessor so the probe and the gate cannot drift apart. Allocation stays ungated: it is stable to about 0.1% across runs on one JVM but is a property of the JDK as much as of the engine, so an expectation recorded on one machine would be a guess about another.
Asserting inside the scenario loop stopped at the first mismatch, so a deliberate engine change surfaced one scenario's new numbers per run and the author had to re-run three more times to collect the rest. Soft assertions report all four together.
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
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.
Why
Nothing in CI could tell a slower engine from a slower machine.
The timing smoke run compares against absolute millisecond ceilings compiled into the source, set about 3x above observed values to absorb CI variance. A regression that stays under the ceiling fails nothing, memory is advisory only, and no job compares a branch against its own merge base. That gap is not theoretical: measuring the current tree against
baselines/current-speed-full.jsonreports four scenarios REGRESSED by 60-100%, and walking the release tags shows those same scenarios flat all the way back tov1.8.0. The baseline stores absolute milliseconds from different hardware, so the "regression" was the machine.MeasurementCountBenchmarkalready answered the question deterministically, and nothing ran it. It wraps the text measurement system in a counting decorator and reports, as exact integers, how many width measurements a document costs.What
MeasurementCountGateTestasserts those counts per scenario — width requests, distinct requests, summed and longest measured characters, line-metric lookups, and page count.It lands in the module whose entire test suite CI already runs as the Run deterministic benchmark gates step, so it gates every performance-relevant PR with no workflow change. A second test requires every probe scenario to carry a recorded expectation, so a scenario added later cannot be measured and silently ungated — the same shape as
CurrentSpeedScenarioGateTest.Complementary to the layout snapshots, not a duplicate: two implementations that place fragments identically but measure a different number of times satisfy every snapshot, and only this gate separates them.
The probe's scenario list moves into a shared package-private accessor so the probe and the gate cannot drift apart, and
measureScenariobecomes package-private static so the gate drives the real compile instead of re-implementing the instrumentation.Allocation stays ungated. It is stable to about 0.1% across runs on one JVM, but it is a property of the JDK as much as of the engine, and the perf job runs a single fixed image while the expectation would be recorded elsewhere — so it would be a guess about another machine, which is the mistake this PR exists to stop making. The probe still reports it.
Tests
Counts verified deterministic first: three consecutive probe runs on this tree produce bit-identical values in every count column.
Mutation-checked. Measuring one token twice in the paragraph wrapping loop takes
long-textfrom 4472 to 8552 width requests, and the gate fails with both records printed:Benchmarks module 44/44. Full eight-module reactor
clean verify— BUILD SUCCESS. The probe's own output is unchanged after the refactor.