test(benchmark): bound calls per parse, not seconds (#475) - #476
Merged
Conversation
The 1.0s wall-clock bound failed four times across #466 and #474, always on CI's 3.12 job, always between 1.01s and 1.08s, with master re-running green each time. Three local methods disagreed with CI and with each other: uninstrumented ~1% branch-over-parent (inside the parent's own spread), coverage-instrumented indistinguishable, and the whole benchmark file under --cov made the branch FASTER. The same harness measured origin/master at 94-96ms and again at 89-92ms twenty minutes later. A check that cannot separate a 1% change from a busy runner does not fail safe; it fails expensively. Call counts are deterministic. Budgets: 470 for parse(), 512 for HumanName(), set at 408.4 and 445.4 with 15% headroom. A loose 5s backstop stays for what call counts cannot see -- a stage that gets slower without calling anything more. WHAT THE OLD BOUND CAUGHT IS RECORDED, NOT LOST. Against the v2.1.0 tag, back to back on one machine: 79.1ms -> 97.0ms for 1000 facade parses, +23%, and 386.4 -> 441.4 calls per parse. The bound was not too tight; the parser had grown into it, and the clock could not say so with enough confidence to act on. The largest contributor is a correctness decision rather than waste: #439 extracted the piece-level predicates so group and assign would share one implementation (mechanisms.md#ONE-PREDICATE-PER-QUESTION, which exists because two copies of "is this a title piece" drifted and caused bugs). That turned inline expressions into ~51 calls per parse while _group shed 8. The leaves are two membership tests each; there is no hotspot, and recovering the cost means inlining them back at hot sites, reintroducing the drift #439 removed. Not taken -- recorded in decisions.md#parse-cost with the per-stage and per-module numbers. One repair was tried and reverted: collapsing post_rules' three role- index scans into one pass saved ONE call per parse, because those sites only run when their rule fires. Recorded so it is not re-tried, and as the reason the budget is the instrument -- a harness with +/-10% noise cannot tell a one-call change from a fifty-call one. The budget binds: 80 synthetic calls per parse trips both tests with the actual number in the message.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #476 +/- ##
=======================================
Coverage 98.66% 98.66%
=======================================
Files 45 45
Lines 3214 3214
=======================================
Hits 3171 3171
Misses 43 43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Rebuilt after review found the instrument sound and everything around it wrong. THE HARNESS IS THE ROOT FIX. tools/perf/call_count.py now produces every number this change quotes -- calls per parse, per module, per stage, and against any git ref in its own checkout. The first draft took its figures across several sessions with throwaway scripts on whichever interpreter was to hand, and it showed: the module table spliced 3.14's before-column onto 3.11's after-column (four of eight numbers wrong), the stage figures were 2000-parse totals labelled per 1000 (contradicted by the entry's own arithmetic, four stages summing past the whole parse), and the reverted-repair figure was not even reachable. docs/design/AGENTS.md already asked for this: "give the one-liner that recomputes it". A BAND, NOT A CEILING. The first draft's 470-against-408 catches nothing smaller than +61 calls on 3.11 and +84 on 3.12 -- it would have missed the regression it was built for on four of five interpreters, because a ceiling with headroom is another threshold that happens to break. A +/-2% band trips at +5 calls, one PR's worth, verified by injection. A drop trips it too. PER INTERPRETER: 410 on 3.11, 388 on 3.12, 406 on 3.13-3.15. The first draft claimed "the same number on any machine" and took its baseline on 3.11 by luck; on 3.12 it would have sent the 3.11 job red with nothing explaining why. An unknown version now fails with its own number instead of passing unguarded. SKIP, DO NOT CLOBBER. sys.setprofile(None) cleared any pre-existing hook, silently truncating a maintainer's cProfile -- the very workflow that produced this entry's module table. Restoring is not available (getprofile hands back a Profile that setprofile refuses), so the test skips when it does not own the slot. ATTRIBUTION CORRECTED, measured against each PR's own parent: #439, which the first draft called "the largest contributor" and built a trade-off on, costs ZERO (404 -> 404, _group shedding exactly what _pieces gained). The real largest is #424 at +23 (381 -> 404). The old conclusion deterred work on a PR that spent nothing. Also: the backstop covers both entry points (both CI failures were on the facade, which the first draft left unguarded); the reference name is fixed WIDTH, so the count is integral and not a function of n; the comments no longer claim setprofile counts C calls or that a quadratic inside a generator is invisible; and the release bullet carries the recompute pointer AGENTS.md requires.
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.
The parse-cost benchmark bounds function calls per parse instead of wall-clock seconds.
Closes #475
Why the clock had to go
The 1.0s bound failed four times across #466 and #474, always on CI's 3.12 job, always between 1.01s and 1.08s — and master's own 3.12 job re-ran green each time. Three local methods, branch against its own parent, disagreed with CI and with each other: uninstrumented ~1% (inside the parent's own spread), coverage-instrumented indistinguishable, the whole benchmark file under
--covfaster. The same harness measuredorigin/masterat 94–96ms and again at 89–92ms twenty minutes later.A check that cannot separate a 1% change from a busy runner does not fail safe. It fails expensively: two merges held, and the honest answer after three measurements was "I cannot reconcile the local evidence with CI".
What replaces it
A per-interpreter band of ±2%, measured with
sys.setprofile:Per interpreter because the count is not machine-independent — PEP 709 inlined the comprehension frames 3.11 counts, and 3.13 added others back. A version with no row fails loudly with its own number rather than passing unguarded.
A band and not a ceiling, which is the correction review forced: a ceiling with headroom is another threshold that happens to break. Measured, the first draft's 470-against-408 catches nothing smaller than +61 calls on 3.11 and +84 on 3.12 — it would have missed the regression it was built for on four of five interpreters. The band trips at +5, one PR's worth, verified by injection. A drop trips it too; an unexplained fall is as much a signal as a rise.
A loose 5s wall-clock backstop stays over both entry points — both CI failures were on the facade, which the first draft left unguarded — for the class frame counts cannot see: a compiled regex that starts backtracking emits no
callevent. (A quadratic inside a generator is visible;callfires per resume.)The test skips rather than clobbers when something else owns the profile slot.
sys.setprofile(None)cleared any pre-existing hook, silently truncating a maintainer's cProfile — the very workflow that produced this entry's module table. Restoring isn't available:getprofile()returns aProfilethatsetprofile()refuses.The harness is in the tree
tools/perf/call_count.pyproduces every number quoted here — calls per parse,--modules,--stages, and--against <ref>measured inside that ref's own checkout.That is the root fix, not a convenience. The first draft of this PR took its figures across several sessions with throwaway scripts on whichever interpreter was to hand, and it showed: the module table spliced 3.14's before-column onto 3.11's after-column (four of eight numbers wrong), the stage figures were 2000-parse totals labelled per 1000 — contradicted by the entry's own arithmetic, four stages summing past the whole parse — and the reverted-repair figure wasn't reachable at all.
docs/design/AGENTS.mdalready asked for this: "give the one-liner that recomputes it."What the old bound had caught
Measured
--against v2.1.0on 3.11, one run:The bound was not too tight — the parser had grown into it, and the clock could not say so with enough confidence to act on.
And the attribution was backwards
The first draft named #439's predicate extraction "the largest contributor" and built a trade-off on it. Measured against each PR's own parent:
_groupshed exactly what_piecesgained, andmechanisms.md#ONE-PREDICATE-PER-QUESTIONsays the sharing predated the module. So the old conclusion — "recovering it means reintroducing the drift #439 removed, not taken" — deterred work on a PR that spent nothing while +23 from #424's mirrored peel walks went unexamined.One repair is worth reconsidering
Collapsing
post_rules' three role-index scans into one pass saves 6 calls per parse, not the 1 the first draft claimed — which wasn't even reachable, since_idxis a one-line comprehension and each removed call frees two frames. It was reverted because a timing harness with ±10% noise showed nothing. The band can see 6.Verification
git diff origin/master..HEAD -- nameparser/is empty.🤖 Generated with Claude Code