gh-111997: C-API for signalling monitoring events

[iritkatriel]

• Issue: C-API for signalling monitoring events #111997

[scoder]

Do I understand correctly that the CodeLike type is expected to be provided by user code? How do we guarantee its struct layout then? Why not define a public object struct for it that users can use C-struct inheritance with? (I.e. if they need more fields on their side, they can put the public struct into the first field of their own struct.)

[iritkatriel]

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]

@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

[markshannon]

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.

[scoder]

ISTM that the version passed into PyMonitoring_EnterScope() is useless

scopes need to be exited and re-entered when the code-like's execution is paused

Ah, right. I hadn't looked into generator/async functions yet for the implementation. I can see the version being helpful there. They keep their execution state, so there is a place for them to also store away their profiling state.

[markshannon]

I'm not sure what others mean by "scope", but what I mean in this context is lexical scope.
Ignoring generator and coroutines that means functions, classes and modules.
The monitoring state should be attached to the relevant object.

Take a simple function:

def f(a, b):
     return  a + b

With the enter and exit inserted it would look something like:

def f(a, b):
     ENTER(&f.version, f.state)
     tmp = a + b
     EXIT(f.state)
     return  tmp

Generator and async functions

def f(a, b):
     yield a
     yield b
     return a+b

becomes:

def f(a, b):
     ENTER(&f.version, f.state)
     tmp = a
     EXIT()
     yield tmp
     ENTER(&f.version, f.state)
     tmp = b
     EXIT()
     yield tmp
     ENTER(&f.version, f.state)
     tmp = a + b
     EXIT()
     return  tmp

[markshannon]

Sorry for not thinking of this earlier, but rather than passing PyMonitoringState *state_array, const uint8_t *event_types, Py_ssize_t length I think it would make sense to put the data together in a struct and pass a pointer to that.

typedef struct _monitoring_state_array {
    const uint8_t *event_types;
    Py_ssize_t length;
    PyMonitoringState states[1];
} PyMonitoringStateArray;

That way
PyMonitoring_EnterScope(PyMonitoringState *state_array, uint64_t *version, const uint8_t *event_types, Py_ssize_t length);
becomes
PyMonitoring_EnterScope(uint64_t *version, PyMonitoringStateArray *state_array) which is a bit neater.

[scoder]

With the enter and exit inserted it would look something like

But you're not expecting the RETURN and YIELD events to come outside of the BEGIN/EXIT scope events, are you?

I was expecting an order like BEGIN, START, LINE, RETURN, EXIT, or BEGIN, RESUME, LINE, YIELD, EXIT, i.e. BEGIN-EXIT would always surround all other events.

[markshannon]

Another thing that I noticed is that it's unclear how the tracing status and the exception state is managed. Meaning, am I (as a user) expected to disable tstate->tracing before calling into PyMonitoring_Fire*()? And/or save and restore the exception state?

The VM disables tracing, no need for you to do it.

Thanks for pointing out exception state.
Ideally, you shouldn't be calling monitoring functions with the exception set, except for those monitoring calls that are about exceptions. We clearly need to document this, though.

[scoder]

which is a bit neater

But wouldn't users end up with casting all over the place, due to the varying struct length?

[markshannon]

I was expecting an order like BEGIN, START, LINE, RETURN, EXIT, or BEGIN, RESUME, LINE, YIELD, EXIT, i.e. BEGIN-EXIT would always surround all other events.

Yes, assuming BEGIN means PyMonitoring_EnterScope and EXIT means PyMonitoring_ExitScope.
I wouldn't think of BEGIN and EXIT as events, though.

[scoder]

I wouldn't think of BEGIN and EXIT as events, though.

I meant the expected order of C-API calls to trigger events. Sorry for being unclear.

[markshannon]

I'm still writing up the initial implementation in Cython, but so far, ISTM that the version passed into PyMonitoring_EnterScope() is useless, at least for me. I cannot store it statically in C, nor in another global place (like the function object or code object). I can really only make it a local variable that is always 0 (as is the PyMonitoringState array), which defeats its purpose. Any ideas how to make actual use of it?

Add version as a field to the Cython code-object equivalent. And then pass its address to PyMonitoring_EnterScope:

struct _cython_codelike_thing {
    /* Whatever normally goes here */
    uint64_t version;
    ...

Used as follows:

