Restore trace callback when the profiler is disabled

[TTsangSC]

(Closes #333.)

Summary

(Update 12 Jul: this summary has been completely rewritten to reflect the latest state of affairs)

This draft PR adds C(-ython) level support to:

• Cache the existing trace-callback state(s) when .enable()-ing line profiling and restoring said state(s) when .disable()-ing it
• Optionally call the cached callback(s) from within the profiler callback(s)
• Improve compatibility with code (e.g. doctest.DocTestRunner.run()) using this Python-level paradigm to temporarily alter "legacy" tracing:

  callback = sys.gettrace()
  try:
      ...  # Do something which involves swapping/unsetting the trace function
  finally:
      sys.settrace(callback)  # Restore the trace function

The option to switch back to using the "legacy" system in Python 3.12+ is also added for debugging purposes.

Code changes

(Click to expand)

• line_profiler/Python_wrapper.h
  New C header wrapping Python.h, centralizing backports for the Python C API
• line_profiler/c_trace_callbacks.{c,h}
  New C functions for handling interactions with the trace system:
  • alloc_callback(), free_callback(), populate_callback(), nullify_callback(), restore_callback(), call_callback()
    Manage the retrieval, use, and restoration of C-level legacy trace callbacks (see _LineProfilerManager)
  • set_local_trace()
    Set the frame-local legacy trace function of a frame object to the _LineProfilerManager, or attach the manager thereto
  • monitoring_restart_version()
    Get the .last_restart_version from the internal interpreter state; this is necessary for detecting calls to sys.monitoring.restart_events()
• line_profiler/_line_profiler.pyx

  • label(), _code_replace(), LineStats
    Updated docstrings

  • disable_line_events()
    New helper function for wrapping a frame-local legacy trace callable so that it no longer receives line events

  • legacy_trace_callback()

    • Updated call signature; now expecting a _LineProfilerManager (manager) instead of a set[LineProfiler] (instances) as the first argument
    • Now calling the cached legacy trace callback if manager.wrap_trace is true
    • Now setting the frame-local legacy trace function if manager.set_frame_local_trace is true

  • _SysMonitoringState
    New thread-local (private) helper object for managing the interactions with sys.monitoring

    • enable()
      Method to replace and cache the sys.monitoring state (relevant callbacks, tool name, active events) upon start of profiling
    • disable()
      Method to restore the cached sys.monitoring state upon end of profiling
    • call_callback()
      C-level method to call the cached callbacks in a safe way (i.e. which wouldn't interfere with line profiling) and manage said cache

  • _LineProfilerManager
    New thread-local (private) helper object superseding the set-of-LineProfilers as the object set as the legacy trace-callback object, which retrieves and caches the trace-callback state it replaces when profiling starts and restores them when it ends:

    • With the "legacy" trace system, PyThreadState.c_tracefunc and PyThreadState.c_traceobj are cached; note that sys.gettrace() only gets the latter and misses the former, hence the need for C-level shenanigans
    • With the sys.monitoring-based system, the callbacks returned by sys.monitoring.register_callback() for the events sys.monitoring.events.LINE, .PY_RETURN, .PY_YIELD, .RAISE, and .RERAISE are cached

    Class members:

    • mon_state
_SysMonitoringState instance
    • __call__()
      Method providing the same API as Python-level legacy trace functions, so that round-tripping the object (sys.settrace(sys.gettrace())) doesn't cause an error
    • wrap_local_f_trace()
      Method for creating wrappers for frame-local legacy trace functions which call both the instance and the wrapped function
    • handle_line_event(), handle_return_event(), handle_yield_event(), handle_raise_event(), handle_reraise_event()
      Methods to be supplied to sys.monitoring.register_callback() for handling the sys.monitoring.LINE, .PY_RETURN, .PY_YIELD, .RAISE, and .RERAISE events and optionally calling the corresponding wrapped/cached callbacks; supersede the previous monitoring_line_event_callback() and monitoring_exit_frame_callback()
      • The cached callbacks and event set are kept updated
      • If a wrapped and cached callback somehow disables tracing (e.g. by trying to unset/change the sys.monitoring callbacks with sys.monitoring.register_callback(), calling sys.monitoring.set_events() to disable line events, or returning sys.monitoring.DISABLE to disable events for a certain code location), those changes are isolated so that line profiling is uninterrupted, but said callbacks will no longer receive the relevant event (except when DISABLE-d code locations are re-enabled by calling sys.monitoring.restart_events())
    • _handle_enable_event(), _handle_disable_event()
      New methods for handling LineProfiler.enable() and .disable(), managing the interactions with existing states of the tracing/monitoring system

  • LineProfiler

    • __doc__
      Extensively extended docstring
    • tool_id
      New class attribute for the sys.monitoring tool ID to use
    • _manager
      New thread-local "class attribute" (_LineProfilerManager instance) which manages LineProfiler instances and provides the trace callbacks, superseding the previous ._active_instances
    • wrap_trace
      New init argument and (thread-local) "class attribute" to control whether existing trace callbacks should be called by the profiling callbacks (true) or stashed away (false)
    • set_frame_local_trace
      New init argument and (thread-local) "class attribute" to control whether to set ._manager as the frame-local legacy trace function, which helps line profiling resume ASAP after legacy tracing is interrupted by e.g. sys.settrace()
    • add_function()
      Added caveat warning against direct use; end-users should use .add_callable() from the wrapper class line_profiler.line_profiler.LineProfiler
    • enable(), disable()
      Deferred most of the implementations to _LineProfilerManager
• line_profiler/_diagnostics.py
  Added the following environmental switches:
  • WRAP_TRACE (env. var.: ${LINE_PROFILER_WRAP_TRACE})
    Boolean flag (default False); default value for LineProfiler.wrap_trace
  • SET_FRAME_LOCAL_TRACE (env. var.: ${LINE_PROFILER_SET_FRAME_LOCAL_TRACE})
    Boolean flag (default False); default value for LineProfiler.set_frame_local_trace
  • USE_LEGACY_TRACE (env. var.: ${LINE_PROFILER_CORE}; old, legacy, and ctrace resolve to True; new, sys.monitoring, and sysmon resolve to False)
    Boolean flag (default False if sys.monitoring is available, True otherwise); toggles whether to use the legacy trace system or the new, sys.monitoring-based system introduced in FIX: restore Cython compatibility + pivot to sys.monitoring #352

Test changes

• tests/test_line_profiler.py::test_sys_monitoring
  Updated implementation to always use sys.monitoring when available
• tests/test_sys_monitoring.py
  New test module for the sys.monitoring-based system
  • test_standalone_callback_usage(), test_wrapping_trace()
    Check that simple sys.monitoring callbacks behave as expected, and are invoked by LineProfiler when wrap_trace=True
  • test_callback_switching()
    Checks that sys.monitoring callbacks which temper with sys.monitoring.register_callback() behave as expected, and are invoked by LineProfiler when wrap_trace=True
  • test_callback_update_events()
    Checks that sys.monitoring callbacks which temper with sys.monitoring.set_[local_]events() behave as expected, and are invoked by LineProfiler when wrap_trace=True
  • test_callback_toggle_local_events()
    Checks that sys.monitoring callbacks which return sys.monitoring.DISABLE and temper with sys.monitoring.restart_events() behave as expected, and are invoked by LineProfiler when wrap_trace=True
• tests/test_sys_trace.py
  New test module for (mostly) the legacy trace system
  • test_callback_preservation()
    Checks the restoration of the trace-callback state after using the profiler
  • test_callback_wrapping()
    Checks trace-callback interoperability – that the existing trace callback can be used by the profiler's callback; also checks the interoperability of LineProfiler.wrap_trace and LineProfiler.set_frame_local_trace
  • test_wrapping_throwing_callback()
    Checks the continuation of line profiling and the disabling of the existing callback in the event that it errors out
  • test_wrapping_line_event_disabling_callback()
    Checks the continuation of line profiling and the hiding of line events from the existing frame-local callback in the event that it disables frame line events
  • test_wrapping_thread_local_callbacks()
    Checks the usage and restoration of thread-local existing trace callbacks by the profiler
  • test_python_level_trace_manipulation()
    Checks that line profiling is amenable to the sys.settrace(sys.gettrace()) paradigm

Packaging changes

• docs/
  Now including the .__call__() methods of objects by default
• line_profiler/CMakeLists.txt
  Updated list of C source files

Motivation

As noted in #333, LineProfiler currently interacts destructively with sys.settrace() and sys.monitoring.register_callback(), setting the corresponding trace callbacks to None when the profiler is .disable()-ed. Beside being in a way "not clean", this also has multiple unfortunate side effects, such as prohibiting cooperative use with other trace-callback based monitoring tools, such as coverage tools and debuggers.

Notably, the discarding of the trace callback was what tanked the coverage that codecov reported, because coverage.py could't see anything after any test called LineProfiler.enable() in-process. While #352 eliminated much of the issues with codecov, because coverage.py defaults to the legacy trace system while line_profiler defaults to sys.monitoring where available, it is still desirable for the tool to function correctly (by restoring existing callbacks) and cooperatively (by optionally enabling calling both the line-profiling and existing callbacks) with other tools.

Effects

Python 3.8

Run on\Filename	Coverage (%, # lines, # missing)	Test duration/s (best, mean of 3)	Test duration/s (w/coverage)
main	70, 1618, 425	11.99, 12.03	19.76
This PR (as of 224d95d)	78, 1618, 301	11.94, 12.01	19.87
This PR w/LINE_PROFILER_WRAP_TRACE=1	79, 1618, 288	-	20.25

Python 3.13

Run on\Filename	Test duration/s best, mean of 3)	Test duration/s (w/coverage)	Test duration/s (w/coverage, COVERAGE_CORE=sysmon)
main	14.55, 14.58	30.64	30.16
This PR (as of 224d95d)	14.72, 14.82	30.85	30.10
This PR w/LINE_PROFILER_WRAP_TRACE=1	-	30.89	29.97

Notes

• Tests are run with the flag -k "not test_cython and not test_sys", to (1) mitigate Cython-related issues with in some configurations and (2) ensure that the same set of tests is run between the different branches.
• The PR does not change coverage in Python 3.13 because coverage.py uses legacy tracing and functions independently of the rest of sys.monitoring (which line_profiler now uses). Also see comments below (comment 1, comment 2).

Caveats

(Outdated; click to expand)

(UPDATE 17 Apr: the optimizations mentioned below have been made.)

As shown by the test-suite output above, this PR may introduce some overhead to the tracing function. However:

• The impact on the measured line timings should be minimal, since any interaction with the cached callback is after the collection of timing data.
• A considerable part of the apparent overhead may just have to do with how the cached callback is run more often (i.e. not being outright discarded) after this PR.

Running the entire test suite without pytest-cov shows the following results:

Run on\Real time elapsed (s; between 10 runs)	Mean	Best	σ
main	7.62	7.42	0.180
This PR	7.65	7.42	0.210
This PR w/LINE_PROFILE_WRAP_TRACE=1	7.47	7.34	0.0983

Hence, in the case where we aren't interacting with other tracing facilities, the overhead is negligible and well within normal variations. Still, it may be worth looking into optimizing wrap_trace (as implemented in line_profiler/_line_profiler.pyx::call_c_trace_state_hook_safe()) since that seem to have more severe performance implications. Since it mostly deals with objects available on the C-level (CTraceState, PyFrameObject.f_trace_lines, PyFrameObject.f_trace), it's likely that we can eke out more performance by moving call_c_trace_state_hook_safe() from Cython to C – or just merge it with the C-level call_c_trace_state_hook() function which it calls.

Update 17 Apr

(Click to expand)

Reorganization

The tests which have to do with sys trace callbacks have been moved from the tests/test_line_profiler.py module to the new tests/test_sys_trace.py.

Handling of edge cases

In order for trace-callback wrapping to be more transparent (i.e. for the wrapped callback to behave more as if it was free-standing):

• In normal execution, a trace callback can unset the sys callback, either via erroring out or explicitly calling sys.settrace(None) (or its C equivalent). If a wrapped callback does so, e.g. in coverage.py::coverage/ctracer/tracer.c::CTracer_dealloc() (L87)):
  • The existing sys callback is restored so that profiling continues, as was previously done in this PR; but
  • The profiler's callback drops the cached callback from that point on, effectively disabling the latter; and
  • When the profiler is .disable()-ed, it unsets the sys trace callback instead of restoring it to the (dropped) cached value.
• In normal execution, a trace callback can set .f_trace_lines to false in a frame, disabling line events for the frame (which obstructs further line profiling). If a wrapped callback does so; e.g. in coverage.py::coverage/ctracer/tracer.c::CTracer_handle_call() (L548)):
  • .f_trace_lines is reset so that profiling continues, as was previously done in this PR; but
  • The frame's local trace callable .f_trace is wrapped so that it is no longer invoked for line events.

