Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor dictionary encoding in PQ writer to migrate to the new cuco::static_map #16541

Merged
merged 39 commits into from
Aug 30, 2024

Conversation

mhaseeb123
Copy link
Member

@mhaseeb123 mhaseeb123 commented Aug 13, 2024

Description

Part of #12261. This PR refactors the dictionary encoding in Parquet writers to migrate from cuco::legacy::static_map to cuco::static_map to build the dictionaries.

Performance Results

The changes result in +0.08% average speed improvement and +16.22% average memory footprint increase (stems from the adjusted sizes by cuco::make_window_extent due to prime gap) across the benchmark cases extended from #16591

Currently, we do see a roughly 8% speed improvement in map insert and find kernels which is counteracted by the map init and map collect kernels as they have to process 16.22% more slots. With a cuco bump, the average speed improvement will increase from 0.08% to +3% and the memory footprint change will go back from 16.22% to +0%.

Hardware used for benchmarking

 `NVIDIA RTX 5880 Ada Generation`
* SM Version: 890 (PTX Version: 860)
* Number of SMs: 110
* SM Default Clock Rate: 18446744071874 MHz
* Global Memory: 23879 MiB Free / 48632 MiB Total
* Global Memory Bus Peak: 960 GB/sec (384-bit DDR @10001MHz)
* Max Shared Memory: 100 KiB/SM, 48 KiB/Block
* L2 Cache Size: 98304 KiB
* Maximum Active Blocks: 24/SM
* Maximum Active Threads: 1536/SM, 1024/Block
* Available Registers: 65536/SM, 65536/Block
* ECC Enabled: No

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@mhaseeb123 mhaseeb123 self-assigned this Aug 13, 2024
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 13, 2024
@mhaseeb123 mhaseeb123 changed the title Refactor PQ writer's dictionary encoder to migrate from cuco::legacy::static_map to cuco::static_map [WIP]: Refactor dictionary encoding in PQ writer to migrate to the new cuco::static_map Aug 13, 2024
cpp/src/io/parquet/writer_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/parquet/chunk_dict.cu Outdated Show resolved Hide resolved
cpp/src/io/parquet/parquet_gpu.cuh Outdated Show resolved Hide resolved
cpp/src/io/parquet/parquet_gpu.cuh Outdated Show resolved Hide resolved
Copy link

copy-pr-bot bot commented Aug 13, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@mhaseeb123 mhaseeb123 added 2 - In Progress Currently a work in progress cuIO cuIO issue improvement Improvement / enhancement to an existing function breaking Breaking change labels Aug 13, 2024
@mhaseeb123 mhaseeb123 requested a review from PointKernel August 19, 2024 21:10
@mhaseeb123 mhaseeb123 changed the title [WIP]: Refactor dictionary encoding in PQ writer to migrate to the new cuco::static_map Refactor dictionary encoding in PQ writer to migrate to the new cuco::static_map Aug 19, 2024
@mhaseeb123
Copy link
Member Author

/ok to test

@PointKernel
Copy link
Member

Redundant capacity issue is tracked via NVIDIA/cuCollections#581

cpp/src/io/parquet/chunk_dict.cu Outdated Show resolved Hide resolved
cpp/src/io/parquet/chunk_dict.cu Show resolved Hide resolved
cpp/src/io/parquet/chunk_dict.cu Outdated Show resolved Hide resolved
cpp/src/io/parquet/chunk_dict.cu Outdated Show resolved Hide resolved
cpp/src/io/parquet/chunk_dict.cu Outdated Show resolved Hide resolved
@mhaseeb123 mhaseeb123 requested a review from PointKernel August 21, 2024 23:30
@mhaseeb123
Copy link
Member Author

/ok to test

Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small question otherwise looks good to me.

Great work!

cpp/src/io/parquet/chunk_dict.cu Outdated Show resolved Hide resolved
@mhaseeb123 mhaseeb123 added 4 - Needs Review Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels Aug 28, 2024
@mhaseeb123
Copy link
Member Author

/ok to test

@mhaseeb123
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 8f2d687 into rapidsai:branch-24.10 Aug 30, 2024
85 checks passed
@mhaseeb123 mhaseeb123 deleted the fea-refactor-dict-pq branch August 30, 2024 16:41
@mhaseeb123 mhaseeb123 added cuco cuCollections related issue 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs Review Waiting for reviewer to review or respond labels Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change cuco cuCollections related issue cuIO cuIO issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants