Skip to content

Optimize median calculation by using nth_element instead of partial_sort - #565

Merged
dmah42 merged 1 commit into
google:masterfrom
FredTingaud:master
Apr 9, 2018
Merged

Optimize median calculation by using nth_element instead of partial_sort#565
dmah42 merged 1 commit into
google:masterfrom
FredTingaud:master

Conversation

@FredTingaud

Copy link
Copy Markdown
Contributor

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

@AppVeyorBot

Copy link
Copy Markdown

Build benchmark 1166 failed (commit eefd4f9782 by @FredTingaud)

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 87.07% when pulling 9a34a24 on FredTingaud:master into 2844167 on google:master.

@AppVeyorBot

Copy link
Copy Markdown

@EricWF

EricWF commented Apr 8, 2018

Copy link
Copy Markdown
Contributor

Ironically, I feel I should ask if there is anyway you can benchmark the perf improvements for this change?

@FredTingaud

Copy link
Copy Markdown
Contributor Author

The url I put in the commit message compares the previous and the new implementations (and it uses google benchmark).
Do you prefer a local comparison?

@dmah42

dmah42 commented Apr 9, 2018

Copy link
Copy Markdown
Member

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.

@FredTingaud

Copy link
Copy Markdown
Contributor Author

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?

@dmah42

dmah42 commented Apr 9, 2018

Copy link
Copy Markdown
Member

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.

@dmah42
dmah42 merged commit 50ffc78 into google:master Apr 9, 2018
JBakamovic pushed a commit to JBakamovic/benchmark that referenced this pull request Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants