gh-91054: Add code object watchers API #99859
Conversation
…on creation and destruction of PyCodeObject Co-authored-by: Ye11ow-Flash <[email protected]>
Co-authored-by: Ye11ow-Flash <[email protected]>
|
@itamaro I talked this over briefly with @markshannon and it sounds like we should make the same optimization here as @mpage did in 812dd5f for func watchers, to make the check for active watchers a bit more efficient and reduce the overhead on code object creation. |
A bit is set in the bit vector iff there is a watcher set at the corresponding offset in the watcher array. Only notify watchers if at least one bit is set.
|
FYI, I expect to add a |
| notify_code_watchers(PyCodeEvent event, PyCodeObject *co) | ||
| { | ||
| PyInterpreterState *interp = _PyInterpreterState_GET(); | ||
| if (interp->active_code_watchers) { |
There was a problem hiding this comment.
I'm OK with this as is, but you could speed this up with an invariant.
(active_code_watchers & (1 << i)) implies interp->code_watchers[i] != NULL
Then the loop can be something like this:
uint8_t watchers = interp->active_code_watchers;
while (watchers) {
int watcher = left_most_bit(watchers);
watchers &= ~(1 << watcher);
if (interp->code_watchers[watcher](...) {
PyErr_WriteUnraisable(...);
}
}
This probably applies to the other watchers as well, so you might want to do this in another PR.
There was a problem hiding this comment.
I think this optimization applies to func watchers, but is already effectively applied for dict and type watchers; in those cases we already never look up the watcher unless the bit is set.
There was a problem hiding this comment.
I have some more watchers-wide cleanup I wanted to do, so will add this optimization to the next PR, thanks!
There was a problem hiding this comment.
cleanup and polish PR (including this optimization) is up at GH-99998
Yep! (At least the ones we had in mind.) |
|
|
FYI that buildbot failure doesn't seem to be transient. Are there new tests which are recursive and assuming a certain stack depth? Looking at the test failure it seems the call stack is being blown out in the emscripten build (line 515 of https://buildbot.python.org/all/#/builders/1050/builds/953/steps/10/logs/stdio). |
|
There are many buildbots failing to build this PR. Check https://buildbot.python.org/all/#/release_status for reference As per our buildbot policy, this PR will be reverted if the buildbot fleet is not fixed in the next 24h. @markshannon @itamaro can you please take a look? I have opened #99976. I will close it if the issue is fixed before the deadline. |
pythonGH-99859 introduced new buildbot failures, as reported [here](python#91054 (comment)). I was able to reproduce the failures with: ``` ./python.exe -m test -v test_capi.test_watchers -m "*TestCodeObjectWatchers*" -R 3:3 ``` The root cause appears to be to static events counters used in the tests, when running the tests with repetitions (using the same interpreter state), the counts from the first test run affected the next runs. This fixes it by resetting the counts when adding and clearing test watchers.
|
Thanks @pablogsal! I have opened GH-99978 to fix the issue. My local repro passes with the fix - could you get all buildbots to test this PR to confirm? @brettcannon I couldn't find anything in this PR that could trigger deep recursion. I wonder if it's the same issue caught by the other buildbots, but manifesting in a different way on the emscripten buildbot? |
Yup, checking! |
|
@itamaro https://github.com/python/cpython/tree/main/Tools/wasm covers how to do anything WebAssembly-related. |
|
thanks @brettcannon, sorry about the delay, I spent the last two days trying to repro and debug this 😬 TLDR: I think this PR may have pushed something over some threshold that caused the junit xml generation to start hitting stack depth limits. (but is not the root cause of the failures) the steps in the docs failed (basically running I was able to repro the failure using some mix of the instructions and reverse engineering the buildbot itself. TLDR build and test steps, inside the docker container: this is more or less what the buildbot does, and it fails (not always, but almost always). this also runs the full test suite, but I was able to narrow it down to running just what makes me think this PR is not the root cause of the regression is that it consistently passes when running just the watchers tests: and what makes me suspect the root cause is in junit xml generation is that dropping that flag causes test_capi to pass consistently: I don't know enough about wasm and junit xml to dig deeper for a fix, but I hope this investigation can provide someone who's more familiar with wasm and junit xml with enough information to take it from here! |
|
Thanks for the research! It looks like yesterday some change was made that brought the builder back to being stable. But if it happens again I will definitely come back to your notes to see if we may need to tweak some code in how we're generating that junit XML output. |
Summary: Backport of python/cpython#99859 (plus some more recent changes.) Reviewed By: alexmalyshev Differential Revision: D47201113 fbshipit-source-id: 9390ca289c043bc56ebfdd2ade63305347190a99
This PR implements gh-91054, allowing extensions to set a callback function to be notified on creation and destruction of code objects
pyperformance results are 1.00x slower (~neutral) using AWS bare metal machine (full comparison gist)
Co-authored-by: @Ye11ow-Flash