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

Set CUDA_USE_STATIC_CUDA_RUNTIME=OFF for java builds [skip-ci] #7884

Closed

Conversation

abellina
Copy link
Contributor

@abellina abellina commented Apr 7, 2021

This issue is to workaround an extra cuInit and slow cudaHostAlloc we see in cudf-0.19. The issue is that arrow is statically linked, and it is also statically linking against cudart.

This work around fixes it for the java side.

Re: #7600

@abellina abellina requested a review from a team as a code owner April 7, 2021 14:53
@github-actions github-actions bot added the Java Affects Java cuDF API. label Apr 7, 2021
@abellina
Copy link
Contributor Author

abellina commented Apr 7, 2021

@jrhemstad @kkraus14 @robertmaynard, adding this for the java side for now.

@abellina abellina added 3 - Ready for Review Ready for review by team bug Something isn't working Performance Performance related issue non-breaking Non-breaking change labels Apr 7, 2021
@abellina abellina requested a review from jlowe April 7, 2021 14:56
@abellina
Copy link
Contributor Author

abellina commented Apr 7, 2021

Woops, I need to change the commit message and title. Got my bits flipped.

@abellina abellina force-pushed the arrow_dynamic_cudart_jni branch from 01f3ad6 to 4745616 Compare April 7, 2021 15:17
@abellina abellina changed the title Set CUDA_USE_STATIC_CUDA_RUNTIME=ON for java builds [skip-ci] Set CUDA_USE_STATIC_CUDA_RUNTIME=OFF for java builds [skip-ci] Apr 7, 2021
@abellina
Copy link
Contributor Author

abellina commented Apr 7, 2021

Fixed.

@robertmaynard
Copy link
Contributor

If you want to work on fixing this at the source ( cudf ), this is working for me locally without having to add any extra cmake build flags:

diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index b49f1e3dda..525e5f9225 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -470,8 +470,14 @@ target_link_libraries(cudf
                   rmm::rmm)
 
 if(CUDA_STATIC_RUNTIME)
+    # Tell CMake what CUDA language runtime to use
+    set_target_properties(cudf PROPERTIES CUDA_RUNTIME_LIBRARY Static)
+    # Make sure to export to consumers what runtime we used
     target_link_libraries(cudf PUBLIC CUDA::cudart_static CUDA::cuda_driver)
 else()
+    # Tell CMake what CUDA language runtime to use
+    set_target_properties(cudf PROPERTIES CUDA_RUNTIME_LIBRARY Shared)
+    # Make sure to export to consumers what runtime we used
     target_link_libraries(cudf PUBLIC CUDA::cudart CUDA::cuda_driver)
 endif()
 
diff --git a/cpp/cmake/thirdparty/CUDF_GetArrow.cmake b/cpp/cmake/thirdparty/CUDF_GetArrow.cmake
index 002085c297..c1c29a693d 100644
--- a/cpp/cmake/thirdparty/CUDF_GetArrow.cmake
+++ b/cpp/cmake/thirdparty/CUDF_GetArrow.cmake
@@ -43,6 +43,7 @@ function(find_and_configure_arrow VERSION BUILD_STATIC)
         GIT_SHALLOW     TRUE
         SOURCE_SUBDIR   cpp
         OPTIONS         "CMAKE_VERBOSE_MAKEFILE ON"
+                        "CUDA_USE_STATIC_CUDA_RUNTIME ${CUDA_STATIC_RUNTIME}"
                         "ARROW_IPC ON"
                         "ARROW_CUDA ON"
                         "ARROW_DATASET ON"

@kkraus14
Copy link
Collaborator

kkraus14 commented Apr 7, 2021

@robertmaynard What's the interaction between CUDA_RUNTIME_LIBRARY and the CUDA::cudart / CUDA::cudart_static targets? I.e. what would happen if I set CUDA_RUNTIME_LIBRARY to Shared and then linked to CUDA::cudart_static?

@robertmaynard
Copy link
Contributor

@robertmaynard What's the interaction between CUDA_RUNTIME_LIBRARY and the CUDA::cudart / CUDA::cudart_static targets? I.e. what would happen if I set CUDA_RUNTIME_LIBRARY to Shared and then linked to CUDA::cudart_static?

They are independent currently, so you will get both on the link line. The CUDA::cudart / CUDA::cudart_static was originally added to allow C or C++ only projects to control CUDA runtime behavior without having to enable the CUDA language.

It would be impossible for CMake to auto select which one is correct, but in theory it could error out when provided a mixed set

@abellina
Copy link
Contributor Author

abellina commented Apr 7, 2021

@robertmaynard @kkraus14 if you guys are ok with the solution for cudf proper then I think @robertmaynard please post it. I don't know what I would add. Other than I do see that CUDA Libraries in ARROW is picking up only the .so (CUDA Libraries: /usr/local/cuda/lib64/libcudart.so), which is good. Also libcudf.so with Robert's patch looks good.

@robertmaynard
Copy link
Contributor

Opened #7887 to address the issue directly in cudf

@abellina
Copy link
Contributor Author

abellina commented Apr 7, 2021

So after this, I ran a test for all so's we use (we have libcudfjni.so, libcudf.so and also libnvcomp.so). Currently, I still see statically linked symbols in libnvcomp), but note that we dynamically link against nvcomp (so maybe we are ok). But I'll look at this a bit more to see if we need to change our nvcomp build. Note that the nvcomp build is specific to the java side.

@abellina
Copy link
Contributor Author

abellina commented Apr 7, 2021

Closing this in favor of: #7887

Continuing work on: #7896

@abellina abellina closed this Apr 7, 2021
@abellina abellina deleted the arrow_dynamic_cudart_jni branch April 7, 2021 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working Java Affects Java cuDF API. non-breaking Non-breaking change Performance Performance related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants