GeezerVison is showing github.com — Add BRANCH_TAKEN and BRANCH_NOT_TAKEN events to sys.monitoring · Issue #122548 · python/cpython
Add BRANCH_TAKEN and BRANCH_NOT_TAKEN events to sys.monitoring #122548DescriptionFeature or enhancement
Proposal:
Proposal:
- Add
BRANCH_TAKEN and BRANCH_NOT_TAKEN events to sys.monitoring
- Deprecate the old
BRANCH event (as it will be redundant)
- Monitoring
BRANCH events implicitly monitors both the BRANCH_TAKEN and BRANCH_NOT_TAKEN events
- Disabling a
BRANCH_TAKEN event will implicitly disable the matching BRANCH event, but only for the taken branch
- Disabling a
BRANCH_NOT_TAKEN event will implicitly disable the matching BRANCH event, but only for the not-taken branch
- Disabling a
BRANCH event will implicitly disable the both the matching BRANCH_TAKEN and BRANCH_NOT_TAKEN events
This 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
### Tasks
- [ ] https://github.com/python/cpython/issues/123044
- [ ] https://github.com/python/cpython/issues/123048
- [ ] https://github.com/python/cpython/issues/123050
- [ ] https://github.com/python/cpython/issues/128375
- [ ] https://github.com/python/cpython/issues/128533
- [ ] https://github.com/python/cpython/issues/128534
Activitymarkshannon commented on Aug 1, 2024 The interactions between BRANCH events and the BRANCH_TAKEN and BRANCH_NOT_TAKEN pair 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_TAKEN and BRANCH_NOT_TAKEN events to sys.monitoring
- Deprecate the old
BRANCH event (as it will be redundant)
- Tools should use either the old
BRANCH event, or the new BRANCH_TAKEN and BRANCH_NOT_TAKEN pair. Not both.
- The behavior of
BRANCH, BRANCH_TAKEN and BRANCH_NOT_TAKEN events is not defined if both are used.
This is still fully backwards compatible, and will allow us to implement BRANCH events on top of the new events without negatively impacting their performance. Reacted by Juan Altmayer Pizzorno jaltmayerpizzorno commented on Aug 14, 2024 If I try to just use the old BRANCH event, 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 0x40020
markshannon commented on Aug 15, 2024 Fixed.
In future, could you report issues with PRs on the PR? It gets hard to track them otherwise.
Thanks. Reacted by Juan Altmayer Pizzorno jaltmayerpizzorno commented on Aug 15, 2024
Fixed. 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. markshannon commented on Aug 16, 2024 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:
It could be surprising if the branch from cond to x is marked "taken" rather than "not taken", but less surprising if it is marked "right" instead of "left".
I therefore proposed renaming BRANCH_NOT_TAKEN to BRANCH_LEFT and BRANCH_TAKEN to BRANCH_RIGHT.
The triples in co_branches will then be origin, left, right. jaltmayerpizzorno commented on Aug 16, 2024
I therefore proposed renaming BRANCH_NOT_TAKEN to BRANCH_LEFT and BRANCH_TAKEN to BRANCH_RIGHT. The triples in co_branches will then be origin, left, right.
Ok! I've modified my code, so as far as I'm concerned, you can remove the taken/not_taken names. added 2 commits that reference this issue on Dec 19, 2024GH-122548: Implement branch taken and not taken events for sys.monito… nedbat commented on Dec 22, 2024 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. markshannon commented on Dec 31, 2024
If we can't map the collected data back to a user-understandable interpretation...
See #123050 (comment) nedbat commented on Jan 6, 2025 (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 = 6Disassembled:
% 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_VALUE
Events:
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 #1
The 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] lasti
Events:
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 #10
Here there are no BRANCH events at all. I would expect some for the async for line, analogous to how synchronous for is evented.
nedbat commented on Feb 1, 2025 @markshannon Any word on the if __debug__ and async problems I mentioned above? Should they be new issues? markshannon commented on Feb 3, 2025 if __debug__ was fixed by #128533, I believe. Please let us know if you are still seeing issues.
The async for issue is tracked in #128534.
Both issues are listed in the "tasks" section above. nedbat commented on Feb 4, 2025 Sorry, I am getting confused. I know I said "#3->4" was wrong, but now I'm thinking I need to see that branch reported. Shouldn't PEP626 require that it be reported? markshannon commented on Feb 6, 2025 PEP 626 says nothing about branches, just line events.
Since __debug__ is a constant, like True or False, there is no branch as far as the interpreter is concerned.
Without -O, if __debug__: is treated the same as if True: nedbat commented on Feb 7, 2025 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 that while True: isn't a branch, but thinks that while not False: is a branch. I'm not sure how to mirror Python's optimizations effectively... nedbat commented on Feb 20, 2025 I think the only thing remaining before coverage.py can do efficient branch coverage is the async issue. When should I expect an update? markshannon commented on Feb 26, 2025 #130569 adds branch monitoring for async for loops. markshannon commented on Feb 27, 2025 @nedbat I think that's everything done.
Is there anything else you think is needed, either code or documentation? nedbat commented on Mar 3, 2025 I think we are set on the code. Not sure about the docs, TBH. markshannon commented on Jan 5, 2026 3.14 has been released with this feature
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|