Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Stabilize, optimize, and increase robustness of QuickSort #45222
Stabilize, optimize, and increase robustness of QuickSort #45222
Changes from 6 commits
6230587
3db4a31
3318c34
1a30832
0010aaf
d002d88
c241add
7b5cafa
93b80f6
b78b99a
e70ae49
e8db300
40342fe
c4104e1
b1cb56d
f610706
db2761d
a408952
24eacc1
a82087f
1b8168f
a3bf888
d09dc37
1cb2c2b
9942028
822f089
4b231e1
57e4d17
9cf4f73
52f5dfe
4b05e9d
76c606e
5f4388d
5b82421
8d2028b
085c2a7
c11a41b
f724e58
64b57ef
7560e61
11f0e6b
24fd62b
2272c28
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timholy will this cause an invalidation problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, does the compiler need a slightly better algorithm (e.g. median of 3)? I'm not sure that the compiler won't hit the edge cases that would turn this quadratic (and we've been spending a lot of effort removing quadratic compiler problems).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these shenanigans should be unnecessary after #46679 because we will then be able to simply depend on Random.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#47066 alone (not needing all of #46679) should make "does the compiler need a slightly better algorithm (e.g. median of 3)?" outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've switched from midpoint to hashing which comes at a performance penalty but substantially increases robustness. Even with the Random stdlib filtered out, this should be immune to accidental pathological inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for invalidations, I'm no Tim Holy, but I injected the following snippet into Random.jl right before the method redefinition
and got the result
So there are 213 invalidations at that time. Luckely, Random is loaded before precompilation statements are executed. With and without the redefinition in Random.jl I got about* 3670 direct and indirect backedges from
Base.Sort.select_pivot
which makes me think that the invalidated methodinstances get recompiled later on in the build process so there should be no impact on latency after the build finishes.*there is a bit of variability because of my manual interaction with the REPL scrollback buffer which causes various methods to be compiled)
I think this result resolves this conversation.