[cudax][cuco] Port fixed_capacity_map benchmarks from cuCollections#9748
[cudax][cuco] Port fixed_capacity_map benchmarks from cuCollections#9748sleeepyjack wants to merge 5 commits into
Conversation
| //! | ||
| //! @param __multiplicity Average number of generated keys that map to each unique key value. | ||
| //! @throws std::invalid_argument if `__multiplicity` is not positive. | ||
| explicit uniform(::nvbench::int64_t __multiplicity) |
There was a problem hiding this comment.
Nit: testing/benchmarks or any non-library code shouldn't be __ugly
|
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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds CUCO benchmark defaults and key generation utilities, then adds nvbench benchmarks for Changesfixed_capacity_map benchmark suite
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers: Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cudax/benchmarks/bench/cuco/common/key_generator.cuh (1)
207-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion:
dropout()unconditionally shuffles[begin, end)at line 247, even whenkeep_prob >= 1.0(no keys actually dropped). This side effect isn't mentioned in the docstring (lines 212-223), which only describes the drop/replace behavior. Worth documenting the shuffle's purpose (or gating it behind thekeep_prob < 1.0branch if it's only meant to intersperse dropped keys) so callers relying on this utility understand the full behavior.As per path instructions for
cudax/**/*, review should focus on "experimental API clarity" among other things.Doc clarification example
//! Drops keys with probability `1 - keep_prob` using the provided execution policy. //! //! Replaced keys are sampled from `[N, max_key]`, where `N` is the number of keys in the range. + //! The full range is always shuffled afterward, regardless of `keep_prob`. //!Source: Path instructions
cudax/benchmarks/bench/cuco/fixed_capacity_map/insert.cu (1)
46-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: the device/stream/memory-resource setup, key→pair transform, and map construction (lines 46-74) are duplicated almost verbatim in
contains.cu(lines 46-75). Sincecommon/already houses shared benchmark utilities, consider extracting a shared helper (e.g. abuild_map(state, keys)or similar incommon/) to avoid drift between the two benchmarks as the map API evolves.cudax/benchmarks/bench/cuco/common/defaults.cuh (1)
28-39: 🚀 Performance & Scalability | 🔵 Trivialsuggestion: trim the low-occupancy sweep The 10% point drives
fixed_capacity_map_insert_unique_occupancyandfixed_capacity_map_contains_unique_occupancyto ~1e9 slots forint64_t/int64_tinstances (~16 GiB of slots alone, before overhead), and that multiplies across 9 occupancy values and 2 valid type combinations. Consider raising the minimum occupancy or loweringnso the sweep stays practical on smaller GPUs.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d0bd7519-6228-406b-9cb5-c72d712b117d
📒 Files selected for processing (5)
cudax/benchmarks/bench/cuco/common/defaults.cuhcudax/benchmarks/bench/cuco/common/key_generator.cuhcudax/benchmarks/bench/cuco/fixed_capacity_map/contains.cucudax/benchmarks/bench/cuco/fixed_capacity_map/insert.cucudax/include/cuda/experimental/__cuco/fixed_capacity_map.cuh
This comment has been minimized.
This comment has been minimized.
|
@sleeepyjack can you please share the benchmark results comparing |
😬 CI Workflow Results🟥 Finished in 1h 08m: Pass: 98%/55 | Total: 5h 08m | Max: 1h 08m | Hits: 98%/36787See results here. |
Closes #9595.
Summary
Ports the relevant cuCollections
static_mapNVBench benchmarks to cudaxfixed_capacity_map.Adjusts the default cooperative-group size for
fixed_capacity_mapto align with the optimal setup used by cuCollections.Changes
cudax/benchmarks/bench/cuco/common/.static_mapinsert benchmarks asfixed_capacity_mapbenchmarks:fixed_capacity_map_insert_unique_capacityfixed_capacity_map_insert_unique_occupancyfixed_capacity_map_insert_uniform_multiplicitystatic_mapcontains benchmarks asfixed_capacity_mapbenchmarks:fixed_capacity_map_contains_unique_capacityfixed_capacity_map_contains_unique_occupancyfixed_capacity_map_contains_unique_matching_rate