Skip to content

Make nvCOMP linkage configurable - #23415

Closed
gerashegalov wants to merge 3 commits into
NVIDIA:mainfrom
gerashegalov:gerashegalov/configurable-nvcomp-link
Closed

Make nvCOMP linkage configurable#23415
gerashegalov wants to merge 3 commits into
NVIDIA:mainfrom
gerashegalov:gerashegalov/configurable-nvcomp-link

Conversation

@gerashegalov

Copy link
Copy Markdown
Contributor

Summary

  • add CUDF_NVCOMP_LINKAGE=AUTO|STATIC|SHARED
  • preserve the existing target-selection behavior under the default AUTO mode
  • fail during configuration when an explicitly requested nvCOMP target is unavailable

Why

The cuDF Java distribution already packages the shared nvCOMP library. Downstream builds that otherwise force static dependencies currently embed a second copy of nvCOMP in libcudf.so while also shipping libnvcomp.so.

This option lets those distributions choose the packaged shared library without changing the default linkage policy for other cuDF consumers.

Related to NVIDIA/cudf-spark#15145.

Validation

  • configured and built Release libcudf.so with both STATIC and SHARED on CUDA 12.9.1 (sm_75)
  • verified the shared variant has DT_NEEDED: libnvcomp.so.5
  • verified Java load order libnvcomp.so, libcudf.so, libcudfjni.so

Measured on otherwise identical one-architecture builds:

nvCOMP linkage libcudf.so ZIP level 6
static 711,163,904 B 359,193,871 B
shared 691,845,960 B 344,989,786 B

This removes 18.42 MiB uncompressed and 13.55 MiB after DEFLATE.

@gerashegalov
gerashegalov requested a review from a team as a code owner July 23, 2026 18:34
@github-actions github-actions Bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Jul 23, 2026
@gerashegalov gerashegalov added Performance Performance related issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 23, 2026
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7bf28175-8094-422d-925a-716487e32c4d

📥 Commits

Reviewing files that changed from the base of the PR and between 0cc568c and 98546bc.

📒 Files selected for processing (1)
  • cpp/cmake/thirdparty/get_nvcomp.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
  • cpp/cmake/thirdparty/get_nvcomp.cmake

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added a CUDF_NVCOMP_LINKAGE configuration option to control nvCOMP linkage mode (AUTO, STATIC, or SHARED).
    • AUTO mode now automatically selects the best available nvCOMP library based on the build configuration and what’s present.
  • Bug Fixes

    • Added clear validation and error messages when an unsupported linkage type is requested.
    • Improved compatibility with packaging scenarios where a shared nvCOMP library is already provided.

Walkthrough

cuDF adds a CMake option for selecting nvCOMP linkage and updates target resolution to support validated static, shared, and automatic modes.

Changes

nvCOMP linkage configuration

Layer / File(s) Summary
Declare nvCOMP linkage option
cpp/CMakeLists.txt
Adds the CUDF_NVCOMP_LINKAGE cache option with AUTO, STATIC, and SHARED values.
Resolve nvCOMP target
cpp/cmake/thirdparty/get_nvcomp.cmake
Selects and validates the static or shared nvCOMP target according to the configured mode, including automatic fallback behavior and an updated SPDX copyright line.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: making nvCOMP linkage configurable.
Description check ✅ Passed The description matches the changes and explains the new linkage option, behavior, and motivation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
@gerashegalov

Copy link
Copy Markdown
Contributor Author

Not needed at the moment per NVIDIA/cudf-spark-jni#4881

gerashegalov added a commit to NVIDIA/cudf-spark-jni that referenced this pull request Jul 26, 2026
## Summary
- rely on the static nvCOMP copy already linked into `libcudf.so`
- stop packaging and loading the redundant nvCOMP shared libraries
- localize symbols from statically linked CUDA compiler and nvCOMP
implementation archives with `--exclude-libs`
- retain the regular ELF symbol table for stack traces and debugging

No GPU architectures are removed.

No upstream cuDF change is required. cuDF already selects
`nvcomp_static` when
`CUDF_BUILD_STATIC_DEPS=FORCE`, as used by this build. This supersedes
the previous dependency on
NVIDIA/cudf#23415.

Related to NVIDIA/cudf-spark#15145.

## Size results
Focused CUDA 12.9.1 Release builds with `CMAKE_CUDA_ARCHITECTURES=75`:

| Variant | Packaged native bytes | ZIP level 6 |
| --- | ---: | ---: |
| main: static nvCOMP in `libcudf.so` plus packaged `libnvcomp.so` |
754,765,376 B | 389,662,932 B |
| this PR: static nvCOMP only in `libcudf.so`, private symbols localized
| 693,387,896 B | 354,470,480 B |

The focused build reduces packaged native data by 58.53 MiB uncompressed
(8.13%) and 33.56 MiB
after DEFLATE (9.03%). `libcudfjni.so` is unchanged and omitted from
both rows.

These measurements use one architecture to make repeated comparisons
practical. The implementation
does not change the production architecture list.

## Linking and symbol compatibility
- the JAR contains `libcudf.so` and `libcudfjni.so`, with no nvCOMP
shared-library entries
- `libcudf.so` has no nvCOMP `DT_NEEDED` entry
- all 12 nvCOMP JNI entry points remain dynamically exported
- nvCOMP implementation symbols are absent from `.dynsym`
- the regular ELF `.symtab` remains available for diagnostics

## Validation
- native CUDA 12.9.1 Release build with `CMAKE_CUDA_ARCHITECTURES=75`
- `mvn package -DskipTests` through the normal patch lifecycle
- `NativeDepsLoaderTest`: 4 passed
- GPU `NvcompTest` LZ4 and Zstd cases: 2 passed
- `git diff --check`

---------

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Performance Performance related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant