-
Notifications
You must be signed in to change notification settings - Fork 133
ENH: use multiple profiler instances #347
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
==========================================
+ Coverage 64.96% 65.33% +0.36%
==========================================
Files 13 13
Lines 1056 1073 +17
Branches 233 234 +1
==========================================
+ Hits 686 701 +15
- Misses 309 310 +1
- Partials 61 62 +1
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Erotemic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self hosting would be very nice. So this PR is able to work around the sys.monitoring soft lock? Is it that it only gets registered if there are any active instance of LineProfiler, and unregistered when none are left?
I think I mostly understand what is going on, but answers to some of the questions will help me cement this understanding.
line_profiler/_line_profiler.pyx
Outdated
| cdef public object threaddata | ||
|
|
||
| # This is shared between instances and threads | ||
| _active_instances_getter = {}.setdefault |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| _active_instances_getter = {}.setdefault | |
| _thread_to_active_instances = {} | |
| _thread_active_instances_getter = _thread_to_active_instances.setdefault |
Too cryptic. Don't throw away access to the underlying object. Personally I'd opt to just access the setdefault method each time, but there may be a performance benefit to caching it as a class variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, will do
line_profiler/_line_profiler.pyx
Outdated
| # This is shared between instances, but thread-local | ||
| property _active_instances: | ||
| def __get__(self): | ||
| return self._active_instances_getter(threading.get_ident(), set()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allocates memory for a new set every time it is called even if that set isn't used (because the default item already exists). I'd opt for a try / except KeyError here, as the happy path in a try-except should be very low over head in recent python versions.
line_profiler/_line_profiler.pyx
Outdated
|
|
||
| if self._c_code_map.count(code_hash): | ||
| time = hpTimer() | ||
| time = hpTimer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any performance impact in creating an instance of this each time rather than only if any of the instances are in the _c_code_map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a C function so I'd expect that to be quick, but yes it's probably better to keep a has_time flag somewhere and only call hpTimer() when needed and at most once in the loop.
line_profiler/line_profiler.py
Outdated
| Attributes: | ||
| func (types.FunctionType): | ||
| The function it wraps. | ||
| profiler (int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a rename to profiler_id would make this more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and much more grep-able. Much agreed.
| # profiler?) | ||
| wrapped = wrapper.__wrapped__ | ||
| info = getattr(wrapped, self._profiler_wrapped_marker, None) | ||
| new_info = _WrapperInfo(info.func if info else wrapped, id(self)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the function is already owned? It forgets that it was wrapped by a different instance of a LineProfiler and only remembers the latest? Could that cause problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a wrapper created by another profiler:
._already_a_wrapper()would return false (i.e. not our wrapper).add_callable()would however catch that it is a wrapper, sowrapper.__line_profiler_id__.funcinstead ofwrapperis passed to.add_function().- Since it isn't a wrapper this profiler created, a new wrapper wrapping the original one is created.
- When the new wrapper is called, it calls the old wrapper which then calls the underlying function (
wrapper.__line_profiler_id__.func).
Hence both profilers would receive timing info from the call – see L677–80 of test_line_profiler.py, where prof1 records twice the number of line hits compared to prof2, because the call to sum_n_wrapper_1 (L665) is only seen by prof1, and the call to sum_n_wrapper_2 (L666) is seen by both.
|
Thanks for the review! It's more about fundamentally allowing |
line_profiler/line_profiler.py[i]::LineProfiler
<General>
No longer a `line_profiler._line_profiler.LineProfiler`
subclass, but instead wrapping around a global instance thereof
add_function(), {en,dis}able[_by_count](), get_stats()
New methods (wrappers around the corresponding methods of the
underlying profiler
code_hash_map, dupes_map, [c_]{code_map, last_time}
enable_count, timer_unit
New properties (wrappers around the corresponding attributes of
the underlying profiler)
wrap_{async_generator,coroutine,generator,function}()
Overrode superclass methods so as to distinguish between cases
where the function has been seen by the underlying C-level
profiler (but not this `LineProfiler` instance), and where it
hasn't
line_profiler/profiler_mixin.py
wrap_{async_generator,coroutine,generator,function}()
Refactored implementations so subclasses are allowed to supply
the appropriate bookkeeping callbacks to be called before and
after the profiled section other than `.enable_by_count()` and
`.disable_by_count()`
TODO: tests
tests/test_line_profiler.py
test_multiple_profilers_metadata()
Test that the metadata wrappers on `line_profiler.LineProfiler`
correctly wraps around the corresponding attributes/descriptors
on `line_profiler._line_profiler.LineProfiler` (the Cython-level
object)
test_multiple_profilers_usage()
Test that multiple instances of `line_profiler.LineProfiler` can
function simultaneously and separately
line_profiler/_line_profiler.pyx
LineProfiler
- Added:
- Private class attribute `._active_instances_getter`
- Private property `._active_instances`
- Refactored `.enable()` and `.disable()` so that multiple
instances of `LineProfiler` are supported
python_trace_callback()
- Updated call signature; instead of taking a `LineProfiler`
`PyObject`, now it takes an iterable thereof
- Refactored implementation to work with all profiler instances
in said iterable
line_profiler/line_profiler.py[i]
<General>
Rolled back most of the code changes (e.g. now a
`line_profiler._line_profiler.LineProfiler` subclass again)
LineProfiler.add_callable()
- Added check for if the callable is a wrapper created by
another profiler, in which case the wrapped function's
`add_callable()` should be called instead
- Fixed erroneous stub-file return annotation (`None` ->
`Literal[0, 1]`)
LineProfiler._already_a_wrapper(), ._mark_wrapper()
Renamed and updated implementations
line_profiler/profiler_mixin.py::ByCountProfilerMixin
<General>
Rolled back most of the code changes (e.g. removed the new
private `._get_toggle_callbacks()` method)
_already_a_wrapper() (<- `_already_wrapped()`)
_mark_wrapper() (<- `_mark_wrapped()`)
Renamed methods for clarity
tests/test_line_profiler.py
test_multiple_profilers_metadata()
Removed test because `line_profiler.LineProfiler` no longer
wraps around `line_profiler._line_profiler.LineProfiler`
test_multiple_profilers_usage()
Updated to reflect the new, improved implementation: profiler
instances and created wrappers are now separate, so profiling is
granular and only happens e.g. when the appropriate wrapper is
called
line_profiler/_line_profiler.pyx
LineProfiler
- Replaced private class attribute `._active_instances_getter`
with `._all_active_instances`
- Refactored private property `._active_instances` to use
try-except instead of `.setdefault()` for performance
python_trace_callback()
Now no longer calls `line_profiler/timers.c::hpTimer()` for all
line events
line_profiler/line_profiler.py::_WrapperInfo
Renamed attribute `.profiler` to `.profiler_id`
Erotemic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Cheers! |
docs/source/auto/line_profiler.unset_trace.rst
line_profiler/unset_trace.{c,h}
Removed files (cherry-picked from where we were before merging pyutils#347)
CHANGELOG.rst
line_profiler/CMakeLists.txt
setup.py
Updated (cherry-picked from where we were before merging pyutils#347)
- `CHANGELOG.rst`: Added entry
- <Others>: Added handling for the below files
line_profiler/Python_wrapper.h
line_profiler/c_trace_callbacks.{c,h}
Added new files to be used by `line_profiler/_line_profiler.pyx`
(cherry-picked from where we were before merging pyutils#347)
- `Python_wrapper.h`:
New header file which wraps around `Python.h` and provides
compatibility layer over CPython C APIs
- `c_trace_callbacks.*`:
New source/header files for code which handles the retrieval and
use of C-level trace callbacks
line_profiler/_line_profiler.pyx
_ThreadState
- Renamed from `ThreadState`
- Made `._wrap_trace` unaccessible on the Python level
LineProfiler
wrap_trace.__set__()
Now syncing between `_ThreadState`s in `._all_thread_states`
_thread_state.__get__()
If another thread is already initialized, get the
`wrap_trace` therefrom
tests/test_sys_trace.py
New test module (mostly cherry-picked from before we merged pyutils#347):
- `test_callback_preservation()`
- `test_callback_wrapping()`
- `test_wrapping_throwing_callback()`
- `test_wrapping_line_event_disabling_callback()`
- `test_wrapping_thread_local_callbacks()`
CHANGELOG.rst
line_profiler/CMakeLists.txt
setup.py
Updated (cherry-picked from where we were before merging pyutils#347)
- `CHANGELOG.rst`: Added entry
- <Others>: Added handling for the below files
line_profiler/Python_wrapper.h
line_profiler/c_trace_callbacks.{c,h}
Added new files to be used by `line_profiler/_line_profiler.pyx`
(cherry-picked from where we were before merging pyutils#347)
- `Python_wrapper.h`:
New header file which wraps around `Python.h` and provides
compatibility layer over CPython C APIs
- `c_trace_callbacks.*`:
New source/header files for code which handles the retrieval and
use of C-level trace callbacks
line_profiler/_line_profiler.pyx
_ThreadState
- Renamed from `ThreadState`
- Made `._wrap_trace` unaccessible on the Python level
LineProfiler
wrap_trace.__set__()
Now syncing between `_ThreadState`s in `._all_thread_states`
_thread_state.__get__()
If another thread is already initialized, get the
`wrap_trace` therefrom
tests/test_sys_trace.py
New test module (mostly cherry-picked from before we merged pyutils#347):
- `test_callback_preservation()`
- `test_callback_wrapping()`
- `test_wrapping_throwing_callback()`
- `test_wrapping_line_event_disabling_callback()`
- `test_wrapping_thread_local_callbacks()`
CHANGELOG.rst
line_profiler/CMakeLists.txt
setup.py
Updated (cherry-picked from where we were before merging pyutils#347)
- `CHANGELOG.rst`: Added entry
- <Others>: Added handling for the below files
line_profiler/Python_wrapper.h
line_profiler/c_trace_callbacks.{c,h}
Added new files to be used by `line_profiler/_line_profiler.pyx`
(cherry-picked from where we were before merging pyutils#347)
- `Python_wrapper.h`:
New header file which wraps around `Python.h` and provides
compatibility layer over CPython C APIs
- `c_trace_callbacks.*`:
New source/header files for code which handles the retrieval and
use of C-level trace callbacks
line_profiler/_line_profiler.pyx
_ThreadState
- Renamed from `ThreadState`
- Made `._wrap_trace` unaccessible on the Python level
LineProfiler
wrap_trace.__set__()
Now syncing between `_ThreadState`s in `._all_thread_states`
_thread_state.__get__()
If another thread is already initialized, get the
`wrap_trace` therefrom
tests/test_sys_trace.py
New test module (mostly cherry-picked from before we merged pyutils#347):
- `test_callback_preservation()`
- `test_callback_wrapping()`
- `test_wrapping_throwing_callback()`
- `test_wrapping_line_event_disabling_callback()`
- `test_wrapping_thread_local_callbacks()`
Motivation
Currently, trying to use multiple
LineProfilerinstances just doesn't work, particularly if they are meant to be active simultaneously and to profile the same functions:'line_profiler'soft-lock onsys.monitoringin its.enable()/.disable()methods. This causes a clash and an error due to a doublesys.monitoring.use_tool_id().line_profiler/_line_profiler.pyx::python_trace_callback()) only supports one profiler object. This will be alleviated by Restore trace callback when the profiler is disabled #334 where we can chain trace callbacks, but even then the feature is experimental and I'm not quite comfortable with making it the default behavior yet.Enabling the use of multiple profiler instances would have obvious benefits, not the least of which being being able to instrument already-profiled code, and perhaps someday making
line_profilerself-hosting (comment 1, comment 2).Implementation
This PR enables the use of multiple profilers by:
line_profiler/_line_profiler.pyx::LineProfilerLineProfilerand refactoring its.enable()and.disable()method to keep tabs on which profiler instances are activated.python_trace_callback()so that it takes an iterable of the active profilers and does its thing with each thereof as before.line_profiler/line_profiler.py::LineProfiler._already_a_wrapper()(resp.._mark_wrapper()) to process (resp. attac) additional metadata to the private.__line_profiler_id__attribute of profiling wrapper-function object, to facilitate otherLineProfilerinstances' retrieving the underlying function..add_callable()so that it checks whether the callable is a profiling wrapper-function object and calls.add_function()on the underlying function instead if so.Other changes
line_profiler/line_profiler.py[i]::LineProfilerFixed/Added return-type annotations to the methods
.add_callable()and.add_module()line_profiler/profiler_mixin.py::ByCountProfilerMixinRenamed private methods to be more indicative of what they do:
._already_wrapped()->._already_a_wrapper()._mark_wrapped()->._mark_wrapper()tests/test_line_profiler.py::test_multiple_profilers_usage()New test for the use of multiple
LineProfilerinstances, checking that profiling data are separately and correctly collectedCHANGELOG.rstAdded entry
Further suggestions
With the use of multiple profiler instances in mind, it may be a good idea to revisit, polish, and modernize #219, so that one can aggregate the profiling data from different instances.