Skip to content

Conversation

@thatbirdguythatuknownot
Copy link
Contributor

@thatbirdguythatuknownot thatbirdguythatuknownot commented May 4, 2022

issue: #91247

@AlexWaygood AlexWaygood added type-feature A feature request or enhancement performance Performance or resource usage labels May 4, 2022
Copy link
Contributor

@MojoVampire MojoVampire left a comment

Choose a reason for hiding this comment

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

You're introducing undefined behavior, and making many changes that aren't related to the actual performance goals of the patch. The actual part that relates to performance (at the very end of the patch) seems okay, but most of the rest of it should be rolled back, as it's unnecessary, unhelpful, or actively harmful.

Copy link
Contributor

@MojoVampire MojoVampire left a comment

Choose a reason for hiding this comment

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

Please stop pushing completely unverified code. Make it work, then submit the PR. A handful of commits in a given PR are not unreasonable, but seven commits in the space of minutes, most of them not even high enough quality to compile, let alone work correctly, does not a good PR request make. Make something that works and you're mostly confident of, minimize the number of changes not required to implement the fix (no pointless/harmful shoving assignments in the middle of larger expressions), and squash the commits down to just one or two for the bulk of the actual work.

@MojoVampire MojoVampire added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label May 4, 2022
@thatbirdguythatuknownot
Copy link
Contributor Author

Please stop pushing completely unverified code. Make it work, then submit the PR. A handful of commits in a given PR are not unreasonable, but seven commits in the space of minutes, most of them not even high enough quality to compile, let alone work correctly, does not a good PR request make. Make something that works and you're mostly confident of, minimize the number of changes not required to implement the fix (no pointless/harmful shoving assignments in the middle of larger expressions), and squash the commits down to just one or two for the bulk of the actual work.

I know that, but I usually can't verify it because I make changes on the desktop website (I don't know how to use git).

@MojoVampire
Copy link
Contributor

I know that, but I usually can't verify it because I make changes on the desktop website (I don't know how to use git).

You're going to have to learn. It's literally the first thing you do to begin contributing. See the Python Developer's Guide: Setup. You can make all the changes you need to locally, build and verify them, all without pushing a single thing to GitHub or opening a PR.

@thatbirdguythatuknownot
Copy link
Contributor Author

thatbirdguythatuknownot commented May 4, 2022

You're going to have to learn. It's literally the first thing you do to begin contributing. See the Python Developer's Guide: Setup. You can make all the changes you need to locally, build and verify them, all without pushing a single thing to GitHub or opening a PR.

I have a local working modified copy of the repo (not by git) but I forgot to check against it.

@eendebakpt
Copy link
Contributor

@thatbirdguythatuknownot @MojoVampire This PR has the same goal and approach as #91247. That PR and the corresponding issue has a detailed analysis of the performance impact (which is quite subtle)

@eendebakpt
Copy link
Contributor

The actual pr is #91482

@methane methane changed the title gh-92285: Performance improvement in list repeating gh-91247: Performance improvement in list repeating May 4, 2022
@iritkatriel
Copy link
Member

This is out of date. Closing. Thank you @thatbirdguythatuknownot .

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

Labels

awaiting core review interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants