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.

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
+20 −200

Conversation

brandtbucher
Copy link
Member

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

Automerge-Triggered-By: GH:brandtbucher

@brandtbucher brandtbucher added interpreter-core needs backport to 3.11 labels Jul 22, 2022
@brandtbucher brandtbucher self-assigned this Jul 22, 2022
@brandtbucher brandtbucher requested a review from markshannon as a code owner Jul 22, 2022
@brandtbucher brandtbucher mentioned this pull request Jul 22, 2022
Closed
markshannon
markshannon requested changes Jul 22, 2022
View changes
Copy link
Member

@markshannon markshannon left a comment

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

@bedevere-bot bedevere-bot commented Jul 22, 2022

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

@markshannon
Copy link
Member

@markshannon markshannon commented Jul 22, 2022

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 changes
Copy link
Member

@sweeneyde sweeneyde left a comment

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 added 🤖 automerge and removed 🤖 automerge labels Jul 22, 2022
@brandtbucher brandtbucher merged commit e402b26 into python:main Jul 22, 2022
14 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 22, 2022

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 Jul 22, 2022
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 22, 2022

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 issue 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 <brandtbucher@microsoft.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 22, 2022

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

brandtbucher added a commit that referenced this issue 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

@sweeneyde sweeneyde

@pablogsal pablogsal

Assignees

@brandtbucher brandtbucher

Labels
interpreter-core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@brandtbucher @bedevere-bot @markshannon @miss-islington @sweeneyde