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

Reduce shared memory usage in gpuComputePageSizes by 50% #13047

Merged

Conversation

nvdbaranec
Copy link
Contributor

@nvdbaranec nvdbaranec commented Mar 31, 2023

In a multithreaded, multi-stream environment (Spark) we were experiencing a performance regression on some benchmark queries. The culprit was gpu scheduling issues related to the gpuComputePageSizes kernel. Dependent kernels (gpuDecodePages) were getting serialized because gpuComputePageSizes wasn't running alongside other streams well.

The fix was reducing shared memory usage in gpuComputePageSizes. The kernel shares a lot of code and data structures with gpuDecodePages but doesn't actually use several of the large buffers that are stored in shared memory. This PR refactors those buffers out so that they are only declared in the gpuDecodePages kernel, reducing the shared usage by 50% (3kb).

This clears up the performance issue on Spark. I am currently experiencing build issues with cudf benchmarks so I'm marking this as do-not-merge until I can verify with them.

Checklist

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

@nvdbaranec nvdbaranec added libcudf Affects libcudf (C++/CUDA) code. cuIO cuIO issue Spark Functionality that helps Spark RAPIDS 5 - DO NOT MERGE Hold off on merging; see PR for details improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 31, 2023
@nvdbaranec nvdbaranec requested a review from a team as a code owner March 31, 2023 19:33
Copy link
Contributor

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

Nit: Recommend the following:

  • Polish comments more if possible, and
  • Run compute sanitizer to check if there is any issue.

Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Looks good.

@nvdbaranec
Copy link
Contributor Author

Nit: Recommend the following:

* Polish comments more if possible, and

Right. Forgot to update all the function comments. Will fix.

@nvdbaranec nvdbaranec removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Apr 6, 2023
@nvdbaranec
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit c4a34eb into rapidsai:branch-23.06 Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuIO cuIO issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants