Add BRANCH_TAKEN and BRANCH_NOT_TAKEN events to sys.monitoring

[markshannon]

Feature 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

• GH-122548: Implement branch taken and not taken events for sys.monitoring #122564
• GH-122548: Correct magic number comment #128115

Tasks

Preview Give feedback

1. sys.monitoring branches for match cases showing incorrectly #123044
   interpreter-core type-feature +2
2. sys.monitoring branch for match case mapping to None #123048
   interpreter-core type-bug +2
   
3. Certain sys.monitoring "not taken" branches in a for loop not showing correctly #123050
   interpreter-core type-feature +2
4. Performance of instrumented FOR_ITER is poor as DISABLE doesn't remove the instrumentation. #128375
   interpreter-core type-feature +2
5. NOT_TAKEN instructions need to be added after optimization #128533
   interpreter-core type-feature +2
6. Find a way to handle branch events in async for #128534
   interpreter-core type-feature +2

[markshannon]

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.

[jaltmayerpizzorno]

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]

Fixed.
In future, could you report issues with PRs on the PR? It gets hard to track them otherwise.
Thanks.

[jaltmayerpizzorno]

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]

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 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]

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.

[nedbat]

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]

If we can't map the collected data back to a user-understandable interpretation...

See #123050 (comment)

[nedbat]

(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_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.