Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GH-95113: Don't use EXTENDED_ARG_QUICK in unquickened code #95121

Merged
merged 3 commits into from
Jul 22, 2022
Merged

GH-95113: Don't use EXTENDED_ARG_QUICK in unquickened code #95121

merged 3 commits into from
Jul 22, 2022

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jul 22, 2022 •
edited by miss-islington
Loading

Automerge-Triggered-By: GH:brandtbucher

@brandtbucher brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) needs backport to 3.11 only security fixes labels Jul 22, 2022
@brandtbucher brandtbucher self-assigned this Jul 22, 2022
@brandtbucher brandtbucher mentioned this pull request Jul 22, 2022
Closed
markshannon
markshannon requested changes Jul 22, 2022
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory the unquickened form of EXTENDED_ARG should be very rare, so this should have no effect on performance.
So I'm not worried about that.

Could you add a comment to EXTENDED_ARG explaining why it uses _PyOpcode_Deopt[opcode] rather than opcode.
Other than that LGTM.

@bedevere-bot
Copy link

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

@markshannon
Copy link
Member

One other thing that might merit a comment.
If RESUME were prefixed by EXTENDED_ARG then the RESUME would not trace correctly.
Since RESUME only has a meaningful oparg in range(3) this shouldn't happen, but might be worth making that explicit somewhere.

sweeneyde
sweeneyde approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@sweeneyde sweeneyde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes things conceptually simpler:

  • EXTENDED_ARG/EXTENDED_ARG_QUICK is now more similar to the JUMP_BACKWARD/JUMP_BACKWARD_QUICK situation.
  • It avoids special-casing EXTENDED_ARG with a separate opcode table.
  • It avoids leaking the _QUICK terminology to dis users that theoretically shouldn't have to care.

The tiny downside is that unquickened EXTENDED_ARGs need one more _PyOpcode_Deopt[...] memory access, which should be totally insignificant, so LGTM.

@brandtbucher brandtbucher merged commit e402b26 into python:main Jul 22, 2022
@miss-islington
Copy link
Contributor

Thanks @brandtbucher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@brandtbucher brandtbucher deleted the extended-arg-not-quick branch July 22, 2022 18:04
@miss-islington
Copy link
Contributor

Sorry, @brandtbucher, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker e402b26b7fb953a2f0c17a0044bb6d6cbd726e54 3.11

brandtbucher added a commit to brandtbucher/cpython that referenced this pull request Jul 22, 2022
@brandtbucher
[3.11] pythonGH-95113: Don't use EXTENDED_ARG_QUICK in unquickened co…
7384e2e 
…de (pythonGH-95121).

(cherry picked from commit e402b26)

Co-authored-by: Brandt Bucher <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 22, 2022
@bedevere-bot
Copy link

GH-95143 is a backport of this pull request to the 3.11 branch.

brandtbucher added a commit that referenced this pull request Jul 22, 2022
@brandtbucher
[3.11] GH-95113: Don't use EXTENDED_ARG_QUICK in unquickened code (GH…
5a48ab0 
…-95121) (GH-95143)

(cherry picked from commit e402b26)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon markshannon requested changes

@sweeneyde sweeneyde sweeneyde approved these changes

@pablogsal pablogsal Awaiting requested review from pablogsal

Assignees

@brandtbucher brandtbucher

Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brandtbucher @bedevere-bot @markshannon @miss-islington @sweeneyde