-
-
Notifications
You must be signed in to change notification settings - Fork 32k
[3.11] GH-93354: Use exponential backoff to avoid excessive specialization attempts (GH-93355) #93379
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
Conversation
…ation attempts. (GH-93355) (cherry picked from commit eb618d5) Co-authored-by: Mark Shannon <[email protected]>
Seems there is a bunch of compiler warnings (check the "Files" tab in this PR). Also, there is a lot of code here and I am not sure I feel very confident merging this so I suppose it depends on how bd is the regression we are fixing. Could you elaborate with some benchmarks on the cases this is fixing (I know there is an explanation on the issue, but I want some discussion here specific for 3.11). Also, if we decided to go with this, we are going to need at least 2 other core devs to review and approve this change. |
Cherry picker messed up the merge. I'll fix that. |
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.
This LGTM.
Re: what Pablo said. IMO the risk of introducing bugs with this change is low. At worst we degrade performance a little. I don't see how this could introduce anything majorly breaking.
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.
Wait, I think we need to apply the changes to these lines too:
Lines 5605 to 5613 in 6b33000
miss: | |
{ | |
STAT_INC(opcode, miss); | |
opcode = _PyOpcode_Deopt[opcode]; | |
STAT_INC(opcode, miss); | |
/* The counter is always the first cache entry: */ | |
_Py_CODEUNIT *counter = (_Py_CODEUNIT *)next_instr; | |
*counter -= 1; | |
if (*counter == 0) { |
I suddenly noticed on my main branch that it now takes 53 misses to deopt an instruction. That seems high.
When you're done making the requested changes, leave the comment: |
I opened #93860 to fix that. |
There is no change. 53 is the same as before. This PR changes the number of executions to the next specialization attempt, not the number of misses before deoptimization. |
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.
My bad, that was confusion on my part. Once again this LGTM.
@markshannon Can you please rebase this so I can land it to the new beta? We are still missing another core dev approval |
I went through the PR, compared it with |
Backport of #93355
This doesn't fix any obvious bugs in 3.11, but it is a preventative fix.
Specialization of some instructions can be quite expensive (relative to the cost of executing the instruction), so we want to avoid excessive numbers of failed specializations for corner cases that we might not yet be aware of.
@pablogsal