Repository navigation
Add BRANCH_TAKEN and BRANCH_NOT_TAKEN events to sys.monitoring #122548
Description
Activity
The interactions between
BRANCHevents and theBRANCH_TAKENandBRANCH_NOT_TAKENpair of events is going to be awkward to implement and unnecessarily inefficient, given there is no use case for combining the old and new approach.So, here's the updated proposal:
- Add
BRANCH_TAKENandBRANCH_NOT_TAKENevents tosys.monitoring - Deprecate the old
BRANCHevent (as it will be redundant) - Tools should use either the old
BRANCHevent, or the newBRANCH_TAKENandBRANCH_NOT_TAKENpair. Not both. - The behavior of
BRANCH,BRANCH_TAKENandBRANCH_NOT_TAKENevents is not defined if both are used.
This is still fully backwards compatible, and will allow us to implement
BRANCHevents on top of the new events without negatively impacting their performance.Reacted by Juan Altmayer Pizzorno- Add
If I try to just use the old
BRANCHevent, I get an error:File "/Users/juan/project/slipcover/pep669.py", line 92, in enable_events sys.monitoring.set_local_events(sys.monitoring.COVERAGE_ID, co, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ sys.monitoring.events.LINE| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ sys.monitoring.events.BRANCH) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ValueError: invalid local event set 0x40020Fixed.
In future, could you report issues with PRs on the PR? It gets hard to track them otherwise.
Thanks.Reacted by Juan Altmayer PizzornoFixed. In future, could you report issues with PRs on the PR? It gets hard to track them otherwise. Thanks.
Ah, sorry about that! Ironically, that's what I intended by reporting it here -- but it was the code in #122564 that I saw that on.
Since monitoring shouldn't impact performance when not used, we don't want to restrict the bytecode compiler in its optimizations, or how it lays out code.
With that in mind, I think we need to drop the "taken"/"not taken" labels and use something like "left" and "right".
That way there is less of an implication that a particular branch direction corresponds to a particular source construct.In the code:
if cond: x y
It could be surprising if the branch from
condtoxis marked "taken" rather than "not taken", but less surprising if it is marked "right" instead of "left".I therefore proposed renaming
BRANCH_NOT_TAKENtoBRANCH_LEFTandBRANCH_TAKENtoBRANCH_RIGHT.
The triples inco_brancheswill then beorigin, left, right.I therefore proposed renaming
BRANCH_NOT_TAKENtoBRANCH_LEFTandBRANCH_TAKENtoBRANCH_RIGHT. The triples inco_brancheswill then beorigin, left, right.Ok! I've modified my code, so as far as I'm concerned, you can remove the taken/not_taken names.
Continuing from #122564 (comment) and #122564 (comment)
There is no branch from line 1 to 2, the "left" branch is from line 1 to line 1.
Maybe first we should decide: from the user's point of view, does line 1 have two possible destinations (line 2 and line 3) or does it not? Should line 1 be considered a branch for the purposes of branch coverage?
If we can't map the collected data back to a user-understandable interpretation, then we haven't finished the feature. Can you help me puzzle through this? I'd like to get efficient branch coverage measurement. So far, we seem to be talking about different things (byte codes vs lines) and we aren't meeting in the middle.
If we can't map the collected data back to a user-understandable interpretation...
(Moved here from #128445 (comment))
Two more examples of branch event surprises. run_sysmon.py is used to see the sys.monitoring events. These are using commit cbe6c7c from PR #128445.
ifdebug.py
for value in [True, False]: if value: if __debug__: x = 4 else: x = 6
Disassembled:
% python3 -m dis -O ifdebug.py 0 0 RESUME 0 1 2 LOAD_CONST 0 ((True, False)) 4 GET_ITER L1: 6 FOR_ITER 18 (to L3) 10 STORE_NAME 0 (value) 2 12 LOAD_NAME 0 (value) 14 TO_BOOL 22 POP_JUMP_IF_FALSE 6 (to L2) 26 NOT_TAKEN 3 28 NOT_TAKEN 4 30 LOAD_SMALL_INT 4 32 STORE_NAME 1 (x) 34 JUMP_BACKWARD 16 (to L1) 6 L2: 38 LOAD_SMALL_INT 6 40 STORE_NAME 1 (x) 42 JUMP_BACKWARD 20 (to L1) 1 L3: 46 END_FOR 48 POP_ITER 50 LOAD_CONST 1 (None) 52 RETURN_VALUEEvents:
3.14.0a3+ (heads/pr/128445:cbe6c7c085c, Jan 4 2025, 05:39:38) [Clang 16.0.0 (clang-1600.0.26.6)] PY_START: ifdebug.py@0 #0 LINE: ifdebug.py #1 BRANCH_LEFT: ifdebug.py@6->10 #1->1 LINE: ifdebug.py #2 BRANCH_LEFT: ifdebug.py@22->28 #2->3 LINE: ifdebug.py #3 BRANCH_LEFT: ifdebug.py@28->30 #3->4 **** LINE: ifdebug.py #4 JUMP: ifdebug.py@34->6 #4->1 LINE: ifdebug.py #1 BRANCH_RIGHT: ifdebug.py@22->38 #2->6 LINE: ifdebug.py #6 JUMP: ifdebug.py@42->6 #6->1 BRANCH_RIGHT: ifdebug.py@6->50 #1->1 PY_RETURN: ifdebug.py@52 #1The starred line is a BRANCH event from a NOT_TAKEN instruction.
1176_async.py
import asyncio async def async_gen(): yield 4 async def async_test(): async for i in async_gen(): print(i + 8) asyncio.run(async_test())
Disassembled:
% python3 -m dis -O 1176_async.py 0 0 RESUME 0 1 2 LOAD_SMALL_INT 0 4 LOAD_CONST 0 (None) 6 IMPORT_NAME 0 (asyncio) 8 STORE_NAME 0 (asyncio) 3 10 LOAD_CONST 1 (<code object async_gen at 0x103781c50, file "1176_async.py", line 3>) 12 MAKE_FUNCTION 14 STORE_NAME 1 (async_gen) 6 16 LOAD_CONST 2 (<code object async_test at 0x1037c5b80, file "1176_async.py", line 6>) 18 MAKE_FUNCTION 20 STORE_NAME 2 (async_test) 10 22 LOAD_NAME 0 (asyncio) 24 LOAD_ATTR 6 (run) 44 PUSH_NULL 46 LOAD_NAME 2 (async_test) 48 PUSH_NULL 50 CALL 0 58 CALL 1 66 POP_TOP 68 LOAD_CONST 0 (None) 70 RETURN_VALUE Disassembly of <code object async_gen at 0x103781c50, file "1176_async.py", line 3>: 3 0 RETURN_GENERATOR 2 POP_TOP L1: 4 RESUME 0 4 6 LOAD_SMALL_INT 4 8 CALL_INTRINSIC_1 4 (INTRINSIC_ASYNC_GEN_WRAP) 10 YIELD_VALUE 0 12 RESUME 5 14 POP_TOP 16 LOAD_CONST 0 (None) 18 RETURN_VALUE -- L2: 20 CALL_INTRINSIC_1 3 (INTRINSIC_STOPITERATION_ERROR) 22 RERAISE 1 ExceptionTable: L1 to L2 -> L2 [0] lasti Disassembly of <code object async_test at 0x1037c5b80, file "1176_async.py", line 6>: 6 0 RETURN_GENERATOR 2 POP_TOP L1: 4 RESUME 0 7 6 LOAD_GLOBAL 1 (async_gen + NULL) 16 CALL 0 24 GET_AITER L2: 26 GET_ANEXT 28 LOAD_CONST 0 (None) L3: 30 SEND 3 (to L6) L4: 34 YIELD_VALUE 1 L5: 36 RESUME 3 38 JUMP_BACKWARD_NO_INTERRUPT 5 (to L3) L6: 40 END_SEND L7: 42 STORE_FAST 0 (i) 8 44 LOAD_GLOBAL 3 (print + NULL) 54 LOAD_FAST 0 (i) 56 LOAD_SMALL_INT 8 58 BINARY_OP 0 (+) 62 CALL 1 70 POP_TOP 72 JUMP_BACKWARD 25 (to L2) 7 L8: 76 CLEANUP_THROW L9: 78 JUMP_BACKWARD_NO_INTERRUPT 20 (to L6) L10: 80 END_ASYNC_FOR 82 LOAD_CONST 0 (None) 84 RETURN_VALUE -- L11: 86 CALL_INTRINSIC_1 3 (INTRINSIC_STOPITERATION_ERROR) 88 RERAISE 1 ExceptionTable: L1 to L2 -> L11 [0] lasti L2 to L4 -> L10 [1] L4 to L5 -> L8 [3] L5 to L7 -> L10 [1] L7 to L8 -> L11 [0] lasti L8 to L9 -> L10 [1] L10 to L11 -> L11 [0] lastiEvents:
3.14.0a3+ (heads/pr/128445:cbe6c7c085c, Jan 4 2025, 05:39:38) [Clang 16.0.0 (clang-1600.0.26.6)] PY_START: 1176_async.py@0 #0 LINE: 1176_async.py #1 LINE: 1176_async.py #3 LINE: 1176_async.py #6 LINE: 1176_async.py #10 PY_START: 1176_async.py@4 #6 LINE: 1176_async.py #7 PY_START: 1176_async.py@4 #3 LINE: 1176_async.py #4 LINE: 1176_async.py #8 12 JUMP: 1176_async.py@72->26 #8->7 LINE: 1176_async.py #7 PY_RESUME: 1176_async.py@12 #4 PY_RETURN: 1176_async.py@18 #4 PY_RETURN: 1176_async.py@84 #7 PY_RETURN: 1176_async.py@70 #10Here there are no BRANCH events at all. I would expect some for the
async forline, analogous to how synchronousforis evented.@markshannon Any word on the
if __debug__and async problems I mentioned above? Should they be new issues?PEP 626 says nothing about branches, just line events.
Since__debug__is a constant, likeTrueorFalse, there is no branch as far as the interpreter is concerned.
Without-O,if __debug__:is treated the same asif True:Thanks, I see what you mean about the
__debug__constant being optimized away. I'm trying to figure out how to accommodate that. Coverage.py has a rudimentary understanding of this kind of optimization. For example, it knows thatwhile True:isn't a branch, but thinks thatwhile not False:is a branch. I'm not sure how to mirror Python's optimizations effectively...I think the only thing remaining before coverage.py can do efficient branch coverage is the
asyncissue. When should I expect an update?#130569 adds branch monitoring for
async forloops.@nedbat I think that's everything done.
Is there anything else you think is needed, either code or documentation?
I think we are set on the code. Not sure about the docs, TBH.
3.14 has been released with this feature
Feature or enhancement
Proposal:
Proposal:
BRANCH_TAKENandBRANCH_NOT_TAKENevents tosys.monitoringBRANCHevent (as it will be redundant)BRANCHevents implicitly monitors both theBRANCH_TAKENandBRANCH_NOT_TAKENeventsBRANCH_TAKENevent will implicitly disable the matchingBRANCHevent, but only for the taken branchBRANCH_NOT_TAKENevent will implicitly disable the matchingBRANCHevent, but only for the not-taken branchBRANCHevent will implicitly disable the both the matchingBRANCH_TAKENandBRANCH_NOT_TAKENeventsThis is fully backwards compatible.
Has this already been discussed elsewhere?
I have already discussed this feature proposal on Discourse
Links to previous discussion of this feature:
Discussion which also links to prior discussions.
Linked PRs