Tune thread count for binning/grouping from scratch #3641
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After #3637 reduced some baseline cost, increasing thread counts earlier pays off according to my local benchmarks (group two columns into 2M bins).
nvaytet
reviewed
Jan 28, 2025
| const auto size = std::max(scipp::index(1), da.dims()[dim]); | ||
| const auto nthread = size > 10000000 ? 24 | ||
| : size > 1000000 ? 4 | ||
| const auto nthread = size > 8000000 ? 24 |
Member
There was a problem hiding this comment.
Should these numbers really be hard-coded or should they depend on how many threads are available on the machine?
It could be that the benchmarks don't look as good on a laptop or a visa instance with less than 24 cores.
Member
Author
There was a problem hiding this comment.
It will not actually make that many threads. Instead, it cuts the input into chunks, which will be processed independently. Scipp's transform automatically uses multi-threading across these, so if you have fewer threads each thread will process multiple chunks. If chunks are too small there is extra cost to this, but I hope at this level it is not relevant.
Regarding thread counts in general, also see #3565 which would probably lead to more gains.
nvaytet
approved these changes
Jan 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
After #3638 reduced some baseline cost, increasing thread counts earlier pays off according to my local benchmarks (group two columns into 2M bins).
Results (blue: before #3638, orange: #3638, green: this)
I included
numpy.bincountfor comparison. It does something much simpler, but it nicely illustrates that our constant (in the number of rows) baseline cost is still quite substantial, significantly affecting timings up to 10M rows or so. I will have another look at this bit as a follow-up task.