Moreover, support for multithreaded environments (where each thread can have its own sys trace function) has been added.

Performance impacts

The previous timing info in "Effects" was unduly biased against the PR in that:

• It was later noted that the virtual environments hosting the main and PR branches had different dependency versions (e.g. pytest); updating them to be consistent substantially closed the gap.
• Since the new tests temper with the sys trace callback, they have to be run in subprocesses to isolate their side effects, resulting in relatively higher overhead.

As such, the venv dependencies have been updated to be consistent (pytest==8.3.5, pytest-cov==6.1.1, coverage==7.8.0), and we now run run_test.py or pytest with -k 'not sys_trace' to deselect the new tests. The updated timings are:

With pytest-cov (run_tests.py -k 'not sys_trace')	Without pytest-cov (pytest -k 'not sys_trace')
Run on\Real time elapsed (s; between 3 runs) Mean Best σ main 15.83 15.77 0.049 This PR 15.87 15.80 0.052 This PR w/LINE_PROFILE_WRAP_TRACE=1 15.83 15.80 0.033	Run on\Real time elapsed (s; between 10 runs) Mean Best σ main 7.045 6.97 0.058 This PR 7.043 6.94 0.109 This PR w/LINE_PROFILE_WRAP_TRACE=1 7.159 7.0 0.101

