gh-134728: Don't deopt due to eval breaker in _TIER2_RESUME_CHECK#134729
gh-134728: Don't deopt due to eval breaker in _TIER2_RESUME_CHECK#134729Fidget-Spinner wants to merge 3 commits intopython:mainfrom
_TIER2_RESUME_CHECK#134729Conversation
Fidget-Spinner
requested review from
brandtbucher and
tomasr8
and removed request for
markshannon
tomasr8
left a comment
tomasr8
left a comment
There was a problem hiding this comment.
Thanks for the review request! I have two comments :)
| if (eval_breaker & _PY_EVAL_EVENTS_MASK) { | ||
| int err = _Py_HandlePending(tstate); | ||
| ERROR_IF(err != 0); | ||
| } |
There was a problem hiding this comment.
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.
Possibly could replace _TIER2_RESUME_CHECK with _CHECK_PERIODIC.
There was a problem hiding this comment.
Ok I think maybe not, the assertions in TIER2_RESUME_CHECK are different.
tomasr8
left a comment
tomasr8
left a comment
There was a problem hiding this comment.
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.
2 participants
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!