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.

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

Make QuickerSort efficient for non-homogonous eltype #47973

Merged
merged 4 commits into from
Jan 5, 2023

Conversation

LilithHafner
Copy link
Member

Set v[j] = pivot in partition! rather than returning pivot to caller to make partition! type stable for non-concrete eltype

This is intended to address the regression in master vs 1.8 revealed by nanosoldier

["union", "array", ("sort", "Union{Nothing, Bool}", 0)] 1.37 (5%) ❌ 3.01 (1%) ❌

[link to motivating benchmark results]

Lilith Hafner added 2 commits December 22, 2022 18:09
… to make partition! type stable for non-concrete eltype
@LilithHafner LilithHafner added performance Must go faster sorting Put things in order backport 1.9 Change should be backported to release-1.9 labels Dec 23, 2022
@LilithHafner
Copy link
Member Author

LilithHafner commented Dec 23, 2022

@nanosoldier runbenchmarks("sort", vs=":master")
@nanosoldier runbenchmarks("sort", vs=":release-1.8") [did not run]

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

@LilithHafner
Copy link
Member Author

@nanosoldier runbenchmarks("sort", vs=":release-1.8")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@LilithHafner
Copy link
Member Author

Much better, though still not perfect

["union", "array", ("sort", "Union{Nothing, Bool}", 0)] 1.00 (5%) 1.33 (1%) ❌

With this and #47966 I believe all the BaseBenchmark regressions in runtime will be gone, though regressions in allocations will remain & some will not correspond to compensating improvements in runtime. Further, using JuliaCI/BaseBenchmarks.jl#305 will also likely still show real regressions. I'll keep looking for regressions once this and #47966 merge.

I'll post here when I get the benchmark results from running JuliaCI/BaseBenchmarks.jl#305 locally across this PR.

@LilithHafner
Copy link
Member Author

Local tests reveal no regressions & memory improvements for "mixed eltype with by order"

@LilithHafner
Copy link
Member Author

LilithHafner commented Dec 27, 2022

Bump. I think this is ready to go. The remaining more minor regression is likely due to a different underlying cause. I'll probably merge it in a couple of days if there are no objections

@KristofferC KristofferC mentioned this pull request Dec 28, 2022
14 tasks
@LilithHafner
Copy link
Member Author

This is orthogonal to #47966 so I'm not concerned about semantic merge conflicts

@LilithHafner
Copy link
Member Author

Running nanosoldier here to search for possible remaining regressions to fix before we release 1.9

@nanosoldier runbenchmarks("sort" vs=":release-1.8")

@LilithHafner
Copy link
Member Author

@nanosoldier runbenchmarks("sort", vs=":release-1.8")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@KristofferC KristofferC mentioned this pull request Jan 2, 2023
41 tasks
@KristofferC KristofferC merged commit 54aa57c into master Jan 5, 2023
@KristofferC KristofferC deleted the sort-union-eltype branch January 5, 2023 20:15
@StefanKarpinski
Copy link
Member

Is QuickerSort what you ended up calling your stable quicksort algorithm?

@LilithHafner
Copy link
Member Author

Yes (reasoning); it is probably not too late to rename it if you have a better idea. (nobody seems to be using it by name)

@StefanKarpinski
Copy link
Member

I think calling it StableQuickSort would be a slightly less loaded choice, but I see you address that there. If I'm understanding the reasoning, it's that your modified quicksort is stable in its current form but could be modified to be still out of place but unstable and would do less work in that form, so there's a potential for a faster unstable variation of it, which would make the name "StableQuickSort" confusing. Is that correct?

@LilithHafner
Copy link
Member Author

Precisely.

@StefanKarpinski
Copy link
Member

I still think StableQuickSort would be ok since the most common version would be stable (you can almost always make something unstable somehow). Or maybe QuickBufferSort? But QuickerSort is ok too I guess.

@LilithHafner
Copy link
Member Author

I prefer QuickBufferSort over StableQuickSort. I don't feel strongly either way between QuickBufferSort (proposed) and QuickerSort (current). If you or others feel strongly enough to rename it, I'd be happy to make a PR. It should be technically trivial.

KristofferC pushed a commit that referenced this pull request Jan 10, 2023
* set `v[j] = pivot` in partition rather than returning pivot to caller to make partition! type stable for non-concrete eltype

(cherry picked from commit 54aa57c)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants