FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-125038: redundant GET_ITER instructions are removed from genexpr code by efimov-mikhail · Pull Request #126408 · python/cpython · GitHub

Repository navigation

gh-125038: redundant GET_ITER instructions are removed from genexpr code - #126408

Closed
efimov-mikhail wants to merge 9 commits into
python:mainfrom
efimov-mikhail:issue_125038_get_iter_rework
Closed

efimov-mikhail wants to merge 9 commits into
python:mainfrom
efimov-mikhail:issue_125038_get_iter_rework

Conversation

efimov-mikhail commented Nov 4, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Adds CHECK_ITERABLE instruction. It checks that TOS is iterable or async iterable.
IMO, new simple instruction is a good way to prevent behavior changes.

Redundant GET_ITER instructions are removed from generator expression code.
Those were added during previous fix.


📚 Documentation preview 📚: https://cpython-previews--126408.org.readthedocs.build/

Copy link
Copy Markdown
Member Author

cc @markshannon, @JelleZijlstra

JelleZijlstra self-requested a review November 4, 2024 19:14
ZeroIntensity self-requested a review November 6, 2024 13:55

Copy link
Copy Markdown
Member

I'll do the initial review on this later today. In the meantime, you can rebase against main to fix the JIT failures (see gh-126465).

…nexpr code

Adds CHECK_ITERABLE instruction. It checks that TOS is iterable or
async iterable. Redundant GET_ITER and GET_AITER instructions are
removed from generator expression code.
efimov-mikhail force-pushed the issue_125038_get_iter_rework branch from 9bbdf16 to affb1f8 Compare November 6, 2024 15:36

Copy link
Copy Markdown
Member

For future reference, don't force push. Everything is squashed onto main anyway.

Copy link
Copy Markdown
Member Author

For future reference, don't force push. Everything is squashed onto main anyway.

I've used the Github button "rebase", it made force push.
But local merging with main and pushing to this branch is preffered, isn't it?

Copy link
Copy Markdown
Member

I like to use the "Update branch" button, which merges in main without a need for local actions. We try to avoid force-pushes since they make it harder to track the history of the PR.

ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In general, this is looking pretty good! Several comments, though, as this is a pretty big PR.

Comment thread Doc/library/dis.rst Outdated
Comment thread Python/bytecodes.c Outdated
Comment thread Python/codegen.c
Comment thread Python/bytecodes.c Outdated
Comment thread Python/bytecodes.c
Comment thread Python/bytecodes.c Outdated
Comment thread Lib/test/test_asyncgen.py Outdated
efimov-mikhail and others added 3 commits November 7, 2024 08:47
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
efimov-mikhail marked this pull request as draft November 7, 2024 06:28
efimov-mikhail marked this pull request as ready for review November 7, 2024 19:51
ZeroIntensity previously approved these changes Nov 7, 2024

ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you! I think this is ready for a core dev to review it. @JelleZijlstra, would you mind taking a look?

Copy link
Copy Markdown
Member

I thought the idea was to entirely remove the GET_ITER, now that the generator expression handles the conversion from iterable to iterator.

I realize that would be a slight change, but it makes generator expressions behave the same as generators:

>>> def make_gen(x):
...     for i in x:
...         yield i
...
>>> gen = make_gen(1)  # No error here
>>> next(gen)
Traceback (most recent call last):
  File "<python-input-28>", line 1, in <module>
    next(gen)
    ~~~~^^^^^
  File "<python-input-26>", line 2, in make_gen
    for i in x:
             ^
TypeError: 'int' object is not iterable

Creating a generator with a non-iterable is fine. It is only we try to iterate over it, that an error occurs.

efimov-mikhail commented Nov 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

It's not possible to check arguments of an arbitrary generators, since it can be of any type.
For example, this generator is correct:

def make_range_gen(n, m):
    for i in range(n + m * 5):
        yield i

But generator expressions can have only one parameter, which should be Iterable.
So we can guarantee that argument is really Iterable.

Let's look at the solution with CHECK_ITERABLE bytecode.
Pros:

  1. Backward compatibility in all details.
  2. Early detection of incorrect code in genexprs.
  3. Very small impact on perfomance, almost zero, since implementation of CHECK_ITERABLE is very simple.

Cons:

  1. Small impact on memory size of compiled object (+2 bytes on each creation of genexpr).
  2. New bytecode with little purpose, which uses one of byte values in range(256), and we want to keep those values for important bytecodes.

I'm not sure about the balance of this pros and cons for the typical user of Python.
Backward compatibility seems to be essential advantage.
But, of course, I can remove CHECK_ITERABLE, if you sure that solution without it is better.

@ZeroIntensity, @JelleZijlstra, what do you think?

Copy link
Copy Markdown
Member

Creating a generator with a non-iterable is fine. It is only we try to iterate over it, that an error occurs.

Can we please discuss this a bit more? I don't think that changing where the error happens is safe in terms of backward compatibility.

I am 100% sure that people expect error to happen when the gen is created and I am also sure that the code reflects that (validation, error handling, etc).

If we are going to break this - we need to issue a deprecation period.

I also don't fully understand why we are doing this.

ZeroIntensity dismissed their stale review November 13, 2024 11:46

Sorry, there's enough contention here that I'd like to have my check removed from the PR (it was only supposed to denote "ready for core review," anyway).

efimov-mikhail commented Apr 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

It seems that this PR can be closed because other PR on this topic (#132351) is already merged.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL