Repository navigation
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR removes a Changesthrust::tabulate CUDA implementation optimization
Assessment against linked issues
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
thrust/thrust/system/cuda/detail/tabulate.h (1)
35-35: ⚡ Quick winsuggestion: The unused include at line 22 (
<cuda/__functional/address_stability.h>) can be removed—transform.halready includes it. Tests cover standard functor cases (identity, placeholder expressions), but don't explicitly exercise capturing lambdas or stateful functors; if such cases are thrust::tabulate use cases, consider adding tests to verifytransform_n's dispatch handles them correctly without the removed wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 23b98d62-9862-4aaf-90d8-1cfb40a29aca
📒 Files selected for processing (1)
thrust/thrust/system/cuda/detail/tabulate.h
bernhardmgruber
left a comment
There was a problem hiding this comment.
I am sorry proclaim_copyable_arguments is causing a regression on your side. However, the proposed change causes a >5x regression for other workloads, see #6012 which shows benchmarks on the initial improvements.
|
Thanks for looking into it with me. I'll report some additional benchmarks on #9070, closing this for now. |
🥳 CI Workflow Results🟩 Finished in 2h 21m: Pass: 100%/117 | Total: 3d 15h | Max: 2h 03m | Hits: 42%/549083See results here. |
Replaces `thrust::tabulate` with `cub::DeviceFor::Bulk` in the row-hashing implementations: - MurmurHash3 x86 32-bit - xxHash 32-bit - xxHash 64-bit This avoids a known slow path in CCCL's CUDA `thrust::tabulate` implementation for row-hashing-style functors. See benchmarks below. Also see NVIDIA/cccl#9070 and NVIDIA/cccl#9071. The hashing logic and row-hasher preprocessing are unchanged; only the launch mechanism is changed. This also adds a `num_cols` axis to `HASHING_NVBENCH` so the benchmark can cover wider tables. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - David Wendt (https://github.com/davidwendt) - Muhammad Haseeb (https://github.com/mhaseeb123) - Yunsong Wang (https://github.com/PointKernel) URL: #22632
Summary
cuda::proclaim_copyable_argumentsfrom the CUDAthrust::tabulatebackend.tabulateimplemented throughcuda_cub::transform_n, but lets transform dispatch choose the stable-address path for user functors.Details
In #6012, CUDA
tabulatewas ported from a customparallel_forimplementation totransform_nand the functor was wrapped withcuda::proclaim_copyable_arguments.A standalone CCCL + NVBench reproducer in #9070 showed that the wrapper is the source of the slowdown. Applying
proclaim_copyable_argumentsmanually to an otherwise fasttransform_nreproduces the slowtabulatebehavior.Representative results on NVIDIA GB10, CUDA 13.2, CCCL main
fcd4ff4ea1:With this patch:
Fixes #9070.