Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-91054: Add code object watchers API#99859
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…on creation and destruction of PyCodeObjectCo-authored-by: Ye11ow-Flash <janshah@cs.stonybrook.edu>
Co-authored-by: Ye11ow-Flash <janshah@cs.stonybrook.edu>
carljm commentedNov 30, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@itamaro I talked this over briefly with@markshannon and it sounds like we should make the same optimization here as@mpage did in812dd5f for func watchers, to make the check for active watchers a bit more efficient and reduce the overhead on code object creation. |
Uh oh!
There was an error while loading.Please reload this page.
A bit is set in the bit vector iff there is a watcher set at thecorresponding offset in the watcher array. Only notify watchersif 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm OK with this as is, but you could speed this up with an invariant.(active_code_watchers & (1 << i)) impliesinterp->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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cleanup and polish PR (including this optimization) is up atGH-99998
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.
Looks good. Does this complete the set of watchers?
Yep! (At least the ones we had in mind.) |
bedevere-bot commentedDec 2, 2022
|
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 ofhttps://buildbot.python.org/all/#/builders/1050/builds/953/steps/10/logs/stdio). |
pablogsal commentedDec 3, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There are many buildbots failing to build this PR. Checkhttps://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 openedGH-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! |
@itamarohttps://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! |
brettcannon commentedDec 8, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the research! It looks like yesterday some change was made that broughtthe 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 ofpython/cpython#99859 (plus some more recent changes.)Reviewed By: alexmalyshevDifferential Revision: D47201113fbshipit-source-id: 9390ca289c043bc56ebfdd2ade63305347190a99
Uh oh!
There was an error while loading.Please reload this page.
This PR implementsgh-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