Repository: rapidsai/cudf (language: python)

Issue:
Compile times are growing significantly
<!--

Thanks for opening an issue! To help the libGDF team handle your information
efficiently, please first ensure that there is no other issue present that
already describes the issue you have
(search at https://github.com/gpuopenanalytics/libgdf/issues?&q=is%3Aissue).

If there is no issue present please jump to a section below and delete the
irrelevant one depending on whether you are:

 * Making a feature request.
 * Reporting a bug.

For more general "how do I do X?" type questions, please speak visit
http://gpuopenanalytics.com/#/COMMUNITY for links to Slack and Google
Groups.

-->

## Feature request

<!--

Please include details of the feature you would like to see, why you would
like to see it/the use case

-->


As anyone who has built libgdf recently has surely noticed, the time to compile the library from scratch has grown significantly in the last few months. For example, compiling on all 10 cores of my i9-7900X  @ 3.30GHz takes 11 minutes as reported by `time make -j`.

Compiling with `-ftime-report` may be a good place to start to see where all the compilation time is being spent.

This is likely due to the large amount of template instantiation that is required for instantiating functions for all possible types. We should make sure that best practices are being followed in template instantiation such that a template for a given type is only having to be instantiated once via explicit instantiation.

Much of our code is implemented in headers, which causes it to be recompiled everywhere that header is included. Using pre-compiled headers may help: 
https://gcc.gnu.org/onlinedocs/gcc/Precompiled-Headers.html
http://itscompiling.eu/2017/01/12/precompiled-headers-cpp-compilation/

Furthermore, code should be scrubbed from excessive and unnecessary `#include`s. Compiling with `-MD` will show what files are being included 

Here's a Clang tool that ensures you only include the necessary headers: https://github.com/include-what-you-use/include-what-you-use

Here's a Clang tool to profile time spent in template instantiation: https://github.com/mikael-s-persson/templight








Discussion / hints:
Do the clang tools work with nvcc? Much of our template time is in nvcc. You can use `nvcc --time foo.csv` to dump timing for different nvcc phases.
I'm not sure. You can technically get Clang to compile device code, so that may be a path worth exploring using Clang + these tools.
Any updates on this? I'd love to use precompiled headers with CUDA projects.
Compile time continues to grow, but that is largely because our supported feature set and supported types continue to grow.  In 0.15 we are aiming to add at least 10 new types (4 unsigned int types, 4 timestamp types, list column type, decimal fixed-point type). Naturally this will increase compile time and binary size.

Meanwhile, in 0.14 we dropped all of the legacy APIs that were previously deprecated, which reduced compile time a bit, and significantly reduced binary size.  There have been and will continue to be various efforts to reduce compile time  of  certain components. We are investigating possibly splitting libcudf into multiple libraries.

We have not discussed precompiled headers. 
@jrhemstad @harrism , is this still a relevant issue?
Our compile time is worse than ever, so I guess its still relevant. We could benefit from someone putting in a concerted effort to eliminate unnecessary includes across the library. 
> I'm not sure. You can technically get Clang to compile device code, so that may be a path worth exploring using Clang + these tools.

Out of curiosity I gave this a quick shot. (Unsurprisingly) clang does not currently support the experimental CUDA features that we have enabled (`--expt-extended-lambda --expt-relaxed-constexpr`) so the compilation terminates pretty quickly. Not sure if there are other downstream issues that we would face if we attempted this after stripping those out (not suggesting that we should, although #7795 remains open so maybe it is worth pursuing).
> clang does not currently support the experimental CUDA features that we have enabled (--expt-extended-lambda --expt-relaxed-constexpr)

Pretty sure clang supports those features natively without the need for any extra compile flags. I'm guessing the error was caused by clang not recognizing those flags. 
You're right, it does. I removed those and made some progress, but not nearly enough for a working build with clang yet. Here's a list of necessary changes so far:
- Remove all `CUDF_CUDA_FLAGS` set in ConfigureCUDA.cmake. Most are either unsupported or ignored (including some of the warnings flags), so the blanket removal is easiest.
- Remove `[[nodiscard]]` attributes, which don't appear to be supported in clang device code yet.
- Set `-D__STRICT_ANSI__` as a CUDA compiler flag. Otherwise it tries to compile float128 code, which is not yet supported in clang.
- Pass `-fcuda-allow-variadic-functions` to the clang compiler (or via the CMake configure CLI `-DCMAKE_CUDA_FLAGS="-Xclang -fcuda-allow-variadic-functions"`

At this point I start seeing failures like this:
```
...type_traits:2777:5: error: no type named 'type' in 'std::invoke_result<cudf::detail::indexalator_factory::nullable_index_accessor, int>'
```
and 
```
error: type 'thrust::transform_iterator<(lambda at ...gather.cu:49:26), cudf::detail::input_indexalator>::super_t' (aka 'iterator_adaptor<transform_iterator<(lambda at ...gather.cu:49:26), cudf::detail::input_indexalator, thrust::use_default, thrust::use_default>, cudf::detail::input_indexalator, int, thrust::use_default, std::random_access_iterator_tag, int>') is not a direct or virtual base of 'thrust::transform_iterator<(lambda at ...gather.cu:49:26), cudf::detail::input_indexalator>'
```
I need to track this down a bit further, but it looks like some aspect of how thrust SFINAEs different code paths isn't supported in clang device code yet either.
I tried to build cuco with clang about a year ago and was blocked by its dependencies like thrust or libcudacxx that cannot be built with clang. To find how much effort is required to build device code with clang, I would suggest starting with a smaller library like cuco and see how it goes from there.

Related issues:

- https://github.com/NVIDIA/cccl/issues/991
- https://github.com/NVIDIA/cuCollections/issues/128
Well then... looks like we've got to work our way all the way up the stack for this. For the purpose of something like clang-tidy we might be able to get some partial results based on the discussion in https://github.com/rapidsai/raft/pull/424, but that's probably only partial support at best and I don't know if that will work with the other tools of interest like IWYU.
Compile times are an ever-present problem for us. This issue as currently framed isn't clearly actionable, so let's lay out some concrete points.

> We should make sure that best practices are being followed in template instantiation 

#379 implemented the `type_dispatcher`, which now controls all our template instantiations and ensures that we have a minimal set.

> Compiling with -ftime-report may be a good place to start to see where all the compilation time is being spent.

Since #9631 we have been tracking build times in CI. We monitor this and keep an eye on TUs that are particularly slow to compile. Where necessary, we have reacted to slow compilation by reorganizing the code and explicitly instantiating templates.

> Furthermore, code should be scrubbed from excessive and unnecessary #includes.

This seems like the main action item remaining. As discussed above, `include-what-you-use` is a good tool for this, so I would consider evaluating and either implementing or rejecting that as the only thing left to do here. Since clang compilation is the bottleneck here, I propose that we just get our C/C++ source files working first. Once we have systematic approaches in place for that and are regularly checking on the quality, we can incrementally work up to getting CUDA source files working since that is a much heavier lift (and at that point we can also split between host and device code in cu files). This is similar to the approach we are taking in #16958 for clang-tidy.

Fix patch:
diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh
deleted file mode 100755
index 4d5d3fc3136..00000000000
--- a/ci/clang_tidy.sh
+++ /dev/null
@@ -1,29 +0,0 @@
-#!/bin/bash
-# Copyright (c) 2024, NVIDIA CORPORATION.
-
-set -euo pipefail
-
-rapids-logger "Create clang-tidy conda environment"
-. /opt/conda/etc/profile.d/conda.sh
-
-ENV_YAML_DIR="$(mktemp -d)"
-
-rapids-dependency-file-generator \
-  --output conda \
-  --file-key clang_tidy \
-  --matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION}" | tee "${ENV_YAML_DIR}/env.yaml"
-
-rapids-mamba-retry env create --yes -f "${ENV_YAML_DIR}/env.yaml" -n clang_tidy
-
-# Temporarily allow unbound variables for conda activation.
-set +u
-conda activate clang_tidy
-set -u
-
-RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)"
-
-source rapids-configure-sccache
-
-# Run the build via CMake, which will run clang-tidy when CUDF_CLANG_TIDY is enabled.
-cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON -GNinja
-cmake --build cpp/build
diff --git a/ci/cpp_linters.sh b/ci/cpp_linters.sh
new file mode 100755
index 00000000000..a7c7255456f
--- /dev/null
+++ b/ci/cpp_linters.sh
@@ -0,0 +1,47 @@
+#!/bin/bash
+# Copyright (c) 2024, NVIDIA CORPORATION.
+
+set -euo pipefail
+
+rapids-logger "Create checks conda environment"
+. /opt/conda/etc/profile.d/conda.sh
+
+ENV_YAML_DIR="$(mktemp -d)"
+
+rapids-dependency-file-generator \
+  --output conda \
+  --file-key clang_tidy \
+  --matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION}" | tee "${ENV_YAML_DIR}/env.yaml"
+
+rapids-mamba-retry env create --yes -f "${ENV_YAML_DIR}/env.yaml" -n clang_tidy
+
+# Temporarily allow unbound variables for conda activation.
+set +u
+conda activate clang_tidy
+set -u
+
+RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)"
+
+source rapids-configure-sccache
+
+# TODO: For testing purposes, clone and build IWYU. We can switch to a release
+# once a clang 19-compatible version is available, which should be soon
+# (https://github.com/include-what-you-use/include-what-you-use/issues/1641).
+git clone --depth 1 https://github.com/include-what-you-use/include-what-you-use.git
+pushd include-what-you-use
+# IWYU's CMake build uses some Python scripts that assume that the cwd is
+# importable, so support that legacy behavior.
+export PYTHONPATH=${PWD}:${PYTHONPATH:-}
+cmake -S . -B build -GNinja --install-prefix=${CONDA_PREFIX}
+cmake --build build
+cmake --install build
+popd
+
+# Run the build via CMake, which will run clang-tidy when CUDF_STATIC_LINTERS is enabled.
+cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_STATIC_LINTERS=ON -GNinja
+cmake --build cpp/build 2>&1 | python cpp/scripts/parse_iwyu_output.py
+
+# Remove invalid components of the path for local usage. The path below is
+# valid in the CI due to where the project is cloned, but presumably the fixes
+# will be applied locally from inside a clone of cudf.
+sed -i 's/\/__w\/cudf\/cudf\///' iwyu_results.txt
diff --git a/cpp/.clang-tidy b/cpp/.clang-tidy
index 12120a5c6d1..0e5699876fc 100644
--- a/cpp/.clang-tidy
+++ b/cpp/.clang-tidy
@@ -39,8 +39,8 @@ Checks:
        -clang-analyzer-optin.core.EnumCastOutOfRange,
        -clang-analyzer-optin.cplusplus.UninitializedObject'
 
-WarningsAsErrors: '*'
-HeaderFilterRegex: '.*cudf/cpp/(src|include|tests).*'
+WarningsAsErrors: ''
+HeaderFilterRegex: '.*cudf/cpp/(src|include).*'
 ExcludeHeaderFilterRegex: '.*(Message_generated.h|Schema_generated.h|brotli_dict.hpp|unbz2.hpp|cxxopts.hpp).*'
 FormatStyle:     none
 CheckOptions:
diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index bfa4bf80724..d3bf7019e35 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -88,7 +88,7 @@ option(
   ${DEFAULT_CUDF_BUILD_STREAMS_TEST_UTIL}
 )
 mark_as_advanced(CUDF_BUILD_STREAMS_TEST_UTIL)
-option(CUDF_CLANG_TIDY "Enable clang-tidy checking" OFF)
+option(CUDF_STATIC_LINTERS "Enable static linters during compilation" OFF)
 
 message(VERBOSE "CUDF: Build with NVTX support: ${USE_NVTX}")
 message(VERBOSE "CUDF: Configure CMake to build tests: ${BUILD_TESTS}")
@@ -146,8 +146,10 @@ if(NOT CUDF_GENERATED_INCLUDE_DIR)
 endif()
 
 # ##################################################################################################
-# * clang-tidy configuration ----------------------------------------------------------------------
-if(CUDF_CLANG_TIDY)
+# * linter configuration ---------------------------------------------------------------------------
+if(CUDF_STATIC_LINTERS)
+  # For simplicity, for now we assume that all linters can be installed into an environment where
+  # any linter is being run. We could relax this requirement if desired.
   find_program(
     CLANG_TIDY_EXE
     NAMES "clang-tidy"
@@ -174,24 +176,48 @@ if(CUDF_CLANG_TIDY)
         "clang-tidy version ${expected_clang_tidy_version} is required, but found ${LLVM_VERSION}"
     )
   endif()
