Prefer defaulted default constructor for Bitset #6524
Conversation
The default argument to the constructor that takes the size of the bitset was deferring to another constructor that creates an empty view with a label argument. This alocates 128 bits for the view header. This showed when constructing an UnorderedMap with a pointless 128-bit "header-only" allocation which implies an unnecessary fence.
dalg24
added
Performance
|
Fixes #6467. |
@masterleinad It fixes part of #6467 I think. What about #6467 (comment) ? |
| /// arg_size := number of bit in set | ||
| Bitset(unsigned arg_size = 0u) : Bitset(Kokkos::view_alloc(), arg_size) {} | ||
| Bitset(unsigned arg_size) : Bitset(Kokkos::view_alloc(), arg_size) {} |
There was a problem hiding this comment.
| Bitset(unsigned arg_size) : Bitset(Kokkos::view_alloc(), arg_size) {} | |
| explicit Bitset(unsigned arg_size) : Bitset(Kokkos::view_alloc(), arg_size) {} |
There was a problem hiding this comment.
I tend to agree with the suggested change but I will note that it was not explicit before and that is besides the scope of this PR.
That said I will apply the suggested change if other reviewers would like me to.
There was a problem hiding this comment.
May need to do this with a "deprecated" guard.
That's a separate discussion and isn't reflected in the title or description of the pull request and serves as a motivation. Open a separate issue for it if you want to discuss it in a larger context. |
Thanks for pointed that out. I will admit I had overlooked it :/ |
|
@dal24 @masterleinad Would you like me to open a new issue with #6467 (comment) ? |
Please do so |
Just did, it is #6526. Please tell me if the title is OK with you 😉 |
which was anticipated in #6467 (comment). |
|
SYCL build timed out but I intend to ignore if I can get a 2nd approval |
dalg24
force-pushed
the
bitset_defaulted_default_constructor
branch
from
94c533c
to
7cd6aa8
Compare
containers/unit_tests/TestBitset.hpp
Outdated
| auto success = validate_existence( | ||
| [&]() { | ||
| Kokkos::Bitset bs1(0); | ||
| EXPECT_TRUE(bs1.is_allocated()); | ||
|
|
||
| Kokkos::Bitset bs2(Kokkos::view_alloc("MyBitset"), 0); | ||
| EXPECT_TRUE(bs2.is_allocated()); | ||
| }, | ||
| [&](AllocateDataEvent) { | ||
| return MatchDiagnostic{true, {"Found alloc event"}}; | ||
| }); | ||
| ASSERT_TRUE(success); |
There was a problem hiding this comment.
Shouldn't this test be done in 2 distinct validate_existence ? If I understand it correctly, in its current state, the first allocation bs1(0) could in fact not allocate anything and the test would still pass if bs2 allocates something?
Not requesting any change though, I just wanted to make sure I understand validate_existence 😄
BTW is there any helper in the Kokkos testing toolbox that would not only allow us to ensure that some event occurred (in this case AllocateDataEvent) but also to count them ?
There was a problem hiding this comment.
You are correct and that is how I had written the test initially but I didn't think it was worth the boiler plate code.
I don't know that there is currently something to count the number of allocations even though writing directly a tool that does that would be trivial.
|
Three of the CUDA builds seg faulted during compilation. The CUDA 12 build was fine. |
|
Retest this please |
The default argument to the constructor that takes the size of the bitset was deferring to another constructor that creates an empty view with a label argument. This alocates 128 bits for the view header. This showed when constructing an UnorderedMap with a pointless 128-bit "header-only" allocation which implies an unnecessary fence.
cc @uliegecsm @romintomasetti