Skip to content

resolve sysmon branch events lazily, one pair at a time - #2221

Merged
nedbat merged 2 commits into
coveragepy:mainfrom
reaperhulk:claude/sysmon-lazy-branch-resolver
Jul 12, 2026
Merged

nedbat merged 2 commits into
coveragepy:mainfrom
reaperhulk:claude/sysmon-lazy-branch-resolver

Conversation

@reaperhulk

Copy link
Copy Markdown
Contributor

Like #2220, this PR was generated via claude but has undergone significant review and changes based on that review before submission.

sysmon branch mode currently pre-analyzes every branch site in a code object on that object's first branch event. This is two dis.get_instructions() decodes plus trail walks per conditional jump. Since branch events are one-shot (each site/direction is DISABLEd after first fire), at most two (source, destination) pairs per site ever need resolving, so this PR drops the precompute and resolves each pair lazily as its event arrives by walking the raw co_code bytes from the destination; branch_trails(), always_jumps(), and InstructionWalker are removed.

This results in a quite significant performance improvement in cryptography's test suite (these numbers do not include the other performance optimizations that have recently landed as I ran them before rebasing to main)

Configuration Wall time Overhead
No coverage 35.44 s —
sysmon branch, main 47.90 s +35.2%
sysmon branch, this PR 38.33 s +8.2%

In conjunction with #2220 it looks like we'll have branch coverage down to a similar overhead as line coverage with the sysmon core.

I'm very open to ideas on how we can improve the confidence in correctness/reviewability for this! 😄 I've run it against both this branch and latest coverage.py on several suites without seeing regression, but LLM-driven significant refactors like this (even with ongoing human feedback during the process, as this had) make me nervous since I lack a deep understanding. I've reviewed this and at the very least it isn't clearly wrong, but that's not a sufficient bar.

@reaperhulk
reaperhulk force-pushed the claude/sysmon-lazy-branch-resolver branch from a218004 to 20f0c96 Compare July 10, 2026 17:12
@nedbat

nedbat commented Jul 12, 2026

Copy link
Copy Markdown
Member

I'm disheartened to realize again how most of bytecode.py is run inside the tracers, and so shows as not covered in the coverage reports. I'd like to have direct tests of that code. But that's not the fault of this PR.

@nedbat

nedbat commented Jul 12, 2026

Copy link
Copy Markdown
Member

Now this has a complicated merge conflict... :(

On the first BRANCH_LEFT/RIGHT event of every code object, the sysmon
core precomputed branch trails for every branch site in the code object:
two full dis.get_instructions() decodes (branch_trails and always_jumps
each built their own InstructionWalker), two trail walks per conditional
jump, and re-registration of every trail under every offset it contains.
On test-suite-shaped workloads — thousands of code objects, most
branches executed a handful of times — this analysis dominates: branch
mode on the pyca/cryptography suite (4,277 tests, 5,671 traced code
objects) costs +35% vs +2% for line mode (issue coveragepy#2172).

But branch events are one-shot: each (site, direction) fires once and is
then DISABLEd, so at most two pairs per branch site ever need resolving,
and only for branches that actually execute.  Replace the precompute
with a BranchArcResolver that resolves each (source offset, destination
offset) pair as it arrives, walking the raw co_code bytes from the
destination — following unconditional jumps, skipping CACHE and
EXTENDED_ARG, stopping at a new source line (arc), a return (arc to
exit), or another branch possibility (no arc: that branch produces its
own events).  Lines come from the byte_to_line dict the tracer already
builds, so nothing in the branch measurement path disassembles whole
code objects any more.  branch_trails(), always_jumps(), and
InstructionWalker become unused and are removed.

Computing jump targets needs the number of inline CACHE entries that
follow the jump instruction (jump distances are measured from the end
of the caches).  Rather than reading opcode._inline_cache_entries — a
private table whose shape has already changed once (list in 3.11, dict
in 3.12) — the resolver counts the CACHE opcodes directly in co_code:
exact by construction, self-consistent with the code object being
walked, and no private API.

Measured on the cryptography suite (Python 3.14.2, branch mode, wall
time best of 3, base 35.44s): 47.90s (+35.2%) before, 38.33s (+8.2%)
after; combined with the sysmon-multiline-map branch the overhead is at
the run-to-run noise floor (~0-2%).  Correctness verified by: coverage's
own test suite under COVERAGE_CORE=sysmon on 3.14 (identical failure set
to main, all environmental); resolving 17,445 (source, dest) pairs
across 3,571 code objects identically to a dis-based reference
implementation; and byte-identical 'coverage report' output on the
cryptography suite (26,376 statements, 2,484 branches).  The raw arc
data differs by five arcs on that suite: the old code's byte_to_line
fallback recorded arcs between two lines inside the same multi-line
statement, which the resolver correctly attributes to the statement's
first line; all five collapse to self-arcs at report time.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_0137DLbSXfm5v5bhCcz7xEKM
@reaperhulk

Copy link
Copy Markdown
Contributor Author

Resolving the branch conflict right now. I think we can improve coverage on bytecode.py as well, but would you prefer that in this PR or as a follow-up?

@reaperhulk
reaperhulk force-pushed the claude/sysmon-lazy-branch-resolver branch from 20f0c96 to f1a1a64 Compare July 12, 2026 19:33
@nedbat
nedbat merged commit 182b010 into coveragepy:main Jul 12, 2026
42 checks passed
@nedbat

nedbat commented Jul 12, 2026

Copy link
Copy Markdown
Member

This is now released as part of coverage 7.15.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants