Skip to content

Destroy RTCX cache before teardown - #22843

Merged
rapids-bot[bot] merged 5 commits into
NVIDIA:mainfrom
fallintoplace:fix-rtcx-teardown-order
Jun 17, 2026
Merged

Destroy RTCX cache before teardown#22843
rapids-bot[bot] merged 5 commits into
NVIDIA:mainfrom
fallintoplace:fix-rtcx-teardown-order

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • Destroy the JIT bundle and RTCX cache before calling rtcx::teardown().
  • Add a context regression that runs a JIT compute_column, tears down, reinitializes, and repeats.

Details

rtcx::cache_t can retain cached rtcx::library objects whose destructor unloads the CUDA library through RTCX driver handles. The context destructor previously called rtcx::teardown() in the destructor body, before member unique_ptrs were destroyed, so cached libraries could be released after RTCX reset those handles.

This makes the teardown order explicit: JIT-owned state is destroyed first, then RTCX global state is reset.

Validation

  • pre-commit run --files cpp/src/runtime/context.cpp cpp/tests/utilities_tests/context_tests.cpp
  • git diff --check

Local UTILITIES_TEST was not run because this checkout does not have a configured cpp/build/CMake build tree.

@fallintoplace
fallintoplace requested a review from a team as a code owner June 10, 2026 21:04
@copy-pr-bot

copy-pr-bot Bot commented Jun 10, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 10, 2026
@davidwendt
davidwendt requested a review from lamarrr June 10, 2026 21:12
@coderabbitai

coderabbitai Bot commented Jun 10, 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: 0dbfc8ec-654a-4955-af5a-ae0c9ef473da

📥 Commits

Reviewing files that changed from the base of the PR and between 7b8192d and a96996d.

📒 Files selected for processing (3)
  • .clang-format
  • cpp/src/runtime/context.cpp
  • cpp/tests/utilities_tests/context_tests.cpp
✅ Files skipped from review due to trivial changes (1)
  • .clang-format
🚧 Files skipped from review as they are similar to previous changes (2)
  • cpp/src/runtime/context.cpp
  • cpp/tests/utilities_tests/context_tests.cpp

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved shutdown behavior by ensuring runtime/cache resources are released in the correct order prior to final runtime teardown.
  • Tests

    • Added a regression test validating JIT cache behavior across multiple init/teardown cycles, confirming computations succeed and teardown does not throw.
  • Chores

    • Updated formatting pragmas and adjusted the SPDX copyright line.

Walkthrough

This PR fixes the context destructor to reset JIT cache member objects (_jit_bundle and _rtcx_cache) before calling rtcx::teardown(), changing the destruction order. A new test validates that repeated JIT cache initialization, computation, and teardown cycles complete safely without exceptions. Supporting changes include a formatter pragma configuration update and copyright header refresh.

Changes

JIT Cache Teardown Fix

Layer / File(s) Summary
Formatter pragma configuration
.clang-format
Updated CommentPragmas setting to match both IWYU pragmas and SPDX-prefixed pragma comments.
Destructor teardown order with copyright update
cpp/src/runtime/context.cpp
context::~context() now explicitly resets _jit_bundle and _rtcx_cache before invoking rtcx::teardown(), ensuring cache objects are released prior to global runtime teardown. SPDX copyright header updated to reference NVIDIA CORPORATION & AFFILIATES.
Test validation for teardown cycles
cpp/tests/utilities_tests/context_tests.cpp
Added cuDF headers for column/table/AST/transform APIs; introduced TeardownAfterJitCacheUse test that cycles twice through INIT_JIT_CACHE initialization, constructs fixed-width columns, builds an addition AST expression from column references, executes compute_column_jit, and validates both compute and teardown complete without exceptions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

improvement

Suggested reviewers

  • vyasr
  • mhaseeb123
  • lamarrr
  • bdice
  • wence-
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the primary change: explicitly destroying the RTCX cache before calling teardown, which is the core fix in the context destructor.
Description check ✅ Passed The description is directly related to the changeset, explaining the rationale for the teardown order fix and detailing both the implementation changes and the regression test added.
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 davidwendt added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Jun 10, 2026
@davidwendt

