Add device aggregators used by shared memory groupby #17031
Conversation
PointKernel
added
feature request
| using DeviceTarget = cudf::detail::underlying_target_t<Source, aggregation::MIN>; | ||
| using DeviceSource = cudf::detail::underlying_source_t<Source, aggregation::MIN>; | ||
|
|
||
| DeviceTarget* target_casted = reinterpret_cast<DeviceTarget*>(target); |
There was a problem hiding this comment.
How will this work for strings columns?
The target type will be a string_view and this cast will be incorrect.
There was a problem hiding this comment.
Good question. I wanted to give you a thorough response, so I took a deeper dive into the details to see how this works. The more I look into the current code, the more surprised I am that it's producing the correct results.
Still working on it. Need more time to fully understand this.
There was a problem hiding this comment.
OK, finally got this. String columns don't have the atomic support so they use the sort-based groupby all the time.
There was a problem hiding this comment.
This then this gets into a more philosophical discussion about column-device-view and shared memory. Technically atomics only work with simple fixed-width types (not including timestamp, duration too) and therefore you would not need something as fancy as wrapping a column-device-view for these types since they will always be representable by pointer, size (device-span) and a validity-mask. You can simply cast the std::byte* to the underlying type without really needing a column-device-view (IMO).
There was a problem hiding this comment.
simply cast the std::byte* to the underlying type without really needing a column-device-view
Totally makes sense.
After removing dictionaries from the shared memory groupby, it now only handles integers and decimals. A pointer to the underlying type is sufficient for this. Let me unify the shared and global memory aggregators first. I’m really happy it has come down to such a simple solution. Thanks a lot for your input! @davidwendt
There was a problem hiding this comment.
I've been working on unifying the aggregators locally, and this requires non-trivial effort around performance improvement and API design.
I started with updating null masks to use bitmask_type* instead of bool*, but I noticed around a 10% slowdown because bitmask_type always requires atomic operations to use. After that, there are some differences in behavior between the shared memory aggregator, global memory aggregator, and the existing row aggregator, particularly in how they handle nulls and supported data types. I would suggest that we merge the current PR as is and track the follow-up work through #17032.
There was a problem hiding this comment.
Looks good to me. Discussion with @davidwendt clarified a few of my confusions as well. I am interested to see these aggregators unified and in action so subscribing to PR #17032 as well.
| cudf::column_device_view source, | ||
| cudf::size_type source_index) const noexcept | ||
| { | ||
| if constexpr (k != cudf::aggregation::COUNT_ALL) { |
There was a problem hiding this comment.
Non blocking nit: Maybe we can add the same comment as above here and in other aggregators as well.
Description
This work is part of splitting the original bulk shared memory groupby PR #16619.
It introduces two device-side element aggregators:
shmem_element_aggregator: aggregates data from global memory sources to shared memory targets,gmem_element_aggregator: aggregates from shared memory sources to global memory targets.These two aggregators are similar to the
elementwise_aggregatorfunctionality. Follow-up work is tracked via #17032.Checklist