Optimization of tdigest merge aggregation. #16780
Merged
Conversation
This file contains 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
nvdbaranec
added
libcudf
revans2
reviewed
Sep 10, 2024
There was a problem hiding this comment.
I ran a simple performance test against the CPU. An a6000 vs 16 CPU cores to do an approximate percentile on 1,000,000,000 rows with 1,000,000 unique keys in the group by. The GPU was 48x faster than the CPU. So it looks good. I still get errors related to #16675 not being in, and I am not sure if there are any merge conflicts that would show up between the two. I have not looked deeply at the C++ code so I am not approving this, but from the results I am +1 on merging it in.
|
I did some more testing against the CPU and it looks really good. The improvements range from 5x faster to 32x faster than 16 CPU cores. A lot of the slowness on the CPU comes from spilling/shuffle when there are lots of groups, which we don't appear to suffer from as badly. |
davidwendt
reviewed
Sep 17, 2024
ttnghia
reviewed
Sep 17, 2024
ttnghia
reviewed
Sep 17, 2024
ttnghia
reviewed
Sep 17, 2024
mhaseeb123
reviewed
Sep 17, 2024
|
Looks like we also need a style fix for CI to run. |
hyperbolic2346
requested changes
Sep 18, 2024
…individual tdigests in the tiny and small group benchmarks.
…df::get_current_device_resource_ref() where appropriate.
mhaseeb123
reviewed
Sep 19, 2024
davidwendt
reviewed
Sep 20, 2024
Co-authored-by: David Wendt <[email protected]>
mhaseeb123
approved these changes
Sep 20, 2024
ttnghia
approved these changes
Sep 21, 2024
|
If there are just a few CI pipelines broken, don't rerun everything. Instead, click on the "Details" link then rerun only the failed jobs. |
That didn't work in this case. |
hyperbolic2346
approved these changes
Sep 25, 2024
|
Yeah, it seems the CI workers are not available. Maybe you need to contact devops. |
|
/merge |
rjzamora
pushed a commit
to rjzamora/cudf
that referenced
this pull request
Sep 25, 2024
Fixes rapidsai#16625 This PR fixes a slow implementation of the centroid merging step during the tdigest merge aggregation. Previously it was doing a linear march over the individual tdigests per group and merging them one by one. This led to terrible performance for large numbers of groups. In principle though, all this really was doing was a segmented sort of centroid values. So that's what this PR changes it to. Speedup for 1,000,000 input tidests with 1,000,000 individual groups is ~1000x, ``` Old --------------------------------------------------------------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------------------------------------------------------------- TDigest/many_tiny_groups/1000000/1/1/10000/iterations:8/manual_time 7473 ms 7472 ms 8 TDigest/many_tiny_groups2/1000000/1/1/1000/iterations:8/manual_time 7433 ms 7431 ms 8 ``` ``` New --------------------------------------------------------------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------------------------------------------------------------- TDigest/many_tiny_groups/1000000/1/1/10000/iterations:8/manual_time 6.72 ms 6.79 ms 8 TDigest/many_tiny_groups2/1000000/1/1/1000/iterations:8/manual_time 1.24 ms 1.32 ms 8 ``` Authors: - https://github.com/nvdbaranec - Muhammad Haseeb (https://github.com/mhaseeb123) - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Muhammad Haseeb (https://github.com/mhaseeb123) - Nghia Truong (https://github.com/ttnghia) - Mike Wilson (https://github.com/hyperbolic2346) URL: rapidsai#16780
Matt711
pushed a commit
to mroeschke/cudf
that referenced
this pull request
Sep 25, 2024
Fixes rapidsai#16625 This PR fixes a slow implementation of the centroid merging step during the tdigest merge aggregation. Previously it was doing a linear march over the individual tdigests per group and merging them one by one. This led to terrible performance for large numbers of groups. In principle though, all this really was doing was a segmented sort of centroid values. So that's what this PR changes it to. Speedup for 1,000,000 input tidests with 1,000,000 individual groups is ~1000x, ``` Old --------------------------------------------------------------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------------------------------------------------------------- TDigest/many_tiny_groups/1000000/1/1/10000/iterations:8/manual_time 7473 ms 7472 ms 8 TDigest/many_tiny_groups2/1000000/1/1/1000/iterations:8/manual_time 7433 ms 7431 ms 8 ``` ``` New --------------------------------------------------------------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------------------------------------------------------------- TDigest/many_tiny_groups/1000000/1/1/10000/iterations:8/manual_time 6.72 ms 6.79 ms 8 TDigest/many_tiny_groups2/1000000/1/1/1000/iterations:8/manual_time 1.24 ms 1.32 ms 8 ``` Authors: - https://github.com/nvdbaranec - Muhammad Haseeb (https://github.com/mhaseeb123) - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Muhammad Haseeb (https://github.com/mhaseeb123) - Nghia Truong (https://github.com/ttnghia) - Mike Wilson (https://github.com/hyperbolic2346) URL: rapidsai#16780
3 tasks
3 tasks
rapids-bot bot
pushed a commit
that referenced
this pull request
Oct 24, 2024
Removes unused variable that contains host copy of the group_offsets data. This host variable appears to have been made obsolete by a combination of #16897 and #16780 Found while working on #17149 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Muhammad Haseeb (https://github.com/mhaseeb123) - Nghia Truong (https://github.com/ttnghia) URL: #17151
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.
Fixes #16625
This PR fixes a slow implementation of the centroid merging step during the tdigest merge aggregation. Previously it was doing a linear march over the individual tdigests per group and merging them one by one. This led to terrible performance for large numbers of groups. In principle though, all this really was doing was a segmented sort of centroid values. So that's what this PR changes it to. Speedup for 1,000,000 input tidests with 1,000,000 individual groups is ~1000x,
Checklist