-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[perf] Use std based dedup in projection #47656
Conversation
Unstable sort was added recently, and the code that is being modified is 3 years old. As quicksort doesn't allocate it will likely perform as well as, or better than linear search.
(rust_highfive has picked a reviewer for you, use r? to override) |
Hmm, so basically sorting fails because most types do not implement Ord. Is it a good idea to add derivation of them in this PR? |
@ishitatsuyuki Let's add it, we need a perf check anyway. If it's unacceptable we could use |
Seems fine. I'd like to see some perf results, for sure -- even a local run on some non-trivial test case might be of interest. |
b89ebbb
to
732ee67
Compare
732ee67
to
c6772b4
Compare
Unwipped. Please perform a try and/or perf. |
@bors try |
⌛ Trying commit c6772b4 with merge 77de396b7685c2d4870314ad7ca865ad7b6feb5c... |
☀️ Test successful - status-travis |
@Mark-Simulacrum please perform a perf run. |
Perf queued. |
... and well, surprisingly no noticable time change except one (see below). To sum up, the code is now cleaner and resistant to quadratic blowup, but some Ord implementations are added.
EDIT: this time is quite fluctuating and should not be used as a metric. |
@bors r+ |
📌 Commit c6772b4 has been approved by |
[perf] Use std based dedup in projection Unstable sort was added recently, and the code that is being modified is 3 years old. As quicksort doesn't allocate it will likely perform as well as, or better than linear search. I didn't benchmark. Have a perf run.
Unstable sort was added recently, and the code that is being modified is 3 years old. As quicksort doesn't allocate it will likely perform as well as, or better than linear search.
I didn't benchmark. Have a perf run.