Hence the overhead introduced by the PR seems negligible. (The coverage improvements remain the same as in "Effects" above.)

Update 19 May

(Click to expand)

In response to review, the C-level functions in line_profiler/_line_profiler.pyx for handling trace callbacks are refactored out into their own C source/header files line_profiler/c_trace_callback.c and .h.

To make Python C-API use more consistent (e.g. back-porting of newer utils), the wrapper line_profiler/Python_wrapper.h is created around Python.h. As such:

• The need to refer to PY_VERSION_HEX in C(-ython) code elsewhere is minimized.
• A corner case which pertains to the use of PyCode_GetCode() in pre-release versions of Python 3.11 is caught and fixed.

Update 26 May

(Click to expand)

The PR is completely rewritten after #347 was merged due to rebasing difficulties, but most of the previous changes are kept, with the following additions and enhancements:

• line_profiler/_line_profiler.pyx:

  • A new object type _LineProfilerManager is introduced so that:

    • The trace-callback state is decoupled from individual LineProfiler instances.
    • The object returned from sys.gettrace() is now:
      • An instance thereof (instead of being a set[LineProfiler])
      • Callable as a Python-level trace function which returns itself, and
      • Amenable to being set via sys.settrace();

    This improves compatibility when profiling code which does something like the below example (e.g. doctest.DocTestRunner.run()):

    callback = sys.gettrace()  # This returns the manager object
    try:
        sys.settrace(something_else)  # This suspends line profiling
        ...
    finally:
        # This restores the manager object and resumes profiling,
        # but causes problems if it isn't callable w/the signature
        # `(frame: types.FrameType,
        #   event: Literal['call', 'line', 'return', 'exception', 'opcode'],
        #   arg: object)`
        sys.settrace(callback)

    The overhead should be minimal since the .__call__() resets the C-level trace callback to python_trace_callback() where possible, sidestepping .__call__() and the default sys trace trampoline which calls it indirectly.

  • LineProfiler.wrap_trace is now a pseudo class attribute and synced between all instances.

  • LineProfiler now takes an extra set_frame_local_trace argument (taking its default from the ${LINE_PROFILE_WRAP_TRACE}) environment variable), which optionally sets the frame-local trace function types.FrameType.f_trace to the manager upon PyTrace_CALL events, facilitating the resumption of line profiling after suspension as in the above code example.

