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

Remove unnecessary LIST_TO_TUPLE conversions #126558

Merged
merged 4 commits into from
Jan 8, 2025
Merged

Remove unnecessary LIST_TO_TUPLE conversions #126558

merged 4 commits into from
Jan 8, 2025

Conversation

brandtbucher
Copy link
Member

This probably doesn't need an issue or NEWS, it's just a little inefficiency @markshannon noticed earlier today. Basically, it doesn't matter if we're actually iterating over a tuple, so if we already have a list with the correct items, use that instead.

@brandtbucher brandtbucher added performance Performance or resource usage skip issue skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Nov 7, 2024
@brandtbucher brandtbucher self-assigned this Nov 7, 2024
@brandtbucher brandtbucher requested review from markshannon and iritkatriel as code owners November 7, 2024 21:21
@bedevere-app bedevere-app bot added the awaiting core review label Nov 7, 2024
markshannon
markshannon requested changes Nov 12, 2024
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.

Looks sound.

I thought this might break the following code:

def f():                                                                                                                                       
    for i in (*(l:= [1,2,3]),):
        x = l.pop()
        print(i)

It doesn't seem to as it builds a fresh list before converting it to a tuple, but could you add this code as a test case.
Otherwise things might break if we remove the extra LIST_EXTEND

@bedevere-app bedevere-app bot removed the awaiting core review label Nov 12, 2024
@bedevere-app
Copy link

bedevere-app bot commented Nov 12, 2024

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

@bedevere-app bedevere-app bot added the awaiting changes label Nov 12, 2024
@brandtbucher
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-app bedevere-app bot added awaiting change review and removed awaiting changes labels Jan 8, 2025
@bedevere-app
Copy link

bedevere-app bot commented Jan 8, 2025

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from markshannon January 8, 2025 02:19
markshannon
markshannon approved these changes Jan 8, 2025
View reviewed changes
@bedevere-app bedevere-app bot added awaiting merge and removed awaiting change review labels Jan 8, 2025
@brandtbucher brandtbucher merged commit 004f9fd into python:main Jan 8, 2025
44 of 47 checks passed
@bedevere-app bedevere-app bot removed the awaiting merge label Jan 8, 2025
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 RHEL8 LTO + PGO 3.x has failed when building commit 004f9fd.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/78/builds/8163) and take a look at the build logs.
  4. Check if the failure is related to this commit (004f9fd) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/78/builds/8163

Summary of the results of the build (if available):

==

Click to see traceback logs 
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64.lto-pgo/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64.lto-pgo/build/Lib/threading.py", line 996, in run
    self.__target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64.lto-pgo/build/Lib/test/test_interpreters/test_stress.py", line 30, in task
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64.lto-pgo/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64.lto-pgo/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64.lto-pgo/build/Lib/threading.py", line 996, in run
    self.__target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64.lto-pgo/build/Lib/test/test_interpreters/test_stress.py", line 47, in run
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64.lto-pgo/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon markshannon approved these changes

@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

Assignees

@brandtbucher brandtbucher

Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brandtbucher @bedevere-bot @markshannon
  NODES
COMMUNITY 2
Project 5
USERS 1