-
Notifications
You must be signed in to change notification settings - Fork 684
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
Implement a safe API wrapping PyEval_SetProfile #4039
base: main
Are you sure you want to change the base?
Conversation
} | ||
|
||
pub fn register_profiler<P: Profiler>(profiler: Bound<'_, P>) { | ||
unsafe { ffi::PyEval_SetProfile(Some(profile_callback::<P>), profiler.into_ptr()) }; |
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.
Do you also want to support the new PyEval_SetProfileAllThreads function?
Also, there should be a way to clear tracing. Either with a separate function, or accepting an Option argument.
Finally, why needless change the name from setprofile
/ SetProfile
to register_profiler
?
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.
Yeah, I think supporting PyEval_SetProfileAllThreads
makes sense. But I'll get the main bit working fully first.
The register_profiler
naming was also inspired by the implementation in #4008 (comment). I'd prefer to decide on final naming once the implementation is complete. setprofile
could well make sense for symmetry.
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.
Looking again, I've decided to just rename now.
On the clearing tracing - I'm not sure there's an exposed way to do this in the Python C-API. We might need to wrap sys.setprofile(None)
and threading.setprofile(None)
. If I'm missing something here a pointer would be very welcome.
register_profiler(profiler); | ||
|
||
py.run_bound(PYTHON_CODE, None, None).unwrap(); | ||
|
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.
Should clear tracing here.
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.
Sure. This was just where I got to before I had to head to bed.
CodSpeed Performance ReportMerging #4039 will not alter performanceComparing Summary
|
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.
Sorry I took so long to take a first look at this! Overall this makes sense and seems a reasonable API to expose. I had a mix of design questions and implementation nits to start the thinking off...
src/instrumentation.rs
Outdated
Return(Option<Bound<'py, PyAny>>), | ||
/// A C function is about to be called. The contained data is the | ||
/// function object being called. | ||
CCall(Bound<'py, PyAny>), |
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.
Should this be PyCFunction
rather than PyAny
here? (not sure, just wondering)
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.
Ooh, maybe!
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 seems a little more correct to do this so I think we might as well, but I'm not sure it really unlocks any useful functionality, since PyCFunction
is a pretty opaque type.
// | ||
// `frame` is an `ffi::PyFrameObject` which can be converted safely to a `PyObject`. | ||
let frame = frame as *mut ffi::PyObject; | ||
Python::with_gil(|py| { |
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 the profiler callback always called with the GIL held? If so, we can probably do something more like the code in trampoline.rs
(we might even want to reuse that trampoline to avoid panic unwinds escaping the callback).
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.
I can double check.
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.
Do we have any docs about how this trampoline stuff works and how it's supposed to be used? It's not clicking for me right now.
} | ||
|
||
/// Trait for Rust structs that can be used with Python's profiling API. | ||
pub trait Profiler: PyClass<Frozen = False> { |
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.
The Profiler
trait as a subtype of PyClass
is an interesting choice. Is that strictly necessary? Is that for the borrow checking? I wonder how this might interact with nogil. I also whether something like what we've got with run_closure
over in the function code might work (e.g. just register a Fn()
closure rather than a full type).
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.
I think it is necessary, but maybe there's an alternative that avoids it that I missed.
No worries - it's more blocked on me finding the time to continue it. |
We can trust the types CPython provides to be correct.
This matches the Python naming better.
`PyRefMut` can lead to problems when profiling a multithreaded Python program. Using `PyRef` and interior mutability is more robust.
Fixes #4008.