• tests/test_sys_trace.py::test_python_level_trace_manipulation():
  New test checking that line profiling plays well with Python-level code which uses sys.gettrace() and sys.settrace() to manipulate the trace-callback state.

Update 11 Jul

(Click to expand)

Since #352 we've been moving towards using sys.monitoring where available (Python 3.12+), instead of the legacy trace system (sys.gettrace(), sys.settrace(), PyEval_SetTrace()). The PR is thus updated so that LineProfilers can cache and wrap sys.monitoring callbacks, like it does with legacy trace callbacks:

• line_profiler/_line_profiler.pyx:
  • _LineProfilerManager:
    • Added new helper object .mon_state = _SysMonitoringState for managing interfacing with sys.monitoring
    • Added new methods handle_{line,return,yield}_event() to be used with sys.monitoring.register_callback(sys.monitoring.PROFILER_ID, sys.monitoring.{LINE,PY_RETURN,PY_YIELD}, ...)
  • _SysMonitoringState:
    New helper object to store and restore the sys.monitoring state, as well as to handle cached callbacks
• tests/test_sys_monitoring.py:
  New test module for sys.monitoring-related tests
  • test_standalone_callback_usage(), test_standalone_callback_switching():
    New tests verifying basic behaviors of sys.monitoring callbacks
  • test_wrapping_trace(), test_wrapping_switching_callback():
    New tests checking LineProfiler interacts with existing sys.monitoring callbacks as expected

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.56%. Comparing base (ce3e2be) to head (60e928f).
Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
+ Coverage   87.36%   87.56%   +0.20%     
==========================================
  Files          18       18              
  Lines        1630     1641      +11     
  Branches      347      348       +1     