     _cython_codelike_thing *this_func;
     ...
     PyMonitoring_EnterScope(&this_func->version, ...);

Does that help?

[markshannon]

which is a bit neater

But wouldn't users end up with casting all over the place, due to the varying struct length?

True, but it is probably better to have fewer arguments as it causes fewer spills around the calls.

Since we have no users, and you are the only prospective user at the moment, feel free to suggest any ergonomic or efficiency improvements you like 🙂

[scoder]

Add version as a field to the Cython code-object equivalent. And then pass its address to PyMonitoring_EnterScope:

If I move the version there, then I also need to move the monitoring state array there. Both are intended to stay in sync.

Does it matter if the monitoring state gets updated concurrently while a function is running? If I tie it to the function/generator/code object, then it will be used by all executions of the function, including concurrent ones. But in the end, it's only restricting the events that get created to the latest set of events for which there are listeners. Those events that get reported to the listeners will be selected based on the listener's current interest anyway. That's not really different from a listener changing its interest while another function is running.

So, should we expect the version+statearray pair to be tied globally to the lexical scope, not to a specific execution?

[scoder]

But wouldn't users end up with casting all over the place, due to the varying struct length?

feel free to suggest any ergonomic or efficiency improvements you like 🙂

PyMonitoring_EnterScope could be a macro that does the cast for us. Not sure if that's a great idea, given that unconditional casting can hide usage bugs. But it would make the correct usage easier.

[markshannon]

If I move the version there, then I also need to move the monitoring state array there. Both are intended to stay in sync.

Yes, they should be kept together.

Does it matter if the monitoring state gets updated concurrently while a function is running?

Updating the array, and synchronizing updates are the VMs problem, not yours.
We might need somewhere to put a lock for free-threading support, though. Without free-threading the GIL is sufficient.

So, should we expect the version+statearray pair to be tied globally to the lexical scope, not to a specific execution?

Mostly yes. Not a specific execution, but it depends what you mean by "globally". If you intend to support multiple interpreters, then it will need to be tied to a specific heap object.

[scoder]

I think this is in a "good enough" shape for merging it into beta-1. From my side, feel free to merge it. I'll continue with the implementation in Cython to make sure we have something proven to be usable for the release.

[iritkatriel]

So, state_array and a corresponding version should be allocated and used together; they must be valid from the call PyMonitoring_EnterScope to the end of the corresponding PyMonitoring_ExitScope. Typically they are part of a CodeObject. Is that right?

Yes, that's correct.

Or can one have multiple scopes per “code object”? The Enter/Exit terminology makes me think of frames rather than code objects. If I emulate a code object for a recursive function, should PyMonitoring_EnterScope be called when the codelike is created, or when it's (first?) called?

By "scope" I mean lexical scope. Typically a function, but could be a class or method. PyMonitoring_EnterScope should be called whenever the lexical scope is entered.
For a code generator like Cython, the call to PyMonitoring_EnterScope would be placed at the start of the generated C function, and the call to PyMonitoring_ExitScope at the end of the function, just before the return.

I think this the kind of info that users need to know to use the feature. It should be in the documentation, along with info that:

• scopes can be nested (e.g. when emulating recursive functions), reusing the same state_array and version
• scopes need to be exited and re-entered when the code-like's execution is paused (e.g. when emulating a generator or async function)

(Assuming that's correct. In the implementation, “Enter” reads more like a “cache update” and “Exit” is a no-op, and I'm having some trouble reverse-engineering how that's intended to map to lexical scopes. What is the Exit reserved for?)

I've updated the docs with this. Does it look right now?

[encukou]

Thanks! The usage is clear now :)

The thread about exception state still looks unresolved, however.

[iritkatriel]

The thread about exception state still looks unresolved, however.

Thank for pointing this out. I've added it now.

[encukou]

Py_THROW is exception-related, but with this PR, calling PyMonitoring_FirePyThrowEvent with an exception set triggers an assertion error.

What's the expected behaviour -- should the user wrap the calls in PyErr_GetRaisedException/PyErr_SetRaisedException?

[iritkatriel]

I think we should probably say that the exception should not be set even for exception-related events (the exception should be an arg). @markshannon do you agree?

[encukou]

That means users typically need to call PyErr_GetRaisedException & PyErr_SetRaisedException themselves, outside the internal _PYMONITORING_IF_ACTIVE check.

[iritkatriel]

We could do it for them.

[iritkatriel]

@encukou Please see the latest commit re exception state.