Copy link
Copy Markdown
Contributor

/ok to test 750f97e

@davidwendt davidwendt removed their assignment Jun 11, 2026
@davidwendt

Copy link
Copy Markdown
Contributor

/ok to test 9bd96e2

Comment on lines +27 to +42
namespace {

void run_jit_compute_column()
{
auto c_0 = cudf::test::fixed_width_column_wrapper<cudf::size_type>{3, 20, 1, 50};
auto c_1 = cudf::test::fixed_width_column_wrapper<cudf::size_type>{10, 7, 20, 0};
auto table = cudf::table_view{{c_0, c_1}};
auto col_ref_0 = cudf::ast::column_reference(0);
auto col_ref_1 = cudf::ast::column_reference(1);
auto expression = cudf::ast::operation(cudf::ast::ast_operator::ADD, col_ref_0, col_ref_1);

auto result = cudf::compute_column_jit(table, expression);
EXPECT_EQ(result->size(), cudf::size_type{4});
}

} // namespace

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
namespace {
void run_jit_compute_column()
{
auto c_0 = cudf::test::fixed_width_column_wrapper<cudf::size_type>{3, 20, 1, 50};
auto c_1 = cudf::test::fixed_width_column_wrapper<cudf::size_type>{10, 7, 20, 0};
auto table = cudf::table_view{{c_0, c_1}};
auto col_ref_0 = cudf::ast::column_reference(0);
auto col_ref_1 = cudf::ast::column_reference(1);
auto expression = cudf::ast::operation(cudf::ast::ast_operator::ADD, col_ref_0, col_ref_1);
auto result = cudf::compute_column_jit(table, expression);
EXPECT_EQ(result->size(), cudf::size_type{4});
}
} // namespace

Comment on lines +60 to +70
TEST_F(ContextTest, TeardownAfterJitCacheUse)
{
cudf::initialize(cudf::init_flags::INIT_JIT_CACHE);
ASSERT_NO_THROW(run_jit_compute_column());
EXPECT_NO_THROW(cudf::teardown());

cudf::initialize(cudf::init_flags::INIT_JIT_CACHE);
ASSERT_NO_THROW(run_jit_compute_column());
EXPECT_NO_THROW(cudf::teardown());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TEST_F(ContextTest, TeardownAfterJitCacheUse)
{
  auto compute_column= []{
    auto c_0        = cudf::test::fixed_width_column_wrapper<cudf::size_type>{3, 20, 1, 50};
    auto c_1        = cudf::test::fixed_width_column_wrapper<cudf::size_type>{10, 7, 20, 0};
    auto table      = cudf::table_view{{c_0, c_1}};
    auto col_ref_0  = cudf::ast::column_reference(0);
    auto col_ref_1  = cudf::ast::column_reference(1);
    auto expression = cudf::ast::operation(cudf::ast::ast_operator::ADD, col_ref_0, col_ref_1);

    auto result = cudf::compute_column_jit(table, expression);
    EXPECT_EQ(result->size(), cudf::size_type{4});
  };

  cudf::initialize(cudf::init_flags::INIT_JIT_CACHE);
  ASSERT_NO_THROW(compute_column());
  EXPECT_NO_THROW(cudf::teardown());

  cudf::initialize(cudf::init_flags::INIT_JIT_CACHE);
  ASSERT_NO_THROW(compute_column());
  EXPECT_NO_THROW(cudf::teardown());
}

@vyasr

vyasr commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

@fallintoplace can you address the open requests? Then this PR should be good to go.

@vyasr

vyasr commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/ok to test 62246a0

@vyasr

vyasr commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/merge

@vyasr

vyasr commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/ok to test a96996d

@rapids-bot
rapids-bot Bot merged commit 3ba965e into NVIDIA:main Jun 17, 2026
257 of 259 checks passed
Comment thread .clang-format
BreakStringLiterals: true
ColumnLimit: 100
CommentPragmas: '^ IWYU pragma:'
CommentPragmas: '(IWYU pragma:|SPDX-)'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar to #22118 (comment) but for clang-format. We'll address this in more detail next week.

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 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