Optimize median calculation by using nth_element instead of partial_sort - #565
Conversation
|
❌ Build benchmark 1166 failed (commit eefd4f9782 by @FredTingaud) |
|
✅ Build benchmark 1167 completed (commit 11651a3e48 by @FredTingaud) |
|
Ironically, I feel I should ask if there is anyway you can benchmark the perf improvements for this change? |
|
The url I put in the commit message compares the previous and the new implementations (and it uses google benchmark). |
|
Checking a few sizes, it looks like it scales as power series in both cases (unsurprisingly) but the nopartial is indeed ~10x faster. For very small vectors (N < 100), the existing method is marginally faster. The only issue might be the memory overhead which, if i'm understanding correctly, is roughly 2x for the full copy (assuming the partial sort copy is done in-place). This trade-off seems reasonable to me, and the code is simpler to read. |
|
The memory footprint is indeed twice the previous one, as partial_sort works with no overhead. Off topic question: I have a bigger changelist I have been working on, that is not finished and that might not be doable at all. Is there a channel through which I could ask you questions about it? |
|
This PR LGTM then. I'll merge it shortly. We're on #googlebenchmark on freenode IRC, or the google group at https://groups.google.com/d/forum/benchmark-discuss, or you can just open an issue here. |
I hope this PR doesn't look too pedantic as it is not correcting an issue but just optimizing a function that might not have a particular performance problem.
It just happened that I stumbled upon this code while writing a talk about how bad partial_sort is at operations like median calculation, so I checked and copying the whole vector then using one or two nth_element calls is indeed 10x faster:
http://quick-bench.com/G0c2a4neSNwcAYcazj_YW5QxBrw