Skip to content

Add noinline to n_table_comparator::operator() - #22699

Merged
rapids-bot[bot] merged 1 commit into
NVIDIA:mainfrom
davidwendt:noinline-attribute
May 28, 2026
Merged

Add noinline to n_table_comparator::operator()#22699
rapids-bot[bot] merged 1 commit into
NVIDIA:mainfrom
davidwendt:noinline-attribute

Conversation

@davidwendt

Copy link
Copy Markdown
Contributor

Description

Adds the noinline declaration to the n_table_comparator::operator() function.
This is based on the discussion and results here: #22675 (comment)

The attribute was necessary for the debug build but showed no issue in runtime for a release build. This PR makes the declaration non-conditional to make the code simpler to maintain.

Checklist

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

@davidwendt davidwendt self-assigned this May 28, 2026
@davidwendt
davidwendt requested a review from a team as a code owner May 28, 2026 19:36
@davidwendt davidwendt added the 3 - Ready for Review Ready for review by team label May 28, 2026
@davidwendt davidwendt added libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 28, 2026
@davidwendt

Copy link
Copy Markdown
Contributor Author

Re-pasting the benchmark results here from the comment linked in the description:

# groupby_max_cardinality

## [0] NVIDIA RTX A6000

|  num_aggregations  |  cardinality  |   Ref Time |   Cmp Time |         Diff |   %Diff |
|--------------------|---------------|------------|------------|--------------|---------|
|         1          |      20       |  11.572 ms |  11.566 ms |    -5.770 us |  -0.05% |
|         2          |      20       |  20.778 ms |  20.561 ms |  -216.966 us |  -1.04% |
|         3          |      20       |  29.776 ms |  29.408 ms |  -367.660 us |  -1.23% |
|         4          |      20       |  38.591 ms |  38.433 ms |  -157.464 us |  -0.41% |
|         5          |      20       |  47.709 ms |  47.931 ms |   222.771 us |   0.47% |
|         6          |      20       |  57.048 ms |  56.741 ms |  -307.554 us |  -0.54% |
|         7          |      20       |  66.286 ms |  65.885 ms |  -400.842 us |  -0.60% |
|         8          |      20       |  75.908 ms |  75.817 ms |   -90.658 us |  -0.12% |
|         1          |      50       |  14.900 ms |  14.879 ms |   -21.199 us |  -0.14% |
|         2          |      50       |  27.196 ms |  27.369 ms |   172.918 us |   0.64% |
|         3          |      50       |  39.764 ms |  39.792 ms |    27.681 us |   0.07% |
|         4          |      50       |  52.335 ms |  52.279 ms |   -55.994 us |  -0.11% |
|         5          |      50       |  64.493 ms |  64.526 ms |    32.748 us |   0.05% |
|         6          |      50       |  76.919 ms |  76.889 ms |   -29.153 us |  -0.04% |
|         7          |      50       |  89.473 ms |  89.633 ms |   160.351 us |   0.18% |
|         8          |      50       | 101.539 ms | 101.834 ms |   294.929 us |   0.29% |
|         1          |      100      |  12.136 ms |  12.110 ms |   -26.572 us |  -0.22% |
|         2          |      100      |  21.256 ms |  21.446 ms |   189.980 us |   0.89% |
|         3          |      100      |  30.662 ms |  30.563 ms |   -98.973 us |  -0.32% |
|         4          |      100      |  39.747 ms |  39.705 ms |   -41.677 us |  -0.10% |
|         5          |      100      |  48.788 ms |  48.889 ms |   101.093 us |   0.21% |
|         6          |      100      |  57.825 ms |  58.251 ms |   426.049 us |   0.74% |
|         7          |      100      |  67.186 ms |  67.521 ms |   334.501 us |   0.50% |
|         8          |      100      |  76.977 ms |  76.955 ms |   -21.540 us |  -0.03% |
|         1          |     1000      |   5.658 ms |   5.648 ms |    -9.629 us |  -0.17% |
|         2          |     1000      |   7.696 ms |   7.714 ms |    18.395 us |   0.24% |
|         3          |     1000      |   9.794 ms |   9.771 ms |   -23.726 us |  -0.24% |
|         4          |     1000      |  13.419 ms |  11.878 ms | -1541.330 us | -11.49% |
|         5          |     1000      |  14.654 ms |  13.954 ms |  -699.957 us |  -4.78% |
|         6          |     1000      |  18.359 ms |  16.132 ms | -2227.036 us | -12.13% |
|         7          |     1000      |  19.741 ms |  18.164 ms | -1577.090 us |  -7.99% |
|         8          |     1000      |  21.966 ms |  20.370 ms | -1596.146 us |  -7.27% |
|         1          |     10000     |   5.316 ms |   5.317 ms |     0.145 us |   0.00% |
|         2          |     10000     |   6.241 ms |   6.236 ms |    -5.247 us |  -0.08% |
|         3          |     10000     |   7.149 ms |   7.153 ms |     4.161 us |   0.06% |
|         4          |     10000     |   8.100 ms |   8.056 ms |   -43.801 us |  -0.54% |
|         5          |     10000     |   9.038 ms |   9.049 ms |    11.261 us |   0.12% |
|         6          |     10000     |   9.923 ms |   9.994 ms |    70.206 us |   0.71% |
|         7          |     10000     |  10.901 ms |  10.926 ms |    25.083 us |   0.23% |
|         8          |     10000     |  11.842 ms |  11.837 ms |    -4.250 us |  -0.04% |
|         1          |    100000     |   9.847 ms |   9.968 ms |   121.809 us |   1.24% |
|         2          |    100000     |  10.850 ms |  10.841 ms |    -9.100 us |  -0.08% |
|         3          |    100000     |  11.748 ms |  11.729 ms |   -19.117 us |  -0.16% |
|         4          |    100000     |  12.621 ms |  12.640 ms |    18.991 us |   0.15% |
|         5          |    100000     |  13.535 ms |  13.575 ms |    39.998 us |   0.30% |
|         6          |    100000     |  14.376 ms |  14.438 ms |    61.369 us |   0.43% |
|         7          |    100000     |  15.411 ms |  15.412 ms |     1.339 us |   0.01% |
|         8          |    100000     |  16.334 ms |  16.320 ms |   -13.540 us |  -0.08% |
|         1          |    1000000    |  11.713 ms |  11.868 ms |   154.464 us |   1.32% |
|         2          |    1000000    |  12.900 ms |  12.927 ms |    26.940 us |   0.21% |
|         3          |    1000000    |  13.844 ms |  13.970 ms |   125.494 us |   0.91% |
|         4          |    1000000    |  15.000 ms |  15.066 ms |    65.585 us |   0.44% |
|         5          |    1000000    |  16.037 ms |  16.012 ms |   -25.173 us |  -0.16% |
|         6          |    1000000    |  16.993 ms |  17.024 ms |    30.910 us |   0.18% |
|         7          |    1000000    |  18.103 ms |  18.133 ms |    30.247 us |   0.17% |
|         8          |    1000000    |  19.068 ms |  19.233 ms |   164.815 us |   0.86% |

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown

