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

### Tasks
- [ ] https://github.com/python/cpython/issues/123044
- [ ] https://github.com/python/cpython/issues/123048
- [ ] https://github.com/python/cpython/issues/123050

[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.