Makes arith_uint256/base_uint trivially copyable by removing the custom copy constructor and copy assignment operators. Removing of the custom code should not result in a change of behavior since base_uint contains a simple array of uint32_t and compiler generated versions of the code could be better optimized.
This was suggested by maflcko here: #30469#pullrequestreview-3186533494
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33332.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Raimo33, l0rinc, hodlinator, achow101 |
| Stale ACK | TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
36@@ -37,21 +37,6 @@ class base_uint
37 pn[i] = 0;
38 }
39
40- base_uint(const base_uint& b)
0 base_uint(const base_uint& b) = default;
1 base_uint& operator=(const base_uint& b) = default;
?
Concept ACK the compiler can now generate constexpr constructors and better optimize.
Approach NACK
I think it’s more explicit to use the = default syntax.
Also a test needs to be added. a simple static_assert as mentioned by others.
ACK 6ffc6d1140a4660e3e2a8717128d567f5557e394
Verified through:
0static_assert(std::is_trivially_copyable_v<arith_uint256>);
-> Only succeeds with PR.
49- for (int i = 0; i < WIDTH; i++)
50- pn[i] = b.pn[i];
51- }
52- return *this;
53- }
54+ base_uint(const base_uint& b) = default;
noexcept already, right? But we’ve added these lines exactly to be explicit, so I’d say we either add them or remove them completely
It is implicitly now could not be in the future if there are changes made to the class. So it might be better to let the compiler choose noexcept conditionally based on what the class looks like in the future.
I would be fine with removing the default declarations like in the original version of the PR but it seems to me so far more reviewers were in favor of them, so keeping them for now.
If others reviewers are convinced by @l0rinc ’s arguments and now in favor of removing, please comment!
50- pn[i] = b.pn[i];
51- }
52- return *this;
53- }
54+ base_uint(const base_uint& b) = default;
55+ base_uint& operator=(const base_uint& b) = default;
we’ve change the code and no tests were updated, which signals to me that our coverage is lacking: the CI won’t complain when the current change is invalidated in any way.
We have an arith_uint256_tests already, we could add a simple test there that proves why we need this change (which fails before the change and passes after):
0diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
1--- a/src/test/arith_uint256_tests.cpp (revision 77c120d6bf1c214b30992ee5723c063d6fb638f9)
2+++ b/src/test/arith_uint256_tests.cpp (date 1757362769804)
3@@ -65,6 +65,8 @@
4
5 BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
6 {
7+ static_assert(std::is_trivially_copyable_v<arith_uint256>);
8+
9 BOOST_CHECK(1 == 0+1);
10 // constructor arith_uint256(vector<char>):
11 BOOST_CHECK(R1L.ToString() == ArrayToString(R1Array,32));
It would also prove that the original version would pass the tests:
0diff --git a/src/arith_uint256.h b/src/arith_uint256.h
1--- a/src/arith_uint256.h (revision 3eea9fd39532eeda648e44de365fc4c9112f6fc6)
2+++ b/src/arith_uint256.h (date 1757363002301)
3@@ -37,21 +37,6 @@
4 pn[i] = 0;
5 }
6
7- base_uint(const base_uint& b)
8- {
9- for (int i = 0; i < WIDTH; i++)
10- pn[i] = b.pn[i];
11- }
12-
13- base_uint& operator=(const base_uint& b)
14- {
15- if (this != &b) {
16- for (int i = 0; i < WIDTH; i++)
17- pn[i] = b.pn[i];
18- }
19- return *this;
20- }
21-
22 base_uint(uint64_t b)
23 {
24 pn[0] = (unsigned int)b;
I don’t mind the explicit one suggested in #33332 (review), but I personally don’t see why it’s better, as long as we have a test that already guarantees what we’re after (note: the PR description still mentions the removal).
I don’t insist on removing needless code since others seem to prefer it, but I do insist on adding test for this.
IMO it is not surprising that tests didn’t need to be updated because this is a refactor. For pure refactors that don’t change behavior it is normal to not add/edit tests.
Furthermore, trivially copyable is a compile-time structural property that affects ABI/optimization, not behavior. So we aren’t changing something that could change behavior in this PR. FWIW, these lines were covered by unit tests, see the coverage reports: https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/arith_uint256.h.gcov.html
Unit tests check behavior so if we want to add a static_assert, which is a compile time check and is unrelated to the unit test framework, then I think we would want to add it in the code directly, not to the unit tests. This means the check still is executed even if someone were to compile the code without the unit tests.
Generally adding compile-time checks for properties is not something we should do lightly IMO. It should be possible to do refactorings that don’t change behavior without changing a lot of such asserts. If we add static_assert for all kinds of properties all over the code base then maintenance will become more cumbersome. We do add static_assert in some rare cases like for example for CScriptCheck that are performance critical.
Personally, I don’t think that this meets the bar for adding such a check but since three reviewers are signaling support for this (comments or emoji) I have added the assert below the implementation.
43- pn[i] = b.pn[i];
44- }
45-
46- base_uint& operator=(const base_uint& b)
47- {
48- if (this != &b) {
Concept ACK, it makes sense to let the compiler generate code.
I’m not sure I agree with making these explicit, there’s a reason the compiler can also generate them now - and we should let it. We don’t have the move ctor/assignments and destructor either, seems like the Rule of Zero applies here: https://en.cppreference.com/w/cpp/language/rule_of_three.html#Rule_of_zero
But we would need to add a test to make the intention obvious - and maybe a benchmark, but I’m also fine with just providing the benchmark in a reproducible manner for reviewers.
I have created some simple micro benchmarks to test a few versions, the original with explicit loops, the first version you had with deleted methods, a version with the default assignments and one with an explicit `noexcept. I also experimented with adding move constructors
0diff --git a/src/bench/CMakeLists.txt b/src/bench/CMakeLists.txt
1--- a/src/bench/CMakeLists.txt (revision 3eea9fd39532eeda648e44de365fc4c9112f6fc6)
2+++ b/src/bench/CMakeLists.txt (date 1757363506278)
3@@ -11,6 +11,7 @@
4 base58.cpp
5 bech32.cpp
6 bip324_ecdh.cpp
7+ arith_uint256.cpp
8 block_assemble.cpp
9 blockencodings.cpp
10 ccoins_caching.cpp
11diff --git a/src/bench/arith_uint256.cpp b/src/bench/arith_uint256.cpp
12new file mode 100644
13--- /dev/null (date 1757366730590)
14+++ b/src/bench/arith_uint256.cpp (date 1757366730590)
15@@ -0,0 +1,53 @@
16+// src/bench/arith_uint256.cpp
17+#include <bench/bench.h>
18+#include <arith_uint256.h>
19+#include <random.h>
20+#include <uint256.h>
21+#include <vector>
22+
23+static void ArithUint256CopyConstruct(benchmark::Bench& bench)
24+{
25+ FastRandomContext rng{/*fDeterministic=*/true};
26+ const arith_uint256 source{UintToArith256(rng.rand256())};
27+
28+ bench.run([&] {
29+ arith_uint256 dest{source};
30+ ankerl::nanobench::doNotOptimizeAway(dest);
31+ });
32+}
33+
34+
35+static void ArithUint256CopyAssign(benchmark::Bench& bench)
36+{
37+ FastRandomContext rng{/*fDeterministic=*/true};
38+ const arith_uint256 source{UintToArith256(rng.rand256())};
39+ arith_uint256 dest{UintToArith256(rng.rand256())};
40+
41+ bench.run([&] {
42+ dest = source;
43+ ankerl::nanobench::doNotOptimizeAway(dest);
44+ });
45+}
46+
47+static void ArithUint256MoveCircle(benchmark::Bench& bench)
48+{
49+ FastRandomContext rng{
50+ /*fDeterministic=*/true};
51+ arith_uint256 a{UintToArith256(rng.rand256())};
52+ arith_uint256 b{UintToArith256(rng.rand256())};
53+ arith_uint256 c{UintToArith256(rng.rand256())};
54+
55+ bench.run([&] {
56+ arith_uint256 temp{std::move(a)};
57+ a = std::move(b);
58+ b = std::move(c);
59+ c = std::move(temp);
60+ ankerl::nanobench::doNotOptimizeAway(a);
61+ ankerl::nanobench::doNotOptimizeAway(b);
62+ ankerl::nanobench::doNotOptimizeAway(c);
63+ });
64+}
65+
66+BENCHMARK(ArithUint256CopyConstruct, benchmark::PriorityLevel::HIGH);
67+BENCHMARK(ArithUint256CopyAssign, benchmark::PriorityLevel::HIGH);
68+BENCHMARK(ArithUint256MoveCircle, benchmark::PriorityLevel::HIGH);
The benchmarks indicate that copy and move are sped up, assignment is the same as before (or maybe the benchmark is off).
0original:
1| ns/op | op/s | err% | total | benchmark
2|--------------------:|--------------------:|--------:|----------:|:----------
3| 1.63 | 614,001,425.31 | 0.2% | 10.99 | `ArithUint256CopyAssign`
4| 0.31 | 3,237,812,975.24 | 0.0% | 11.00 | `ArithUint256CopyConstruct`
5explicit:
6| ns/op | op/s | err% | total | benchmark
7|--------------------:|--------------------:|--------:|----------:|:----------
8| 0.35 | 2,842,958,663.82 | 0.0% | 11.01 | `ArithUint256CopyAssign`
9| 0.31 | 3,233,836,395.08 | 0.0% | 10.99 | `ArithUint256CopyConstruct`
10explicit + noexcept:
11| ns/op | op/s | err% | total | benchmark
12|--------------------:|--------------------:|--------:|----------:|:----------
13| 0.35 | 2,840,325,820.89 | 0.0% | 11.00 | `ArithUint256CopyAssign`
14| 0.31 | 3,229,023,239.94 | 0.0% | 11.01 | `ArithUint256CopyConstruct`
15deleted:
16| ns/op | op/s | err% | total | benchmark
17|--------------------:|--------------------:|--------:|----------:|:----------
18| 0.35 | 2,839,982,039.30 | 0.0% | 11.01 | `ArithUint256CopyAssign`
19| 0.31 | 3,231,208,395.96 | 0.0% | 10.99 | `ArithUint256CopyConstruct`
20
21
22------
23
24before:
25| ns/op | op/s | err% | total | benchmark
26|--------------------:|--------------------:|--------:|----------:|:----------
27| 4.09 | 244,356,175.43 | 0.1% | 11.02 | `ArithUint256MoveCircle`
28
29after:
30| ns/op | op/s | err% | total | benchmark
31|--------------------:|--------------------:|--------:|----------:|:----------
32| 2.83 | 353,350,683.29 | 0.1% | 10.99 | `ArithUint256MoveCircle`
Replacing the custom code with default behavior should not result in a change of behavior since base_uint contains a simple array of uint32_t and compiler generated versions of the code could be better optimized.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59
I’m fine with the change as it is currently, would prefer more implicit behavior instead (the original PR), but this is also better than before, thanks for doing it.
re-ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59
New push only adds static_assert with motivation.
The members of base_uint are all trivially copyable: WIDTH is a scalar, and pn is an array of scalars, so the class is trivially copyable and we should use the implicit copy constructors of the class. But, by explicitly declaring the implicitly defined copy constructors the move constructors are suppressed.
Implicitly-declared move constructor
If no user-defined move constructors are provided for a class type, and all of the following is true:
- there are no user-declared copy constructors;
- there are no user-declared copy assignment operators;
- there are no user-declared move assignment operators;
- there is no user-declared destructor.
Then the compiler will declare a move constructor as a non-explicit inline public member of its class with the signature T::T(T&&).
There are a few places where move constructors would be used, and where copy elision is not guaranteed, so it would be better to either remove the explicit declarations or to also declare move constructors.
Edit: Missed that this was merged, I can open a follow-up.
if I understand this correctly, move constructors were suppressed before this PR as well.
They were, so this is not a regression, but it would be better for the implicitly defined move constructors to be usable.
it would be better for the implicitly defined move constructors to be usable.
good catch, totally agree, let’s run some benchmarks
let’s run some benchmarks
I have already tested move construction in #33332 (review)
As I understand it, std::move() does not force the invocation of a move assignment / constructor, it casts its argument to an rvalue but if a constructor / assignment operator that takes an rvalue reference is not found, a function that takes an lvalue will be used, so in the ArithUint256MoveCircle benchmark, the copy assignment / constructor is actually being called in each use of std::move(), this can be demonstrated by adding logging to the copy functions. (https://godbolt.org/z/Y6c8qjo5s)
https://godbolt.org/z/5j7hEY8zs
0#include <iostream>
1
2class no_move
3{
4public:
5 int val{};
6
7 no_move(int x) : val(x) {}
8
9 no_move(const no_move& b) {
10 std::cout << "Copy Constructor called" << std::endl;
11 val = b.val;
12 }
13 no_move& operator=(const no_move& b) {
14 std::cout << "Copy assignment operator called" << std::endl;
15 val = b.val;
16 return *this;
17 }
18
19 /* if you uncomment these explicit declarations of the move constructor/assignment
20 they will be invoked.
21 */
22 // no_move(no_move&& b) = default;
23 // no_move& operator=(no_move&& b) = default;
24
25};
26
27int main(int argc, char** argv)
28{
29 no_move a{1};
30 no_move b{2};
31 no_move c{3};
32
33 no_move temp{std::move(a)};
34 a = std::move(b);
35 b = std::move(c);
36 c = std::move(temp);
37}
0Copy Constructor called
1Copy assignment operator called
2Copy assignment operator called
3Copy assignment operator called
But, on second thought, since base_uint is just trivially copyable data, i.e. there are no external resources to be moved, the move constructor won’t do anything differently than the copy constructor. There is even a clang-tidy rule that checks for this since it has no effect: https://clang.llvm.org/extra/clang-tidy/checks/performance/move-const-arg.html#cmdoption-arg-CheckTriviallyCopyableMove so this can be left as-is.
As-is I think the class kind of violates the rule of three (https://en.cppreference.com/w/cpp/language/rule_of_three.html), not a big deal but that could be fixed by making the constructor default, but probably not worth a follow-up. (e.g. https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5)
std::array as done in https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 since some debug builds in CI run with extra bounds-checking for std-types. Although maybe MSAN builds already catch out of bounds access for raw arrays, and I don’t suspect we would detect any new issues for base_uint/arith_uint256).
std::array is needed.