Actionable comments posted: 0

@coderabbitai

coderabbitai Bot commented May 28, 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: 5de3f6db-c1fd-48ae-9676-a8356a05c951

📥 Commits

Reviewing files that changed from the base of the PR and between 7a9a1db and 951b31e.

📒 Files selected for processing (1)
  • cpp/src/groupby/streaming_groupby/common.cuh

📝 Walkthrough

Summary by CodeRabbit

  • Chores
    • Internal compilation optimization applied to improve performance in both debug and release builds. No user-facing functionality changes.

Walkthrough

The PR unconditionally applies the __attribute__((noinline)) annotation to the n_table_comparator::operator() method in the streaming groupby kernel. Previously, the attribute was guarded by #ifndef NDEBUG, making it conditional on debug builds only. The change ensures the comparator function is never inlined in any build configuration.

Changes

Comparator Inlining Update

Layer / File(s) Summary
Unconditional noinline attribute on comparator operator
cpp/src/groupby/streaming_groupby/common.cuh
The n_table_comparator::operator() method now applies __attribute__((noinline)) unconditionally, removed the #ifndef NDEBUG preprocessor guard, and ensures consistent non-inlining behavior across all build configurations.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • rapidsai/cudf#22675: Both PRs modify the same device functor n_table_comparator::operator() in cpp/src/groupby/streaming_groupby/common.cuh; the earlier PR made __attribute__((noinline)) conditional on NDEBUG, while this PR makes the noinline behavior unconditional.

Suggested reviewers

  • PointKernel
  • kingcrimsontianyu
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add noinline to n_table_comparator::operator()' accurately and specifically describes the main change made in the pull request.
Description check ✅ Passed The description clearly explains the purpose of adding the noinline attribute, references the relevant discussion, and explains the motivation for making it unconditional.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@davidwendt

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit ad987ea into NVIDIA:main May 28, 2026
119 checks passed
@davidwendt
davidwendt deleted the noinline-attribute branch May 28, 2026 20:52
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 improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants