Make stream and memory resource explicit in fixed_capacity_map APIs#9719
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthroughfixed_capacity_map and its backend now require explicit stream and memory-resource arguments, with stream-aware methods reordered to take the stream first. Tests were updated to construct explicit streams and device-default memory pools and pass them through map construction and operations. ChangesStream parameter reordering
Assessment against linked issues
Out-of-scope changesNone found. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cudax/test/cuco/fixed_capacity_map/test_shared_memory.cu (1)
84-89: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuesuggestion: prefer
stream.sync()overcudaDeviceSynchronize()now that an explicit stream is used for the kernel launch — keeps synchronization scoped to the stream this test actually cares about.Proposed fix
insert_shmem_kernel<<<grid_size, block_size, 0, stream.get()>>>( map.ref(), cuda::transform_iterator(cuda::counting_iterator<int>{0}, iota_pair<fixed_capacity_map_512_type::value_type>{}), num_keys); - REQUIRE(cudaDeviceSynchronize() == cudaSuccess); + stream.sync();Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 16036a28-a5bc-42e5-bad4-e3eebd92a1a1
📒 Files selected for processing (6)
cudax/include/cuda/experimental/__cuco/detail/open_addressing/open_addressing_impl.cuhcudax/include/cuda/experimental/__cuco/fixed_capacity_map.cuhcudax/test/cuco/fixed_capacity_map/test_capacity.cucudax/test/cuco/fixed_capacity_map/test_insert_and_contains.cucudax/test/cuco/fixed_capacity_map/test_key_sentinel.cucudax/test/cuco/fixed_capacity_map/test_shared_memory.cu
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 1h 08m: Pass: 100%/55 | Total: 5h 06m | Max: 1h 08m | Hits: 91%/39400See results here. |
Description
closes #9695
This PR removes the default CUDA stream and default memory resource from
cuda::experimental::cuco::fixed_capacity_map, following thecuda::bufferdesign agreed on during the #7705 review discussion:cuda::stream_refas the first argument and the memory resource as the second, matchingcuda::buffer's argument order. The helper that silently picked the current device's default memory pool is removed.clear,clear_async,insert,insert_async,contains,contains_async) take the stream as the first parameter, with no default.__open_addressing_implfollows the same stream-first, memory-resource-second order.Checklist