[libcu++] Optimize tuple and pair constraint checks - #10719
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesTuple and pair APIs now use centralized constraint objects for construction, assignment, comparison, deletion, implicitness, and Tuple and pair constraint refactor
Suggested reviewers: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
libcudacxx/include/cuda/std/__type_traits/sfinae_traits.h (1)
37-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Rename these two class templates to use the
__prefix, for example__constructor_constraintand__assignment_constraint. The single-underscore-uppercase form is reserved for macros and template parameters in this codebase. Also_Trait == truecan be just_Trait.As per coding guidelines: "Use C++ reserved identifiers for all non-public symbols:
_for macros and template parameters,__for all other symbols".Source: Coding guidelines
libcudacxx/include/cuda/std/__tuple_dir/tuple_constraints.h (2)
211-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: The
sizeof...(_Types) == 0branch is unreachable. The first branch already returns for every case wheresizeof...(_Types) != 1. Remove the second branch and keep its comment on the first one.Proposed change
if constexpr (sizeof...(_Types) != 1) { // [tuple.cnstr]-13.1: sizeof...(Types) equals sizeof...(UTypes), - return __select_constructor::__invalid; - } - else if constexpr (sizeof...(_Types) == 0) - { // [tuple.cnstr]-13.2: sizeof...(Types) >= 1, + // [tuple.cnstr]-13.2: sizeof...(Types) >= 1, return __select_constructor::__invalid; }
123-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Apply the same deferred-instantiation pattern to
__select_variadic_move_constructible: addtemplate <int = 0>, or remove it from the other two helpers if it is not required. Document any compiler-specific exception.libcudacxx/include/cuda/std/__tuple_dir/tuple.h (1)
292-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Drop
_CCCL_APIfrom this deleted constructor. All the other deleted overloads in this file (lines 269, 314, 368) omit it.libcudacxx/include/cuda/std/__utility/pair.h (1)
157-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: The comments in the non-const branch quote
const T1&andconst T2&, but the code checks_T1&and_T2&. Update the comment text to match [pairs.pair]-42.3 and 42.4.libcudacxx/include/cuda/std/__tuple_dir/tuple_leaf.h (1)
412-413: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winsuggestion: Make
__default_construction,__variadic_copy_construction, and__variadic_move_constructionalias templates, like__variadic_construction, and update their uses to instantiate them only when needed. Completing__tuple_constraints<_Tp...>currently evaluates all three_CCCL_CONSTEVALselectors for every tuple.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c40a49d-146b-4073-80a6-837e2b6a4eb1
📒 Files selected for processing (5)
libcudacxx/include/cuda/std/__tuple_dir/tuple.hlibcudacxx/include/cuda/std/__tuple_dir/tuple_constraints.hlibcudacxx/include/cuda/std/__tuple_dir/tuple_leaf.hlibcudacxx/include/cuda/std/__type_traits/sfinae_traits.hlibcudacxx/include/cuda/std/__utility/pair.h
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
griwes
left a comment
griwes
left a comment
There was a problem hiding this comment.
Overall looks good, and this is some nice non-immediately-obvious level of laziness, but the compile time bench numbers, even on just the public headers, speak for themselves. Both me and codex found some, both new and preexisting, constness woes though, plus some overly eagerly evaluated fold expressions.
|
|
||
| // Comparisons | ||
| template <class... _UTypes> | ||
| static constexpr bool __is_equality_comparable_v = (__is_cpp17_equality_comparable_v<_Types, _UTypes> && ...); |
There was a problem hiding this comment.
I was a bit concerned about the constness here (I guess a knee-jerk reaction to seeing const qualified member functions that are using this) after the assignment operators, so I had codex look at this for me, aaaaaand looks like the issue is (1) backwards from what I thought (too much is constified, not too little), and (2) preexisting. Apparently by going through the __is_cpp17_blah_comparable_v family here, we eventually run through __make_const_lvalue_ref, which turns everything into const T &... ...except for tuple<int &>, we now check for comparisons with const int & instead of int & (since we aren't supposed to deep const the references). Soooo this isn't new to this PR, but it is adjacent to what you're doing. Would you prefer to just drive-by fix this, or should I file an issue for this problem?
There was a problem hiding this comment.
I am not sure, whether there is an actual issue here.
The only situation where this makes a difference is with plain references. But we can compare those in any case. So is there a situation where it would actually return the wrong result?
9f2db78 to
0bddb9e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This comment has been minimized.
This comment has been minimized.
0bddb9e to
f21a18e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f21a18e to
166f992
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
166f992 to
ba3f184
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libcudacxx/test/libcudacxx/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_const_move.pass.cpp (1)
117-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winimportant: Add a GCC-specific comment above the guard. State the failure mode and the evidence for the
< 12boundary. Do not describe GCC 12 as the first supported version without that evidence.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b444c26-c768-433c-8e20-e7d14429fe12
📒 Files selected for processing (5)
libcudacxx/include/cuda/std/__cccl/builtin.hlibcudacxx/include/cuda/std/__tuple_dir/tuple.hlibcudacxx/include/cuda/std/__tuple_dir/tuple_constraints.hlibcudacxx/test/libcudacxx/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_const_move.pass.cpplibcudacxx/test/libcudacxx/std/utilities/tuple/tuple.tuple/tuple.cnstr/deduct.pass.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- libcudacxx/include/cuda/std/__tuple_dir/tuple.h
- libcudacxx/include/cuda/std/__tuple_dir/tuple_constraints.h
This comment has been minimized.
This comment has been minimized.
ba3f184 to
b71a42e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6fcf659 to
e2eca4f
Compare
This comment has been minimized.
This comment has been minimized.
b194cda to
a88fd16
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a88fd16 to
0c165d4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I did a round of compile-time comparisons against thrust and it looks really promising: rank,event_name,event_key,baseline_impact_s,current_impact_s,impact_delta_s,impact_magnitude_s,baseline_selected_s,current_selected_s,selected_delta_s,selected_magnitude_s,baseline_total_inclusive_s,current_total_inclusive_s,baseline_total_exclusive_s,current_total_exclusive_s,baseline_event_count,current_event_count,matched_trace_count
1,Total Compilation Time,thrust/set_operations.h,98.379904,73.661442,-24.718462,24.718462,98.379904,73.661442,-24.718462,24.718462,98.379904,73.661442,98.379904,73.661442,4,4,4
2,Total Compilation Time,thrust/mr/sync_pool.h,97.703798,75.402452,-22.301346,22.301346,97.703798,75.402452,-22.301346,22.301346,97.703798,75.402452,97.703798,75.402452,4,4,4
3,Total Compilation Time,thrust/sort.h,100.825112,79.366195,-21.458917,21.458917,100.825112,79.366195,-21.458917,21.458917,100.825112,79.366195,100.825112,79.366195,4,4,4
4,Total Compilation Time,thrust/transform_reduce.h,91.747395,70.650480,-21.096915,21.096915,91.747395,70.650480,-21.096915,21.096915,91.747395,70.650480,91.747395,70.650480,4,4,4
5,Total Compilation Time,thrust/mr/tls_pool.h,97.562636,76.619570,-20.943066,20.943066,97.562636,76.619570,-20.943066,20.943066,97.562636,76.619570,97.562636,76.619570,4,4,4
6,Total Compilation Time,thrust/scan.h,82.655272,63.046243,-19.609029,19.609029,82.655272,63.046243,-19.609029,19.609029,82.655272,63.046243,82.655272,63.046243,4,4,4
7,Total Compilation Time,thrust/device_make_unique.h,91.685886,72.201465,-19.484421,19.484421,91.685886,72.201465,-19.484421,19.484421,91.685886,72.201465,91.685886,72.201465,4,4,4
8,Total Compilation Time,thrust/logical.h,96.659350,77.366428,-19.292922,19.292922,96.659350,77.366428,-19.292922,19.292922,96.659350,77.366428,96.659350,77.366428,4,4,4
9,Total Compilation Time,thrust/unique.h,91.149183,72.022348,-19.126835,19.126835,91.149183,72.022348,-19.126835,19.126835,91.149183,72.022348,91.149183,72.022348,4,4,4
10,Total Compilation Time,thrust/equal.h,94.891331,75.764761,-19.126570,19.126570,94.891331,75.764761,-19.126570,19.126570,94.891331,75.764761,94.891331,75.764761,4,4,4
11,Total Compilation Time,thrust/inner_product.h,91.049666,72.263369,-18.786297,18.786297,91.049666,72.263369,-18.786297,18.786297,91.049666,72.263369,91.049666,72.263369,4,4,4
12,Total Compilation Time,thrust/device_new_allocator.h,90.597483,71.940949,-18.656534,18.656534,90.597483,71.940949,-18.656534,18.656534,90.597483,71.940949,90.597483,71.940949,4,4,4
13,Total Compilation Time,thrust/system/cuda/vector.h,90.766537,72.116559,-18.649978,18.649978,90.766537,72.116559,-18.649978,18.649978,90.766537,72.116559,90.766537,72.116559,4,4,4
14,Total Compilation Time,thrust/remove.h,86.561351,68.270543,-18.290808,18.290808,86.561351,68.270543,-18.290808,18.290808,86.561351,68.270543,86.561351,68.270543,4,4,4
15,Total Compilation Time,thrust/device_new.h,93.483493,76.021011,-17.462482,17.462482,93.483493,76.021011,-17.462482,17.462482,93.483493,76.021011,93.483493,76.021011,4,4,4
|
It seems the current approach with the constraints is a bit costly Try to go with a single struct per type and use type aliases in the SFINAE expressions to hopefully defer instantiation
0c165d4 to
754f29e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
754f29e to
4e02fff
Compare
⏱️ CCCL compile-time benchmark comparison: Public headers compile-time benchResult: 0 regression row(s), 2 improvement row(s) above threshold.
Artifacts: reports and traces Direct file processing
🟢 Direct file processing — Improvements
|
🥳 CI Workflow Results🟩 Finished in 22h 57m: Pass: 100%/117 | Total: 3d 02h | Max: 1h 35m | Hits: 61%/607591See results here. |
2 participants
It seems the current approach with the constraints is a bit costly
Try to go with a single struct per type and use type aliases in the SFINAE expressions to hopefully defer instantiation