Skip to content

[3.11] GH-93354: Use exponential backoff to avoid excessive specialization attempts (GH-93355)#93379

Merged
ambv merged 4 commits into3.11from
backport-eb618d5-3.11
Jun 30, 2022
Merged

[3.11] GH-93354: Use exponential backoff to avoid excessive specialization attempts (GH-93355)#93379
ambv merged 4 commits into3.11from
backport-eb618d5-3.11

Conversation

@markshannon
Copy link
Copy Markdown
Member

@markshannon markshannon commented May 31, 2022 •
edited by bedevere-bot
Loading

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

@markshannon
[3.11] GH-93354: Use exponential backoff to avoid excessive specializ…
852dca8 
…ation attempts. (GH-93355)

(cherry picked from commit eb618d5)

Co-authored-by: Mark Shannon <mark@hotpy.org>
@bedevere-bot bedevere-bot mentioned this pull request May 31, 2022
Merged
@pablogsal
Copy link
Copy Markdown
Member

pablogsal commented May 31, 2022

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.

@markshannon
Copy link
Copy Markdown
Member Author

markshannon commented May 31, 2022

Seems there is a bunch of compiler warnings

Cherry picker messed up the merge. I'll fix that.

Fidget-Spinner
Fidget-Spinner approved these changes Jun 11, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner left a 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.

@gvanrossum gvanrossum self-requested a review June 13, 2022 16:37
Fidget-Spinner
Fidget-Spinner requested changes Jun 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner left a comment •
edited
Loading

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:

cpython/Python/ceval.c

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.

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jun 15, 2022

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@Fidget-Spinner
Copy link
Copy Markdown
Member

Fidget-Spinner commented Jun 15, 2022

I suddenly noticed on my main branch that it now takes 53 misses to deopt an instruction. That seems high.

I opened #93860 to fix that.

@markshannon
Copy link
Copy Markdown
Member Author

markshannon commented Jun 15, 2022

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.

Fidget-Spinner
Fidget-Spinner approved these changes Jun 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner left a 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.

@pablogsal
Copy link
Copy Markdown
Member

pablogsal commented Jun 23, 2022 •
edited
Loading

@markshannon Can you please rebase this so I can land it to the new beta?

We are still missing another core dev approval

@ambv
Copy link
Copy Markdown
Contributor

ambv commented Jun 30, 2022

I went through the PR, compared it with main, built a pydebug and a macOS framework release Python with this PR, ran regrtest on both. Looks good to me.

@ambv ambv merged commit 113b309 into 3.11 Jun 30, 2022
@ambv ambv deleted the backport-eb618d5-3.11 branch June 30, 2022 21:03
@brandtbucher brandtbucher mentioned this pull request Oct 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

@brandtbucher brandtbucher Awaiting requested review from brandtbucher

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@markshannon @pablogsal @bedevere-bot @Fidget-Spinner @ambv