Repository navigation
resolve sysmon branch events lazily, one pair at a time - #2221
Merged
nedbat merged 2 commits intoJul 12, 2026
Merged
Conversation
reaperhulk
force-pushed
the
claude/sysmon-lazy-branch-resolver
branch
from
July 10, 2026 17:12
a218004 to
20f0c96
Compare
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. |
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
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
force-pushed
the
claude/sysmon-lazy-branch-resolver
branch
from
July 12, 2026 19:33
20f0c96 to
f1a1a64
Compare
Member
|
This is now released as part of coverage 7.15.1. |
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.
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(), andInstructionWalkerare 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)
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.