GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-132554: Fix tier2 `FOR_ITER` implementation and optimizations by markshannon · Pull Request #135137 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Jun 4, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

When adding virtual iterators, the tier 1 and tier 2 implementations of FOR_ITER diverged. I've already fixed a problem where the instrumented FOR_ITER differed from the normal one.

To prevent these problems happening again, this PR factors out the majority of FOR_ITER into a helper function for the 3 versions of FOR_ITER to share.

I've also added PyStackRef_ERROR to distinguish between errors and no result and remove the need for an additional out parameter for the helper function.

Also fixes a bug in the code generator where there are three or more output values, one is an unchanged input, one is a changed input and one is undefined.

  • Issue: Use tagged ints for faster iteration #132554

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
markshannon added 3 commits June 4, 2025 10:56
@markshannon
Fix FOR_ITER for tier 2
eedbe8b
@markshannon
Add test
a10eadb
@markshannon
Update debug stackref
5725821
@markshannon
markshannon requested a review from Fidget-Spinner as a code owner June 4, 2025 14:09
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 4, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Jun 4, 2025
Use tagged ints for faster iteration #132554
Open
@markshannon
markshannon requested review from brandtbucher and removed request for Fidget-Spinner June 4, 2025 14:43
@markshannon markshannon added the skip news label Jun 4, 2025
@Fidget-Spinner

Fidget-Spinner commented Jun 4, 2025

Copy link
Copy Markdown
Member

Can you check out the Windows JIT failures please? I'll review the PR after that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brandtbucher
brandtbucher approved these changes Jun 5, 2025
View reviewed changes
Comment thread Include/internal/pycore_ceval.h Outdated
#endif

_PyStackRef _PyForIter_NextWithIndex(PyObject *seq, _PyStackRef index);
_PyStackRef

@brandtbucher brandtbucher Jun 5, 2025

Copy link
Copy Markdown
Member

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.

Suggested change
_PyStackRef
PyAPI_FUNC(_PyStackRef)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/optimizer_bytecodes.c Outdated
op(_GET_ITER, (iterable -- iter, index_or_null)) {
if (sym_matches_type(iterable, &PyTuple_Type) || sym_matches_type(iterable, &PyList_Type)) {
iter = iterable;
index_or_null = sym_new_type(ctx, &PyLong_Type);

@brandtbucher brandtbucher Jun 5, 2025

Copy link
Copy Markdown
Member

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.

Hm, this is sort of weird. We don't have a symbol for "unboxed int" in the JIT, but it really doesn't feel correct to type this as int. Maybe leave as unknown and we can update our lattice with unboxed C types later? It's not like this information is being used yet, anyways.

Suggested change
index_or_null = sym_new_type(ctx, &PyLong_Type);
index_or_null = sym_new_unknown(ctx);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/bytecodes.c
ERROR_NO_POP();
}
next = PyStackRef_FromPyObjectSteal(next_o);
JUMPBY(oparg + 1);

@brandtbucher brandtbucher Jun 5, 2025

Copy link
Copy Markdown
Member

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.

Can you add back the comment that this is skipping the END_FOR?

Suggested change
JUMPBY(oparg + 1);
// Jump forward by oparg, then skip the following END_FOR:
JUMPBY(oparg + 1);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c
if (PyStackRef_IsError(item)) {
ERROR_NO_POP();
}
JUMPBY(oparg + 1);

@brandtbucher brandtbucher Jun 5, 2025

Copy link
Copy Markdown
Member

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.

Suggested change
JUMPBY(oparg + 1);
// Jump forward by oparg, then skip the following END_FOR:
JUMPBY(oparg + 1);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/ceval.c Outdated
_PyErr_Clear(tstate);
}
else {
return PyStackRef_ERROR;

@brandtbucher brandtbucher Jun 5, 2025

Copy link
Copy Markdown
Member

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.

Suggested change
return PyStackRef_ERROR;
return PyStackRef_ERROR;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jun 5, 2025
@brandtbucher

brandtbucher commented Jun 5, 2025

Copy link
Copy Markdown
Member

Confirmed that this fixed the pprint benchmarks locally. Just kicked off new benchmarks now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
Address review comments
cc09774
@brandtbucher

brandtbucher commented Jun 5, 2025

Copy link
Copy Markdown
Member

27% slower... ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
markshannon merged commit b90ecea into python:main Jun 5, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Jun 5, 2025
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@markshannon @Pranjal095
pythonGH-132554: Fix tier2 FOR_ITER implementation and optimizations (
3557d29 
pythonGH-135137)
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@markshannon @taegyunkim
pythonGH-132554: Fix tier2 FOR_ITER implementation and optimizations (
dd6f0ca 
pythonGH-135137)
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
@markshannon @Agent-Hellboy
pythonGH-132554: Fix tier2 FOR_ITER implementation and optimizations (
e7ac90f 
pythonGH-135137)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@markshannon @Fidget-Spinner @brandtbucher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.