Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-116750: Add clear_tool_id function to unregister events and callbacks #124568

Merged
merged 9 commits into from
Oct 1, 2024
12 changes: 5 additions & 7 deletions Doc/library/sys.monitoring.rst
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,14 @@ Registering and using tools
*tool_id* must be in the range 0 to 5 inclusive.
Raises a :exc:`ValueError` if *tool_id* is in use.

.. function:: free_tool_id(tool_id: int, /) -> None
.. function:: clear_tool_id(tool_id: int, /) -> None

Should be called once a tool no longer requires *tool_id*.
Unregister all events and callback functions associated with *tool_id*.

.. note::
.. function:: free_tool_id(tool_id: int, /) -> None

:func:`free_tool_id` will not disable global or local events associated
with *tool_id*, nor will it unregister any callback functions. This
function is only intended to be used to notify the VM that the
particular *tool_id* is no longer in use.
Should be called once a tool no longer requires *tool_id*.
Will call :func:`clear_tool_id` before releasing *tool_id*.

.. function:: get_tool(tool_id: int, /) -> str | None

Expand Down
4 changes: 4 additions & 0 deletions Include/cpython/code.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
extern "C" {
#endif

/* Total tool ids available */
#define _PY_MONITORING_TOOL_IDS 8
/* Count of all local monitoring events */
#define _PY_MONITORING_LOCAL_EVENTS 10
/* Count of all "real" monitoring events (not derived from other events) */
Expand Down Expand Up @@ -57,6 +59,8 @@ typedef struct {
_Py_LocalMonitors active_monitors;
/* The tools that are to be notified for events for the matching code unit */
uint8_t *tools;
/* The version of tools when they instrument the code */
uintptr_t tool_versions[_PY_MONITORING_TOOL_IDS];
/* Information to support line events */
_PyCoLineInstrumentationData *lines;
/* The tools that are to be notified for line events for the matching code unit */
Expand Down
1 change: 1 addition & 0 deletions Include/internal/pycore_interp.h
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ struct _is {
Py_ssize_t sys_tracing_threads; /* Count of threads with c_tracefunc set */
PyObject *monitoring_callables[PY_MONITORING_TOOL_IDS][_PY_MONITORING_EVENTS];
PyObject *monitoring_tool_names[PY_MONITORING_TOOL_IDS];
uintptr_t monitoring_tool_versions[PY_MONITORING_TOOL_IDS];

struct _Py_interp_cached_objects cached_objects;
struct _Py_interp_static_objects static_objects;
Expand Down
41 changes: 41 additions & 0 deletions Lib/test/test_monitoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,14 @@ def nth_line(func, offset):

class MonitoringBasicTest(unittest.TestCase):

def tearDown(self):
sys.monitoring.free_tool_id(TEST_TOOL)

def test_has_objects(self):
m = sys.monitoring
m.events
m.use_tool_id
m.clear_tool_id
m.free_tool_id
m.get_tool
m.get_events
Expand Down Expand Up @@ -77,6 +81,43 @@ def test_tool(self):
with self.assertRaises(ValueError):
sys.monitoring.set_events(TEST_TOOL, sys.monitoring.events.CALL)

def test_clear(self):
events = []
sys.monitoring.use_tool_id(TEST_TOOL, "MonitoringTest.Tool")
sys.monitoring.register_callback(TEST_TOOL, E.PY_START, lambda *args: events.append(args))
sys.monitoring.register_callback(TEST_TOOL, E.LINE, lambda *args: events.append(args))
def f():
a = 1
sys.monitoring.set_local_events(TEST_TOOL, f.__code__, E.LINE)
sys.monitoring.set_events(TEST_TOOL, E.PY_START)

f()
sys.monitoring.clear_tool_id(TEST_TOOL)
f()

# the first f() should trigger a PY_START and a LINE event
# the second f() after clear_tool_id should not trigger any event
# the callback function should be cleared as well
self.assertEqual(len(events), 2)
callback = sys.monitoring.register_callback(TEST_TOOL, E.LINE, None)
self.assertIs(callback, None)

sys.monitoring.free_tool_id(TEST_TOOL)

events = []
sys.monitoring.use_tool_id(TEST_TOOL, "MonitoringTest.Tool")
sys.monitoring.register_callback(TEST_TOOL, E.LINE, lambda *args: events.append(args))
sys.monitoring.set_local_events(TEST_TOOL, f.__code__, E.LINE)
f()
sys.monitoring.free_tool_id(TEST_TOOL)
sys.monitoring.use_tool_id(TEST_TOOL, "MonitoringTest.Tool")
f()
# the first f() should trigger a LINE event, and even if we use the
# tool id immediately after freeing it, the second f() should not
# trigger any event
self.assertEqual(len(events), 1)
sys.monitoring.free_tool_id(TEST_TOOL)


class MonitoringTestBase:

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Provide :func:`sys.monitoring.clear_tool_id` to unregister all events and callbacks set by the tool.
29 changes: 28 additions & 1 deletion Python/clinic/instrumentation.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

84 changes: 84 additions & 0 deletions Python/instrumentation.c
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,16 @@ update_instrumentation_data(PyCodeObject *code, PyInterpreterState *interp)
if (allocate_instrumentation_data(code)) {
return -1;
}
// If the local monitors are out of date, clear them up
_Py_LocalMonitors *local_monitors = &code->_co_monitoring->local_monitors;
for (int i = 0; i < PY_MONITORING_TOOL_IDS; i++) {
if (code->_co_monitoring->tool_versions[i] != interp->monitoring_tool_versions[i]) {
for (int j = 0; j < _PY_MONITORING_LOCAL_EVENTS; j++) {
local_monitors->tools[j] &= ~(1 << i);
}
}
}

_Py_LocalMonitors all_events = local_union(
interp->monitors,
code->_co_monitoring->local_monitors);
Expand Down Expand Up @@ -2004,6 +2014,8 @@ _PyMonitoring_SetLocalEvents(PyCodeObject *code, int tool_id, _PyMonitoringEvent
goto done;
}

code->_co_monitoring->tool_versions[tool_id] = interp->monitoring_tool_versions[tool_id];

_Py_LocalMonitors *local = &code->_co_monitoring->local_monitors;
uint32_t existing_events = get_local_events(local, tool_id);
if (existing_events == events) {
Expand Down Expand Up @@ -2036,6 +2048,43 @@ _PyMonitoring_GetLocalEvents(PyCodeObject *code, int tool_id, _PyMonitoringEvent
return 0;
}

int _PyMonitoring_ClearToolId(int tool_id)
{
assert(0 <= tool_id && tool_id < PY_MONITORING_TOOL_IDS);
PyInterpreterState *interp = _PyInterpreterState_GET();

for (int i = 0; i < _PY_MONITORING_EVENTS; i++) {
PyObject *func = _PyMonitoring_RegisterCallback(tool_id, i, NULL);
if (func != NULL) {
Py_DECREF(func);
}
}

if (_PyMonitoring_SetEvents(tool_id, 0) < 0) {
return -1;
}

_PyEval_StopTheWorld(interp);
uint32_t version = global_version(interp) + MONITORING_VERSION_INCREMENT;
if (version == 0) {
PyErr_Format(PyExc_OverflowError, "events set too many times");
_PyEval_StartTheWorld(interp);
return -1;
}

// monitoring_tool_versions[tool_id] is set to latest global version here to
// 1. invalidate local events on all existing code objects
// 2. be ready for the next call to set local events
interp->monitoring_tool_versions[tool_id] = version;
Copy link
Member

@markshannon markshannon Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the version for tool_id will be the only one that matches the global version, which means that tool_id will be the only tool with an up-to-date version number.
Isn't that the opposite of what we want?

Maybe interp->monitoring_tool_versions[tool_id] = 0 and leave the global version untouched?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to force all code object, executing or not, to "refresh" because the local events will be changed, so we need to update global version for the code objects to know.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understood you here. You think it would make sense to do

interp->monitoring_tool_versions[tool_id] = 0;

here so all the code objects checking tool_id will know they are out of date. However, setting it to the latest global version will have the same effect - none of the code object has this version either.

If we set this to 0, we need to set it to the latest global version some time in the future, before we use this tool to instrument code again.

So we can either set it to 0 here and set it to global_version in use_tool_id, or we can do it together here(I can add some. comments here to explain).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I was wrong in the previous comment. The update to global version is critical here because the user can set local events immediately after clear_tool_id. clear_tool_id does not surrender the tool_id. So we have to set the tool id version to the latest global version here for the following local events.


// Set the new global version so all the code objects can refresh the
// instrumentation.
set_global_version(_PyThreadState_GET(), version);
int res = instrument_all_executing_code_objects(interp);
_PyEval_StartTheWorld(interp);
return res;
}

/*[clinic input]
module monitoring
[clinic start generated code]*/
Expand Down Expand Up @@ -2083,6 +2132,33 @@ monitoring_use_tool_id_impl(PyObject *module, int tool_id, PyObject *name)
Py_RETURN_NONE;
}

/*[clinic input]
monitoring.clear_tool_id

tool_id: int
/

[clinic start generated code]*/

static PyObject *
monitoring_clear_tool_id_impl(PyObject *module, int tool_id)
/*[clinic end generated code: output=04defc23470b1be7 input=af643d6648a66163]*/
{
if (check_valid_tool(tool_id)) {
return NULL;
}

PyInterpreterState *interp = _PyInterpreterState_GET();

if (interp->monitoring_tool_names[tool_id] != NULL) {
if (_PyMonitoring_ClearToolId(tool_id) < 0) {
return NULL;
}
}

Py_RETURN_NONE;
}

/*[clinic input]
monitoring.free_tool_id

Expand All @@ -2099,6 +2175,13 @@ monitoring_free_tool_id_impl(PyObject *module, int tool_id)
return NULL;
}
PyInterpreterState *interp = _PyInterpreterState_GET();

if (interp->monitoring_tool_names[tool_id] != NULL) {
if (_PyMonitoring_ClearToolId(tool_id) < 0) {
return NULL;
}
}

Py_CLEAR(interp->monitoring_tool_names[tool_id]);
Py_RETURN_NONE;
}
Expand Down Expand Up @@ -2376,6 +2459,7 @@ monitoring__all_events_impl(PyObject *module)

static PyMethodDef methods[] = {
MONITORING_USE_TOOL_ID_METHODDEF
MONITORING_CLEAR_TOOL_ID_METHODDEF
MONITORING_FREE_TOOL_ID_METHODDEF
MONITORING_GET_TOOL_METHODDEF
MONITORING_REGISTER_CALLBACK_METHODDEF
Expand Down
1 change: 1 addition & 0 deletions Python/pystate.c
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,7 @@ init_interpreter(PyInterpreterState *interp,
interp->monitoring_callables[t][e] = NULL;

}
interp->monitoring_tool_versions[t] = 0;
}
interp->sys_profile_initialized = false;
interp->sys_trace_initialized = false;
Expand Down
Loading