==========================================
+ Hits         1424     1437      +13     
+ Misses        151      149       -2     
  Partials       55       55              

Files with missing lines	Coverage Δ
line_profiler/_diagnostics.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5817f3e...60e928f. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Erotemic]

Because this modifies C code, it's going to take more time for me to carefully review. I think this will be independent of the other changes you were interested in working on, but let me know if any PR is a blocker for other features. The feature I'm most interested in right now, is the one where -p will mark a submodule to be autoprofiled on import (or we could do it the easy way and just pre-import the specified modules in kernprof as an initial pass with plans to improve on it later).

With all of these changes, I'm also starting to think it would be a good idea for me to spend some effort building regression tests that monitor the overhead of line profiler on benchmark use-cases. Right now I currently test it out and check that it doesn't feel sluggish, but that's not sustainable.

[TTsangSC]

Fair points, please take your time.

Since we already have provisions for running "setup" code in kernprof, it's probably easy to also hack that for doing pre-imports. I'll give that a try, as well as try to port the AST stuff over in the longer run. Oh and of course the TOML stuff that we talked about yesterday.

[Erotemic]

Can the body (or at least most of it) of this be refactored into a separate c/h file? It's adding a lot of bloat to this file.

[TTsangSC]

Makes sense, will do.

[TTsangSC]