+
+  find_program(IWYU_EXE NAMES include-what-you-use iwyu REQUIRED)
 endif()
 
 # Turn on the clang-tidy property for a target excluding the files specified in SKIPPED_FILES.
-function(enable_clang_tidy target)
-  set(_tidy_options)
+function(enable_static_checkers target)
+  set(_tidy_options IWYU CLANG_TIDY)
   set(_tidy_one_value)
   set(_tidy_multi_value SKIPPED_FILES)
   cmake_parse_arguments(
-    _TIDY "${_tidy_options}" "${_tidy_one_value}" "${_tidy_multi_value}" ${ARGN}
+    _LINT "${_tidy_options}" "${_tidy_one_value}" "${_tidy_multi_value}" ${ARGN}
   )
 
-  if(CUDF_CLANG_TIDY)
-    # clang will complain about unused link libraries on the compile line unless we specify
-    # -Qunused-arguments.
-    set_target_properties(
-      ${target} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_EXE};--extra-arg=-Qunused-arguments"
-    )
-    foreach(file IN LISTS _TIDY_SKIPPED_FILES)
+  if(CUDF_STATIC_LINTERS)
+    if(_LINT_CLANG_TIDY)
+      # clang will complain about unused link libraries on the compile line unless we specify
+      # -Qunused-arguments.
+      set_target_properties(
+        ${target} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_EXE};--extra-arg=-Qunused-arguments"
+      )
+    endif()
+    if(_LINT_IWYU)
+      # A few extra warnings pop up when building with IWYU. I'm not sure why, but they are not
+      # relevant since they don't show up in any other build so it's better to suppress them until
+      # we can figure out the cause. Setting this as part of CXX_INCLUDE_WHAT_YOU_USE does not
+      # appear to be sufficient, we must also ensure that it is set to the underlying target's CXX
+      # compile flags. To do this completely cleanly we should modify the flags on the target rather
+      # than the global CUDF_CXX_FLAGS, but this solution is good enough for now since we never run
+      # the linters on real builds.
+      foreach(_flag -Wno-missing-braces -Wno-absolute-value -Wunneeded-internal-declaration)
+        list(FIND CUDF_CXX_FLAGS "${flag}" _flag_index)
+        if(_flag_index EQUAL -1)
+          list(APPEND CUDF_CXX_FLAGS ${flag})
+        endif()
+      endforeach()
+      set(CUDF_CXX_FLAGS
+          "${CUDF_CXX_FLAGS}"
+          PARENT_SCOPE
+      )
+      set_target_properties(${target} PROPERTIES CXX_INCLUDE_WHAT_YOU_USE "${IWYU_EXE}")
+    endif()
+    foreach(file IN LISTS _LINT_SKIPPED_FILES)
       set_source_files_properties(${file} PROPERTIES SKIP_LINTING ON)
     endforeach()
   endif()
@@ -771,11 +797,15 @@ set_target_properties(
              INTERFACE_POSITION_INDEPENDENT_CODE ON
 )
 
+# Note: This must come before the target_compile_options below so that the function can modify the
+# flags if necessary.
+enable_static_checkers(
+  cudf SKIPPED_FILES src/io/comp/cpu_unbz2.cpp src/io/comp/brotli_dict.cpp CLANG_TIDY IWYU
+)
 target_compile_options(
   cudf PRIVATE "$<$<COMPILE_LANGUAGE:CXX>:${CUDF_CXX_FLAGS}>"
                "$<$<COMPILE_LANGUAGE:CUDA>:${CUDF_CUDA_FLAGS}>"
 )
-enable_clang_tidy(cudf SKIPPED_FILES src/io/comp/cpu_unbz2.cpp src/io/comp/brotli_dict.cpp)
 
 if(CUDF_BUILD_STACKTRACE_DEBUG)
   # Remove any optimization level to avoid nvcc warning "incompatible redefinition for option
diff --git a/cpp/scripts/parse_iwyu_output.py b/cpp/scripts/parse_iwyu_output.py
new file mode 100644
index 00000000000..822a980a1a8
--- /dev/null
+++ b/cpp/scripts/parse_iwyu_output.py
@@ -0,0 +1,170 @@
+# Copyright (c) 2024, NVIDIA CORPORATION.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""Helper script to modify IWYU output to only include removals.
+
+Lines that are not from include-what-you-use are removed from the output.
+"""
+
+import argparse
+import re
+from enum import Enum
+
+
+class Mode(Enum):
+    NORMAL = 0
+    ADD = 1
+    REMOVE = 2
+    FULL_INCLUDE_LIST = 3
+
+
+def extract_include_file(include_line):
+    """Extract the core file path from an #include directive."""
+    match = re.search(r'#include\s+[<"]([^">]+)[">]', include_line)
+    if match:
+        return match.group(1)
+    return None
+
+
+def parse_output(input_stream):
+    include_modifications = {}
+    current_file = None
+    mode = Mode.NORMAL
+
+    for line in input_stream:
+        if match := re.match(r"(\/\S+) should add these lines:", line):
+            current_file = match.group(1)
+            include_modifications.setdefault(
+                current_file,
+                {
+                    "add_includes": [],
+                    "remove_includes": [],
+                    "full_include_list": [],
+                },
+            )
+            mode = Mode.ADD
+        elif match := re.match(r"(\/\S+) should remove these lines:", line):
+            mode = Mode.REMOVE
+        elif match := re.match(r"The full include-list for (\/\S+):", line):
+            mode = Mode.FULL_INCLUDE_LIST
+        elif line.strip() == "---":
+            current_file = None
+            mode = Mode.NORMAL
+        else:
+            if current_file:
+                if mode == Mode.ADD:
+                    include_modifications[current_file]["add_includes"].append(
+                        line.strip()
+                    )
+                elif mode == Mode.REMOVE:
+                    include_modifications[current_file][
+                        "remove_includes"
+                    ].append(line.strip())
+                elif mode == Mode.FULL_INCLUDE_LIST:
+                    include_modifications[current_file][
+                        "full_include_list"
+                    ].append(line.strip())
+            else:
+                if (
+                    line.strip()
+                    and "include-what-you-use reported diagnostics" not in line
+                    and "In file included from" not in line
+                    and "has correct #includes/fwd-decls" not in line
+                ):
+                    print(line, end="")
+
+    return include_modifications
+
+
+def post_process_includes(include_modifications):
+    """Deduplicate and remove redundant entries from add and remove includes."""
+    for mods in include_modifications.values():
+        # Deduplicate add_includes and remove_includes
+        mods["add_includes"] = list(set(mods["add_includes"]))
+        mods["remove_includes"] = list(set(mods["remove_includes"]))
+
+        # Extract file paths from add_includes and remove_includes
+        add_files = {
+            extract_include_file(line) for line in mods["add_includes"]
+        }
+        remove_files = {
+            extract_include_file(line) for line in mods["remove_includes"]
+        }
+
+        # Remove entries that exist in both add_includes and remove_includes
+        common_files = add_files & remove_files
+        mods["add_includes"] = [
+            line
+            for line in mods["add_includes"]
+            if extract_include_file(line) not in common_files
+        ]
+        mods["remove_includes"] = [
+            line
+            for line in mods["remove_includes"]
+            if extract_include_file(line) not in common_files
+        ]
+
+        # Remove entries that exist in add_includes from full_include_list
+        mods["full_include_list"] = [
+            include
+            for include in mods["full_include_list"]
+            if extract_include_file(include) not in add_files
+        ]
+
+
+def write_output(include_modifications, output_stream):
+    for filename, mods in include_modifications.items():
+        if mods["remove_includes"]:
+            # IWYU requires all sections to exist, so we write out this header even
+            # though we never write out any actual additions.
+            output_stream.write(f"{filename} should add these lines:\n\n")
+
+            output_stream.write(f"{filename} should remove these lines:\n")
+            for line in mods["remove_includes"]:
+                output_stream.write(line + "\n")
+            output_stream.write("\n")
+
+            output_stream.write(f"The full include-list for {filename}:\n")
+            for line in mods["full_include_list"]:
+                output_stream.write(line + "\n")
+            output_stream.write("---\n")
+
+
+def main():
+    parser = argparse.ArgumentParser(
+        description="Process include modifications from a build output log."
+    )
+    parser.add_argument(
+        "input",
+        nargs="?",
+        type=argparse.FileType("r"),
+        default="-",
+        help="Input file to read (default: stdin)",
+    )
+    parser.add_argument(
+        "--output",
+        type=argparse.FileType("w"),
+        default="iwyu_results.txt",
+        help="Output file to write (default: iwyu_output.txt)",
+    )
+    args = parser.parse_args()
+
+    include_modifications = parse_output(args.input)
+    post_process_includes(include_modifications)
+    write_output(include_modifications, args.output)
+
+
+if __name__ == "__main__":
+    main()
diff --git a/dependencies.yaml b/dependencies.yaml
index 4c6aefe996f..4080cdca65c 100644
--- a/dependencies.yaml
+++ b/dependencies.yaml
@@ -587,6 +587,12 @@ dependencies:
         packages:
           - clang==19.1.0
           - clang-tools==19.1.0
