Make stream and memory resource explicit in hyperloglog APIs#9720
Make stream and memory resource explicit in hyperloglog APIs#9720PointKernel wants to merge 6 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test 84e48f4 |
This comment has been minimized.
This comment has been minimized.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughHyperLogLog host and reference APIs now require explicit ChangesHyperLogLog Stream API Rework
Suggested reviewers: Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cudax/include/cuda/experimental/__cuco/hyperloglog.cuh (1)
154-169: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winimportant: constructor still returns before the initial clear completes The three constructors enqueue
clear_async(__stream)and return without synchronizing, so the@noteclaiming stream sync is stale. Either callclear(__stream)here or update the note to say construction is asynchronous and the caller must synchronize before using the object.
🧹 Nitpick comments (3)
cudax/test/cuco/hyperloglog/test_hyperloglog.cu (1)
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: the
::cuda::stream{device_ref{0}}+device_default_memory_pool(device_ref{0})setup is repeated identically across all five tests. Could extract into a shared fixture/helper to reduce duplication, but not blocking.Also applies to: 122-146, 189-199, 224-230, 251-258
cudax/include/cuda/experimental/__cuco/hyperloglog_ref.cuh (1)
231-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Stray
// Review:note sits inside the//!doxygen block and will bleed into the generated docs / is a leftover artifact. Drop it (or convert to a tracked TODO).- //! `@throw` If sketch_bytes() != __other.sketch_bytes() - //! - // Review: For host-side merges, consider validating `__other` is on the same device. - //! `@tparam` _OtherScope Thread scope of `other` estimator + //! `@throw` If sketch_bytes() != __other.sketch_bytes() + //! + //! `@tparam` _OtherScope Thread scope of `other` estimatorcudax/include/cuda/experimental/__cuco/hyperloglog.cuh (1)
159-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: add
#include <cuda/__utility/no_init.h>here;::cuda::no_initis used directly and shouldn’t depend ondevice_memory_pool.hfor availability.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 87f29846-e10b-4203-be12-9e09a637769c
📒 Files selected for processing (7)
cudax/include/cuda/experimental/__cuco/detail/hyperloglog/default_policy.cuhcudax/include/cuda/experimental/__cuco/detail/hyperloglog/finalizer.cuhcudax/include/cuda/experimental/__cuco/detail/hyperloglog/hyperloglog_impl.cuhcudax/include/cuda/experimental/__cuco/detail/hyperloglog/kernels.cuhcudax/include/cuda/experimental/__cuco/hyperloglog.cuhcudax/include/cuda/experimental/__cuco/hyperloglog_ref.cuhcudax/test/cuco/hyperloglog/test_hyperloglog.cu
This comment has been minimized.
This comment has been minimized.
😬 CI Workflow Results🟥 Finished in 1h 09m: Pass: 98%/55 | Total: 5h 20m | Max: 1h 09m | Hits: 93%/38334See results here. |
Description
This PR removes the default CUDA stream and default memory resource from
cuda::experimental::cuco::hyperloglogandhyperloglog_ref, following thecuda::bufferdesign adopted forfixed_capacity_mapin #9719:hyperloglogconstructors now takecuda::stream_refas the first argument and the memory resource as the second, matchingcuda::buffer's argument order. The constructors that silently picked the default memory pool of device 0 are removed.clear,clear_async,add,add_async,merge,merge_async,estimate) on bothhyperloglogandhyperloglog_reftake the stream as the first parameter, with no default.It also applies the conventions established during the #7705 review to the hyperloglog headers:
_CCCL_HOST_API/_CCCL_HOST_DEVICE_APIannotations on allhyperloglogmember functions, which were previously unannotatedconstexpr._CCCL_TEMPLATE/_CCCL_REQUIRESinstead of rawenable_ifSFINAE for the constrainedclear(_CG)andmerge(_CG, ...)device overloads.#if !_CCCL_COMPILER(NVRTC)guard around the host-onlyhyperloglogclass and#if _CCCL_CUDA_COMPILATION()around the device kernels.<cuda/experimental/container.cuh>,<cuda/experimental/memory_resource.cuh>, and<cuda/experimental/stream.cuh>headers.this->prefixes for member access.Checklist