Workaround nvcc compiler hangs in libcudf debug build - #22675
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR applies conditional inlining control to two device operator functions. The ChangesDevice operator inlining control for debug vs release builds
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
|
Here is a snippet of Claude's analysis of the build hanging The hang was introduced by commit 962d15b (PR #21924, "Add streaming_groupby for stateful streaming aggregation"). It affects both The trigger is
That body gets inlined into |
|
In the same session, I also asked Claude to analyze why the
Why if (is_outer_join) {
retrieve_matches<true>(...); // __forceinline__
} else {
retrieve_matches<false>(...); // __forceinline__
}
It's the same root cause family as |
There was a problem hiding this comment.
@davidwendt Thanks for the fix. Good news is that we will replace mixed join internals with normal hash join and filter_join_indicies in this release so the mixed join kernels will be gone (see #22124).
|
@PointKernel I would be curious if the |
|
👍 We can check the nightly benchmark once it's merged. |
The nightly benchmarks are run on release code and not debug built code. So this change would have to be measured locally or by the nightly run on a draft PR that removes the |
|
/merge |
./GROUPBY_NVBENCH -b 5 -a api=streamingvoila |
|
@PointKernel Here is results from that benchmark on my local machine using (removed the columns which did not change value: I32, 20000000, streaming) There appears to be no regression using |
|
Thank you for double checking the performance impact. That aligns with my expectations, since the overall workflow is fairly complex and whether the row comparator is inlined or not likely won’t have a noticeable effect on performance. Consistently marking it as |
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. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Bradley Dice (https://github.com/bdice) - Muhammad Haseeb (https://github.com/mhaseeb123) - Yunsong Wang (https://github.com/PointKernel) URL: #22699
Description
Adds
noinlinedeclaration to select functor operators that normally inline a significant amount code. Otherwise the compiler will appear to hang (run for many hours) trying to process and generate the ptx.Recent changes appear to have pushed the size of the inlined code beyond some internal boundary.
Adding the
noinlineoption may reduce the overall runtime for functions that use these utilities but only for a debug build and not for a release build where performance is required.Checklist