Support empty segments in segmented bitmask reductions - #23689
Conversation
7b0eb55 to
76d938b
Compare
vuule
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe segmented bitmask API now accepts identity values, supports empty segments, and validates offsets. Bitwise-AND uses an all-bits-set identity. Tests cover empty segments and invalid offsets. The benchmark now generates non-negative segment sizes. Segmented bitmask operations
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change defines correct results for empty segments and prevents out-of-bounds indexing; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ity seed Seeding each segment's reduction with the identity of the binary operator, instead of loading the segment's first mask, gives an empty segment a well defined result (an all-valid destination mask, null count zero) and removes the read of sources[segment_start] that a segment with no masks would otherwise perform out of bounds. Also validate that segment offsets are non-decreasing and within the mask array, and stop the bitmask benchmark's segment size generator from drawing negative sizes, which produced non-monotonic offsets and an illegal memory access for a small expected_masks_per_segment.
76d938b to
9b5cc7a
Compare
…ty-segments # Conflicts: # cpp/tests/bitmask/bitmask_tests.cpp
| auto const segment_id = cudf::detail::grid_1d::global_thread_id() / warp.size(); | ||
| auto const segment_id = cudf::detail::grid_1d::global_thread_id() / warp.size(); | ||
|
|
||
| if (segment_id >= num_segments) { return; } |
There was a problem hiding this comment.
exit before reading from segment_offsets
|
/ok to test 400bf84 |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| CUDF_EXPECTS(segment_offsets.size() >= 2, | ||
| "At least one segment needs to be passed for bitwise operations"); | ||
| CUDF_EXPECTS(std::is_sorted(segment_offsets.begin(), segment_offsets.end()), | ||
| "Segment offsets must be non-decreasing"); |
There was a problem hiding this comment.
This seems expensive and not a check we normally do (i.e. other segmented APIs).
Could we just check in the kernel and just return if segment_start > segment_end ?
There was a problem hiding this comment.
This is a linear check, same as the two all_ofs above. Turns out it's not expensive, even compared to those:
| segments | masks | API (µs) | is_sorted (µs) |
both all_of (µs) |
is_sorted share (%) |
|---|---|---|---|---|---|
| 100 | 420 | 227.5 | 0.054 | 0.30 | 0.024 |
| 1,000 | 3,993 | 1,883.8 | 0.448 | 2.71 | 0.024 |
| 10,000 | 39,862 | 18,560 | 4.38 | 27.0 | 0.024 |
| 100,000 | 400,186 | 188,860 | 43.9 | 273.9 | 0.023 |
In addition, AFAICT, checking in the kernel would require some mechanism to return this information to the host, which is likely to be more expensive than is_sorted.
Match the convention of the neighboring host-side index checks in this header (validate_segmented_indices) and of slice/contiguous_split: a decreasing pair of offsets throws std::invalid_argument and an offset outside the mask array throws std::out_of_range, instead of a bare cudf::logic_error.
|
/ok to test 50ebdd5 |
3 participants
Description
segmented_offset_bitmask_binopseeded each segment's reduction by loading the segment's first mask,sources[segment_start], before knowing whether the segment contains any masks. An empty segment therefore had no defined result and read a mask it does not own; for a trailing empty segment, that read is past the end of the mask array.With this PR, seeding is done with the identity of the binary operator. An empty segment then means the identity, which for bitwise AND is an all-valid mask with a null count of zero; this is now documented on the public
segmented_bitmask_andoverloads.The kernel also indexed
segment_offsetsanddestinationsbefore thesegment_id >= num_segmentsguard, which the excess warps of the last block do whenever the segment count is not a multiple of the warps per block; those loads now happen after the guard.Segment offsets are additionally validated as non-decreasing and within the mask array, since a decreasing pair leaves a segment's bounds meaningless. The bitmask benchmark's segment size generator was producing exactly that: it truncated draws from an unbounded normal distribution into
size_type, so a smallexpected_masks_per_segmentgave negative sizes and an illegal memory access. It now clamps at zero, keeping empty segments.Checklist