Skip to content

test(benchmark): bound calls per parse, not seconds (#475) - #476

Merged
derek73 merged 2 commits into
masterfrom
fix/475-benchmark-bound
Aug 31, 2026
Merged

test(benchmark): bound calls per parse, not seconds (#475)#476
derek73 merged 2 commits into
masterfrom
fix/475-benchmark-bound

Conversation

@derek73

@derek73 derek73 commented Aug 31, 2026

Copy link
Copy Markdown
Owner

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 --cov 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: 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:

parse()      410 on 3.11    388 on 3.12    406 on 3.13–3.15
HumanName()  447            425            443

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 call event. (A quadratic inside a generator is visible; call fires 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 a Profile that setprofile() refuses.

The harness is in the tree

tools/perf/call_count.py produces 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.md already asked for this: "give the one-liner that recomputes it."

What the old bound had caught

Measured --against v2.1.0 on 3.11, one run:

parse()      343 → 410        +67 calls per parse
HumanName()  380 → 447

per 1000 parses:   group 13.5 → 22.2ms    classify 11.6 → 13.5
                   assign 11.1 → 13.0     post_rules 5.2 → 8.0

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:

#424 381 → 404 +23 — the real largest
#404 +12
#361, #367, #434 +8 each
#439 404 → 404 0 — a pure relocation

_group shed exactly what _pieces gained, and mechanisms.md#ONE-PREDICATE-PER-QUESTION says 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 _idx is 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

  • 6187 → 6188 passed; mypy, ruff and 232 sphinx doctests clean; gate green at 1.4.0 / 2.0.0 / 2.1.0.
  • Band-binding verified by injection at +5 and +9 calls per parse.
  • CI green on all five interpreters, which is what confirms the 3.12 / 3.13 / 3.15 baselines — those three were seeded from a review measurement and could not be checked on the author's machine. A wrong seed fails with the real number.
  • No behavior change: git diff origin/master..HEAD -- nameparser/ is empty.

🤖 Generated with Claude Code

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.
@derek73 derek73 added this to the v2.3 milestone Aug 31, 2026
@derek73 derek73 self-assigned this Aug 31, 2026
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.66%. Comparing base (b4eeb93) to head (bb88b51).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.
@derek73 derek73 modified the milestones: v2.3, v2.2 Aug 31, 2026
@derek73
derek73 merged commit 865e6bf into master Aug 31, 2026
11 checks passed
@derek73
derek73 deleted the fix/475-benchmark-bound branch September 1, 2026 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The 1000-parse benchmark has a fixed 1.0s wall-clock bound and has blocked two merges on runner variance

1 participant