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

[REVIEW] Add CMake option for per-thread default stream #4995

Merged
merged 10 commits into from
May 6, 2020

Conversation

rongou
Copy link
Contributor

@rongou rongou commented Apr 22, 2020

This change adds the option to build cuDF with --default-stream per-thread enabled.

For code not built with nvcc, we pass -DCUDA_API_PER_THREAD_DEFAULT_STREAM to gcc.

This is the alternative solution to rapidsai/rmm#352.

By default the option is disabled. To enable it:

cmake .. -DPER_THREAD_DEFAULT_STREAM=ON ...
mvn clean install -DPER_THREAD_DEFAULT_STREAM=ON

I've tested this manually on some spark jobs. For TPCH Q4, there is about a 25% speedup in my setup (YMMV).

The corresponding change in RMM is rapidsai/rmm#354.

@harrism @jrhemstad @revans2 @jlowe

@rongou rongou requested review from a team as code owners April 22, 2020 23:42
@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@revans2
Copy link
Contributor

revans2 commented Apr 23, 2020

The code looks fine to enable/disable the feature. I'm mostly concerned about synchronization with cnmem.

@rongou
Copy link
Contributor Author

rongou commented Apr 23, 2020

CNMeM does extra synchronization in PTDS mode: https://github.com/NVIDIA/cnmem/blob/master/src/cnmem.cpp#L386

So far in my testing I haven't found any problems. Once this is merged we can probably switch it on in our CI and test more thoroughly.

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@harrism
Copy link
Member

harrism commented Apr 23, 2020

Add to whitelist

@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@rongou rongou changed the title add option for per-thread default stream [REVIEW] Add CMake option for per-thread default stream Apr 24, 2020
@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #4995 into branch-0.14 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.14    #4995   +/-   ##
============================================
  Coverage        88.44%   88.44%           
============================================
  Files               54       54           
  Lines            10267    10267           
============================================
  Hits              9081     9081           
  Misses            1186     1186           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b604247...0a7f79c. Read the comment docs.

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge CMake CMake build issue labels May 1, 2020
@kkraus14 kkraus14 added 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Java) Reviewer and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels May 1, 2020
@harrism
Copy link
Member

harrism commented May 5, 2020

@rongou need a Java codeowners review...

Copy link
Contributor

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in reviewing. The changes here look OK, but they are disconnected from the cpp build. I don't think we ever want to build the cpp with default stream and the JNI code without it or vice-versa. If there's no good way to ensure the Java CMake settings are linked to the cpp build automatically then minimally we need to update java/README.md with a section on per-thread default stream. If PER_THREAD_DEFAULT_STREAM was specified during the native build then it also needs to be specified for the Java build (and show how to do so).

@rongou
Copy link
Contributor Author

rongou commented May 5, 2020

@jlowe I couldn't figure out a way to link cmake options, so added a section to the readme. Hopefully this is only temporary, we may want to make PTDS the default.

@jlowe jlowe added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Java) Reviewer labels May 5, 2020
@kkraus14 kkraus14 merged commit f610f14 into rapidsai:branch-0.14 May 6, 2020
@rongou rongou deleted the per-thread-default-stream branch June 6, 2020 00:55
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 CMake CMake build issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants