GeezerVison

Do I understand correctly that the CodeLike type is expected to be provided by user code?

I think we should provide CodeLike in some form, but for now I put it in the text file so we can talk about how and where we want it to be. The offset to location mapping should also be part of this.

I don't see why this public API should live in an internal header file. And why the underscore names? We only use(d) them for non-public functions.

@markshannon did you intend for these to be private?

@markshannon did you intend for these to be private?

My thinking was that it is better to keep any function private unless we need it to be public. It sounds like they need to be public.

How about this?

• The declaration of the implementation function, _PyMonitoring_FirePyStartEvent(), goes in the semi-private cpython/ folder.
• We expose an inline function calling for the unstable API and a non-inline one for the stable API in the public header.

Something like this:

#ifndef Py_LIMITED_API
static inline int
PyMonitoring_FirePyStartEvent(PyMonitoringState *state, PyObject *codelike, uint32_t offset)
{
    if (state->active)
        return _PyMonitoring_FirePyStartEvent(state, codelike, offset);
    else
        return 0;
}
#else
extern int
PyMonitoring_FirePyStartEvent(PyMonitoringState *state, PyObject *codelike, uint32_t offset);
#endif

The "code like" object is entirely opaque to the monitoring machinery. As far as it is concerned, a location is a pair of an object and an integer index.
Debuggers, profilers, etc need to present that object, int as a meaningful location. If the object is a code object, they can already do that using co.co_filename and co.co_positions().
The point of a "code like" object is to provide the necessary interface for tools (debuggers, profilers, etc) to turn object, int into a meaningful source location.

So, no C struct, but a Python abstract base class.

Looking through the code for the interpeter implementation of firing events about exceptions, it looks like the exception is usually passed explicitly.

And considering that the concept of there being one (and only one) "current exception" doesn't always make sense, I think all API functions for events where the callback receives the exception should have an exception parameter.

Looking through the code for the interpeter implementation of firing events about exceptions, it looks like the exception is usually passed explicitly.

What do you mean? I see all those functions call do_monitor_exc without an exception, and it calls PyErr_GetRaisedException to get the exception.

What do you mean?

Good question. I mean that the exception is passed explicitly to any callback function.

From PEP 669:

func(code: CodeType, instruction_offset: int, exception: BaseException) -> DISABLE | Any

do_monitor_exc may not take an exception as an argument, but it calls _Py_call_instrumentation_arg() which does.

Passing the exception as an argument might require the user of the API to do a bit more work, but it removes a lot of uncertainty about the "current exception".

For example, consider a C extension that is about to raise an exception. To raise an exception, it must have a reference to that exception. Since it has a reference to the exception is straightforward to pass it as an argument:

PyObject *exception; /* Strong reference to the exception to be raised */
if (PyMonitoring_FireRaiseEvent(state, codelike, offset, exception) < 0) {
    /* Would could chain the exceptions, but I'm lazy, so I'm just going to discard it */
    Py_DECREF(exception);
    return NULL;
}
PyErr_SetRaisedException(exception);
return NULL;

The c api also passes the exception to the callbacks. It's the Fire functions that we removed it from.

We need to merge this pretty much now, so last call for strong objections. Otherwise we can iterate via bugfixes.

No strong objections.

the only events that should happen with the current exception set are RAISE, RERAISE, STOP_ITERATION, C_RAISE and UNWIND.

Does STOP_ITERATION really belong into this group? Isn't the idea rather that generators do not need to create a StopIteration in order to save time on termination?

Does CPython really create a StopIteration instance here just for monitoring it?

EDIT: it seems that it does:

That said, this is a detail that can still be decided during the beta phase, so feel free to merge this PR as it is if there's remaining need for discussion.

Does CPython really create a StopIteration instance here just for monitoring it?

Yes, but only if you ask for it (it has zero cost if you don't ask for it, thanks to the adaptive interpreter)

https://github.com/python/cpython/blob/main/Python/bytecodes.c#L291

It seems that you already found that 🙂

Does CPython really create a StopIteration instance here just for monitoring it?

Yes, but only if you ask for it (it has zero cost if you don't ask for it, thanks to the adaptive interpreter)

Ok … then I think it would be best if CPython's monitoring infrastructure created a StopIteration instance automatically when it detects that it needs to notify about it without having one. It's in the best position to detect whether it really needs an exception instance or not. Event sources shouldn't be forced to a) figure that out themselves or b) create an instance just for it to be thrown away.

Basically, move the existing code from the byte code handlers all the way to the other side into the notification mechanism.

Basically, move the existing code from the byte code handlers all the way to the other side into the notification mechanism.

That might suggest that the signature for the StopIteration event should not receive an exception or use the current one, but should better receive the StopIteration value directly (with or without a live exception).

I'll merge, please create an issue to iron out the StopIteration business.

I'll merge, please create an issue to iron out the StopIteration business.

Or discuss it on #111997.

In order to avoid any confusion about blocker tickets during the beta release, I created a new ticket to discuss the StopIteration behaviour/functions here: #118692