+          # TODO: These are build requirements for IWYU and can be replaced
+          # with IWYU itself once a conda package of IWYU supporting clang 19
+          # is available.
+          - clangdev==19.1.0
+          - llvm==19.1.0
+          - llvmdev==19.1.0
   docs:
     common:
       - output_types: [conda]


Test patch:
diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml
index 1275aad757c..ad3f5940b94 100644
--- a/.github/workflows/test.yaml
+++ b/.github/workflows/test.yaml
@@ -63,7 +63,8 @@ jobs:
       branch: ${{ inputs.branch }}
       date: ${{ inputs.date }}
       sha: ${{ inputs.sha }}
-      run_script: "ci/clang_tidy.sh"
+      run_script: "ci/cpp_linters.sh"
+      file_to_upload: iwyu_results.txt
   conda-python-cudf-tests:
     secrets: inherit
     uses: rapidsai/shared-workflows/.github/workflows/conda-python-tests.yaml@branch-24.12
diff --git a/cpp/tests/CMakeLists.txt b/cpp/tests/CMakeLists.txt
index 23632f6fbba..51b41c3f7b2 100644
--- a/cpp/tests/CMakeLists.txt
+++ b/cpp/tests/CMakeLists.txt
@@ -83,7 +83,6 @@ function(ConfigureTest CMAKE_TEST_NAME)
         "GTEST_CUDF_STREAM_MODE=new_${_CUDF_TEST_STREAM_MODE}_default;LD_PRELOAD=$<TARGET_FILE:cudf_identify_stream_usage_mode_${_CUDF_TEST_STREAM_MODE}>"
     )
   endif()
-  enable_clang_tidy(${CMAKE_TEST_NAME})
 endfunction()
 
 # ##################################################################################################