[FEA] Refactor to eliminate redundant device aggregation logic
- Dominant language
- C++
- Stars
- 9.8k
- Forks
- 1.1k
- Avg merge
- 3d 6m
- Merged PRs (30d)
- 278
Description
**Is your feature request related to a problem? Please describe.**
Once #17031 is merged, three copies of similar device aggregator logic will exist in libcudf, and we need to address this issue
1. https://github.com/rapidsai/cudf/blob/branch-24.12/cpp/src/groupby/hash/shared_memory_aggregator.cuh
2. https://github.com/rapidsai/cudf/blob/branch-24.12/cpp/src/groupby/hash/global_memory_aggregator.cuh
3. https://github.com/rapidsai/cudf/blob/branch-24.12/cpp/include/cudf/detail/aggregation/device_aggregators.cuh
We currently cannot share the same code path because the existing device aggregator only accepts column_device_view as input, and libcudf does not yet support constructing a column_device_view from shared memory.
Proposed Solution The initial plan was to extend column_device_view to allow its construction from shared memory. The ultimate goal is to create a unified aggregator that handles all types of aggregations: global-global, shared-global, and global-shared. However, after further discussions, it appears that unifying all three into a single aggregator may not be feasible. Nonetheless, there are several potential improvements we want to explore:
- [ ] Replace the `bool` array used for shared memory nullability with a `bitmask_type` array. While preliminary tests show this can cause a 10% slowdown due to the atomic operations required by bitmasks, there's potential for optimization. The key benefit is that bitmasks save memory, allowing for more complex requests to be performed in shared memory.
- [ ] Once we transition to using row bitmasks, it should be possible for all three aggregators to share a common source for underlying kernels.
- [ ] Adding dictionary template instantiations results in a performance degradation of up to 5x. Profiling is needed to determine the cause. Additionally, on V100 GPUs, dictionary instantiations trigger a `cudaErrorInvalidValue` when querying available dynamic shared memory size using `cudaOccupancyAvailableDynamicSMemPerBlock`. This error seems related to the dictionary template instantiation in the aggregator, which causes a nested invocation of the type dispatcher. Notably, this error occurs on V100 but not on RTX8000.
Contributor guide
Assessment
This issue has not been assessed yet.