Hi, just in case you wanted to look at something else this is also ready and should have minimal conflicts with the other PRs (esp. #337). But the bad news is that since #347 made substantial changes to line_profiler/_line_profiler.pyx quite a bit had to be reworked (see this section in the PR description).

[TTsangSC]

Rebasing on main took a good while because of the sheer volume of changes (#352, #344, #349, and #351) we've made to line_profiler/_line_profiler.pyx over the past month, as well as the fact that I essentially have to write the other half to the PR by writing the same hooks for sys.monitoring as we've already done with the legacy trace system.

Almost done now, but the majority of the tests in tests/test_sys_trace.py still focus on the old trace system instead of the new one. Will work on those tonight, and hopefully we can proceed with further review and merging by tomorrow.

[TTsangSC]

@Erotemic

Whew. Sorry for the long delay, but I'm finally done fixing the sys.monitoring side of things¹ and wrote some new tests for that, and the PR's ready for review. Several points of note though:

• This PR no longer causes an increase in coverage in Python 3.12+ relative to main, because:

  • The default behavior of coverage is (still) to use the legacy trace system because they haven't got dynamic contexts and branch coverage figured out. Since the legacy system is largely parallel to sys.monitoring, our profiling doesn't interfere with coverage measurement.
  • Even when using COVERAGE_CORE=sysmon and forcing coverage to use sys.monitoring, coverage and profiling tools are transparent to one another because they occupy different tool IDs (sys.monitoring.COVERAGE_ID = 1, PROFILER_ID = 2).

  Nonetheless, we now have peace of mind knowing we'd be able to handle other tools even if the IDs did clash. And improvements in coverage for lower Python versions are of course preserved.

• Still, to leave the possibility of tool-ID shenanigans² open, tool IDs are now pulled from the class attribute LineProfiler.tool_id, instead of hard-coded references to sys.monitoring.PROFILER_ID.

• Performance-wise, I redid the measurements comparing main and this PR, and again the overhead (when not interacting with other legacy/sys.monitoring callbacks) is negligible.

• We may want to think about whether the env vars ${LINE_PROFILE_WRAP_TRACE} and ${LINE_PROFILE_SET_FRAME_LOCAL_TRACE} are named appropriately. E.g. do we want ${LINE_PROFILE_*} or ${LINE_PROFILER_*}? Or maybe are we gonna throw the env vars out once ENH: read TOML files for configurations #335 is good to go, and we can make LineProfiler read them from the configs instead?

• We may also want to provide the option somewhere to switch back to the legacy trace system, like how coverage allows for the backend to be specified by ${COVERAGE_CORE}. But that of course can wait.

Footnotes

1. I had to dig deep into CPython (see c_trace_callbacks.h) because we needed to manipulate PyInterpreterState in order to find out whether sys.monitoring.restart_events() has been called, and we should thus no longer withhold previously locally-disabled events from the cached callbacks. ↩

2. Notably, coverage uses a ${COVERAGE_COVERAGE} env var to switch to an unoccupied ID (3) when measuring its own coverage. ↩

[Erotemic]

We may want to think about whether the env vars ${LINE_PROFILE_WRAP_TRACE} and ${LINE_PROFILE_SET_FRAME_LOCAL_TRACE} are named appropriately. E.g. do we want ${LINE_PROFILE_} or ${LINE_PROFILER_}?

I agree. I think having LINE_PROFILE=1 to activate the global profiler makes sense (I read it more like an action than a setting), but for everything else where it is controlling behavior using LINE_PROFILER_xxx seems better. We could even alias LINE_PROFILE=1 to mean something like LINE_PROFILER_ACTIVATE_GLOBAL_PROFILER=1 (or something maybe a bit more concise)

Or maybe are we gonna throw the env vars out once

Controlling configuration with pyproject.toml will only make sense in some cases. I generally thinking having environment variables for config options like cibuildwheel does it is generally a good idea. But we can move there slowly.

We may also want to provide the option somewhere to switch back to the legacy trace system, like how coverage allows for the backend to be specified by ${COVERAGE_CORE}. But that of course can wait.

An environment variable in this PR might make sense for this. If something goes wrong with the 4.3 release it will be useful to be able to say does it work if you set LINE_PROFILER_CORE=legacy (or whatever the variable is).

I will work on reviewing this over the weekend.

[Erotemic]

I haven't fully grasped everything here yet, but I wanted to at least get a start on it. I will take another pass at this later in the weekend.

Something that would be helpful would be to rewrite the top-level summary comment to reflect the current state. Be sure to save everything else you've already written, that is useful context, but put it under a HistoricalSummary section. This will make it easier for me to get a high level vantage point and be more confident in my review.

[Erotemic]

Could the value of py_frame->f_trace that we are overwriting need to be dereferenced? Or are we guaranteed some other piece of code is managing that heap memory? Would we want to use the Py_XSETREF macro here?

[TTsangSC]

Looking at frameobject.c::frame_trace_set_impl() there's actually some extra cleanup that FrameType.f_trace = ... does compared with just doing Py_SETREF(py_frame->f_trace, Py_XNewRef(value)). Seeing that we're already jumping through hoops in the preceding code just to call a Python function (line_profiler._line_profiler.disable_line_events()), it may make sense to offload part of this function to the Python/Cython side of things.

[Erotemic]

Do we need a check here if the interpreter is shutting down? Should we check Py_IsFinalizing in any of these calls?

[Erotemic]

As python moves to disable the GIL could this (or many other places) cause problems for us in the future? (In general I think there is probably a lot of work to do before this works with nogil. I'm just keeping that in the back of my mind).

[TTsangSC]

As far as I can tell frame objects should be thread-local (see frameobject.c::init_frame()) so that's probably not an issue here... ? I could be dead wrong though.

[Erotemic]

Do we need to do:

PyObject* new_trace = PyObject_CallMethodOneArg(manager, method, py_frame->f_trace);
Py_XSETREF(frame->f_trace, new_trace);

so we don't leak old value of frame->f_trace?

[Erotemic]

Is this worth raising a CPython issue to see what core devs have to say?

[TTsangSC]

If you already have rapport with them it may be worth bringing up... I'd imagine the feature to be rather useful.

Maybe they have a reason for not having done it already though, like preventing 3rd-party code from tempering with an existing callback without triggering the audit event (which BTW is only mentioned in the sys.monitoring docs but not in the table of audit events). But of course if there's a demand for the feature I'm sure they'll figure out a way to make it sufficiently secure.

[Erotemic]

I've only had minor interactions with the core CPython team, but the way to bring it up would be creating a discussion thread on: https://discuss.python.org/ From there, if there is community interest, that may get the ball rolling, or we may learn the reasons why they don't have it. The answer to CPython changes is generally no, but if you have a simple and good idea, and you can present it clearly, you can sometimes get a yes.

[Erotemic]

Accessing the values of PyThreadState like this might not be supported. What about PyThreadState_GetInterpreter? Although I don't see any mechanism to access last_restart_version outside directly doing it.

[Erotemic]

Maybe just state the high level point of this is to add the tool_id to to calls in sys.monitoring that need it. Probably do that in the class level docstring. Something like:

    Helper object which helps with simplifying attribute access on
    :py:mod:`sys.monitoring` by passing the managed tool-id to functions that require it. 

or similar.

I almost just want to have an allowlist of the functions that do need tool_id, there aren't that many, and really we only need to register the ones that we use, although the way you have it is more extensible.

EDIT: oh this is in a test file, never mind.

[Erotemic]

Suggested change

	Yield-event (|PY_RETURN|_) callback passed to
	Yield-event (|PY_YIELD|_) callback passed to

[Erotemic]

Is using fetch to denote you are setting the state of an allocated object like this idiomatic? To me fetch implies that you're going to return something (which you are in an out-var), but I think something like populate_callback might read better? Maybe setup or init could also work?

[TTsangSC]

Yeah honestly I'm struggling a bit with the naming scheme here, not really a big fan of restore_callback() either. Was thinking over the options but I fell asleep midway. Maybe fetch_callback() -> push_callback() and restore_callback() -> pop_callback()?

Anyway I guess we can just go with populate_callback() (which seems to be the most intuitive one) at the moment, but more input would be very welcome.

[TTsangSC]

We could even alias LINE_PROFILE=1 to mean something like LINE_PROFILER_ACTIVATE_GLOBAL_PROFILER=1 (or something maybe a bit more concise)

That's a good idea, and also a rather quick one, since we can just change [tool.line_profiler.setup] environ_flags = ... after #335.

If something goes wrong with the 4.3 release it will be useful to be able to say does it work if you set LINE_PROFILER_CORE=legacy (or whatever the variable is).

On it. Just an update, it turned out that my earlier comment was wrong – the legacy trace APIs are just built in a backward-compatible manner on top of sys.monitoring, so it isn't completely independent therefrom. Still, they occupy reserved sys.monitoring tool IDs (6 for sys.setprofile(), 7 for sys.settrace()), so it's in the same situation as coverage.

Thanks for the comments (esp. on the C code), will take a closer look today.

[TTsangSC]

Implemented fixes for the points you brought up above. Really appreciated the input, there's a lot of the C API that I am not that familiar with.

However, I just noticed that there's unfortunately some unintended interaction between LineProfiler.wrap_trace and LineProfiler.set_frame_local_trace:

Why we have a recursion error (click to expand)

• Say we're using the legacy system, both params are set to true, and we have a Python-level callback set via sys.settrace(some_python_callback). Now some_python_callback() returns itself for a 'call' event of a function so that it receives events and does whatever it does inside the function's frame.

• When calling that function ('call' event), our manager: _LineProfilerManager thus:

  • Caches the active callback: manager.legacy_callback: TraceCallback = { trace_trampoline, some_python_callback } (in _line_profiler.pyx::_LineProfilerManager._handle_enable_event()),
  • Replaces the active callback with { legacy_trace_callback, manager }, which then
  • Sets the frame-local trace callback frame->f_trace to manager (in _line_profiler.pyx::legacy_trace_callback() -> c_trace_callbacks.c::set_local_trace()), and
  • Calls the cached callback via c_trace_callbacks.c::call_callback(), which then calls some_python_callback() with the 'call' event; since some_python_callback() returns itself, sysmodule.c::trace_trampoline() sets it instead as frame->f_trace...
  • ... but since we know that will happen and it defeats the purpose of .set_frame_local_trace, another call to set_local_trace() in legacy_trace_callback() sets the frame->f_trace to the wrapper manager.wrap_local_f_trace(some_python_callback) instead.

  So far so good – we've ensured that Python code that tempers with sys.settrace() and sys.gettrace() will not interrupt line-profiling of the frame.

• However, when we then proceed through the function body ('line' event):

  • legacy_trace_callback(manager, ...) is called, which then calls call_callback()
  • Since the cached callback uses trace_trampoline(), it calls frame->f_trace, which is now our wrapper around some_python_callback() instead of said callback itself...
  • ... but the wrapper also calls manager(), which calls legacy_trace_callback(), causing a recursion.

Honestly it's on me – I didn't think to test the two features side-by-side before, just individually. Will try to code up a fix ASAP (or maybe we just make it so that the two can't be used simultaneously).

[TTsangSC]

Just fixed the recursion bug and extended tests/test_sys_trace.py::test_callback_wrapping() to test the case where LineProfiler.wrap_trace and .set_frame_local_trace are both set.

[TTsangSC]

Do we want to work on #355 separately or just merge it into this?

[Erotemic]

Let's rebase now that #356 is merged. What is your self-assessment on the readiness of this branch? Would you be comfortable merging in its current state? I need to do at least one more pass to up my own confidence a little, but I think there's so much going on here that the probability that we missed something is high and we are going to get issues filed after we make a release. Still I think its worth doing so, but having your self-assessment on this branch will help me calibrate.

[TTsangSC]

Just rebased.

I'd say we're on the same page – there's probably stuff that we've missed, but the PR is reasonably covered by tests, the primary function (trace-callback caching/restoration) is reasonably simple, and the secondary function (trace-callback wrapping) is explicitly said to be experimental. So I'm in general comfortable with merging as-is as far as the code goes – will give it another look to iron out wrinkles in the docs.

[Erotemic]

Should pin these URLS to a specific tag or commit hash, otherwise it risks dead links.

[TTsangSC]

Maybe we can pin to 3.13 for the time being then? After all we aren't even sure about how far we are from officially supporting 3.14.

[TTsangSC]

Actually, I'm also wondering if the docs should be moved to line_profiler.line_profiler.LineProfiler (the subclass) for visibility...

[Erotemic]

Depends if they are internal or external documentation. They look like they are mostly documenting internals. Ideally user-facing docs would be geared towards "how do I use it" rather than "how does it work?" (which can sometimes be the same question, so its tricky).

Just pin to something, 3.13 is fine. The idea is to ensure the link doesn't rot if CPython renames the file.

[TTsangSC]

Considering that wrap_trace and set_frame_local_trace are parameters fully accessible from public APIs (since LineProfiler just inherits the .__init__() from the Cython base class... yeah it's a tricky one.

But then again they are experimental... maybe it makes better sense to keep them tucked away here, before we've fully refined the feature and are ready to further advertise it in the API (e.g. by adding the corresponding command-line options to kernprof).

Anyway, I've fixed the links among other things in the latest two commits (6b37307 and 60e928f).

[Erotemic]

I did another pass, and I think it looks good. That being said, its easy to miss something with a big refactor like this and additional C code. I'm going to merge, and then we just have one more PR before we can release.

I do wonder if it would be possible to write a rust backend where we can prove there are no memory errors, and if that would be as performent as the current cython+c implementation. I think that would be a possible and useful AI task in the not so distant future.

[TTsangSC]

Again, thanks for the review!

This reminds me, I really should be learning Rust. Not entirely sure though if there would be real benefits in terms of memory safety (since at the end of the day we still have features requiring talking to the Python interpreter via the C API – with both the legacy and sys.monitoring backends), but I guess it wouldn't hurt.

[Erotemic]

There likely would be unsafe sections, but it would restrict any scope of concern to those sections, which is the main draw. I imagine it is only a matter of time before Python is re-written in Rust. It may be quite a bit of time, which means that now is a great time to be learning it.