Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

GH-116554: Relax list.sort()'s notion of "descending" runs#116578

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tim-one merged 21 commits intopython:mainfromtim-one:descend
Mar 13, 2024

Conversation

@tim-one
Copy link
Member

@tim-onetim-one commentedMar 10, 2024
edited by bedevere-appbot
Loading

Not yet done, but good enough for timing.

tim-oneand others added5 commitsMarch 10, 2024 19:19
`sortslice_reverse()`. The current    sortslice_advance(&slice, n - neq);    reverse_sortslice(&slice, neq);pair of lines is an affront to the soul ;-) Puttingthe compound name io    <OBJECT TYPE NAME>_<METHOD NAME>order feels significantly more natural.
with few distinct elements. It's in the nature of the beast thatthis will catch nearly all plausible "off by 1" coding errorsin this context.
Switching to what I guess are some tool's different spelling of GH's single backticks.
@tim-one
Copy link
MemberAuthor

This looks good to go to me. I wasn't able to provoke any errors in the code, and didn't find any significant timing surprises (compared to the currentmain branch). The OP's original "problem case" (on StackOverflow) runs much faster now, and, as expected, is down to about 1.5 compares per element (independent of list size - the first call ofcount_run() now sorts the whole thing in-place).

Reviews always appreciated, but if a few days pass without one, I'll just commit it anyway.

tim-oneand others added12 commitsMarch 11, 2024 18:36
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
…e-116554.gYumG5.rstCo-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
I kept `lo` mostly to reduce fiddly typing needed for IFLT arguments.But IFLT calls were tedious and error-prone too. So introduced twotiny macros to capture the gibberish needed to spell "is the nextelement smaller/larger?" once and for all. There was no real useleft for `lo` then, so got rid of it.Although a vrbl named `lo` still exists. But with a different meaning.It's a const capturing slo->keys, viewed as an array for `n` to index.A modern optimizing compiler shouldn't need my help to realize thatmarching through an array one at a time can be strength-reduced topointer increments (which is what the old `lo` did).
thing we should be oprimizi9ng for ;-) Seriously, they'llreturn very early anyway, as a matter of course, after thefirst loop terminates without doing anything, and then the`n == nremaining` test will pass.
@tim-onetim-one merged commitbf121d6 intopython:mainMar 13, 2024
@tim-onetim-one deleted the descend branchMarch 13, 2024 01:01
vstinner pushed a commit to vstinner/cpython that referenced this pull requestMar 20, 2024
…hon#116578)*pythonGH-116554: Relax list.sort()'s notion of "descending" runRewrote `count_run()` so that sub-runs of equal elements no longer end a descending run. Both ascending and descending runs can have arbitrarily many sub-runs of arbitrarily many equal elements now. This is tricky, because we only use ``<`` comparisons, so checking for equality doesn't come "for free". Surprisingly, it turned out there's a very cheap (one comparison) way to determine whether an ascending run consisted of all-equal elements. That sealed the deal.In addition, after a descending run is reversed in-place, we now go on to see whether it can be extended by an ascending run that just happens to be adjacent. This succeeds in finding at least one additional element to append about half the time, and so appears to more than repay its cost (the savings come from getting to skip a binary search, when a short run is artificially forced to length MIINRUN later, for each new element `count_run()` can add to the initial run).While these have been in the back of my mind for years, a question on StackOverflow pushed it to action:https://stackoverflow.com/questions/78108792/They were wondering why it took about 4x longer to sort a list like:[999_999, 999_999, ..., 2, 2, 1, 1, 0, 0]than "similar" lists. Of course that runs very much faster after this patch.Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
adorilson pushed a commit to adorilson/cpython that referenced this pull requestMar 25, 2024
…hon#116578)*pythonGH-116554: Relax list.sort()'s notion of "descending" runRewrote `count_run()` so that sub-runs of equal elements no longer end a descending run. Both ascending and descending runs can have arbitrarily many sub-runs of arbitrarily many equal elements now. This is tricky, because we only use ``<`` comparisons, so checking for equality doesn't come "for free". Surprisingly, it turned out there's a very cheap (one comparison) way to determine whether an ascending run consisted of all-equal elements. That sealed the deal.In addition, after a descending run is reversed in-place, we now go on to see whether it can be extended by an ascending run that just happens to be adjacent. This succeeds in finding at least one additional element to append about half the time, and so appears to more than repay its cost (the savings come from getting to skip a binary search, when a short run is artificially forced to length MIINRUN later, for each new element `count_run()` can add to the initial run).While these have been in the back of my mind for years, a question on StackOverflow pushed it to action:https://stackoverflow.com/questions/78108792/They were wondering why it took about 4x longer to sort a list like:[999_999, 999_999, ..., 2, 2, 1, 1, 0, 0]than "similar" lists. Of course that runs very much faster after this patch.Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
…hon#116578)*pythonGH-116554: Relax list.sort()'s notion of "descending" runRewrote `count_run()` so that sub-runs of equal elements no longer end a descending run. Both ascending and descending runs can have arbitrarily many sub-runs of arbitrarily many equal elements now. This is tricky, because we only use ``<`` comparisons, so checking for equality doesn't come "for free". Surprisingly, it turned out there's a very cheap (one comparison) way to determine whether an ascending run consisted of all-equal elements. That sealed the deal.In addition, after a descending run is reversed in-place, we now go on to see whether it can be extended by an ascending run that just happens to be adjacent. This succeeds in finding at least one additional element to append about half the time, and so appears to more than repay its cost (the savings come from getting to skip a binary search, when a short run is artificially forced to length MIINRUN later, for each new element `count_run()` can add to the initial run).While these have been in the back of my mind for years, a question on StackOverflow pushed it to action:https://stackoverflow.com/questions/78108792/They were wondering why it took about 4x longer to sort a list like:[999_999, 999_999, ..., 2, 2, 1, 1, 0, 0]than "similar" lists. Of course that runs very much faster after this patch.Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
@nanonyme
Copy link

This seems to be resulting in sorting changes between 3.12 and 3.13. Is the sort still Timsort after this? The descending criteria seems to now conflict withhttps://en.m.wikipedia.org/wiki/Timsort

@mwtoewsmwtoews mentioned this pull requestMar 4, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@eendebakpteendebakpteendebakpt left review comments

@AlexWaygoodAlexWaygoodAlexWaygood left review comments

Assignees

No one assigned

Labels

interpreter-core(Objects, Python, Grammar, and Parser dirs)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tim-one@nanonyme@eendebakpt@AlexWaygood

[8]ページ先頭

©2009-2025 Movatter.jp