Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-134728: Don't deopt due to eval breaker in_TIER2_RESUME_CHECK#134729
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Thanks for the review request! I have two comments :)
Uh oh!
There was an error while loading.Please reload this page.
| if (eval_breaker & _PY_EVAL_EVENTS_MASK) { | ||
| int err = _Py_HandlePending(tstate); | ||
| ERROR_IF(err != 0); | ||
| } |
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.
Just throwing this out there since I don't yet know enough about these specific instructions, but with this change,_TIER2_RESUME_CHECK is now looking quite similar to_CHECK_PERIODIC. Is there any way we can combine them? Or do we need to keep them separate?
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.
Possibly could replace _TIER2_RESUME_CHECK with _CHECK_PERIODIC.
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.
Ok I think maybe not, the assertions in TIER2_RESUME_CHECK are different.
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.
Again, I don't fully understand this part so my approval doesn't count for much, but the changes look good to me, judging by what we have in_CHECK_PERIODIC and_CHECK_PERIODIC_IF_NOT_YIELD_FROM :)
Btw, Mark left a comment on the issue:#134728 (comment), not sure if you've seen it.
Uh oh!
There was an error while loading.Please reload this page.
On this benchmark (unscientific, noisy)https://gricad-gitlab.univ-grenoble-alpes.fr/augierpi/augierpi.gricad-pages.univ-grenoble-alpes.fr/-/blob/branch/default/content/docs/2025/about-py-jit/bench_loops_sum.py, I get the following results:
So nearly a 10% improvement!