diff options
author | Andrew Chow <github@achow101.com> | 2023-06-27 15:33:13 -0400 |
---|---|---|
committer | Andrew Chow <github@achow101.com> | 2023-06-27 15:42:51 -0400 |
commit | 5cce4d293e8065ddd69838c7279fa5b4ddcc2daa (patch) | |
tree | 2d23c394d5e712998a82911056997ae0695bb528 | |
parent | 7ee41217b3b3fe4d8b7eb4fd1d4577b9b33d466d (diff) | |
parent | bfb9291a8661fe5b26c14ed755cfa89d27c37110 (diff) |
Merge bitcoin/bitcoin#27334: util: implement `noexcept` move assignment & move ctor for `prevector`
bfb9291a8661fe5b26c14ed755cfa89d27c37110 util: implement prevector's move ctor & move assignment (Martin Leitner-Ankerl)
fffc86f49f4eeb811b8438bc1b7f8d9e05882c6f test: CScriptCheck is used a lot in std::vector, make sure that's efficient (Martin Leitner-Ankerl)
81f67977f543faca2dcc35846f73e2917375ae79 util: prevector's move ctor and move assignment is `noexcept` (Martin Leitner-Ankerl)
d380d2877ed45cf1e75a87d822b30e4e1e21e3d4 bench: Add benchmark for prevector usage in std::vector (Martin Leitner-Ankerl)
Pull request description:
`prevector`'s move assignment and move constructor were not `noexcept`, which makes it inefficient to use inside STL containers like `std::vector`. That's the case e.g. for `CScriptCheck`. This PR adds `noexcept`, and also implements the move assignment & ctor, which makes it quite a bit more efficient to use prevector in an std::vector.
The PR also adds a benchmark which grows an `std::vector` by adding `prevector` objects to it.
merge-base:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 6,440.29 | 155,272.42 | 0.2% | 40,713.01 | 20,473.84 | 1.989 | 7,132.01 | 0.2% | 0.44 | `PrevectorFillVectorDirectNontrivial`
| 3,213.19 | 311,217.35 | 0.7% | 35,373.01 | 10,214.07 | 3.463 | 6,945.00 | 0.2% | 0.43 | `PrevectorFillVectorDirectTrivial`
| 34,749.70 | 28,777.23 | 0.1% | 364,396.05 | 110,521.94 | 3.297 | 78,568.37 | 0.1% | 0.43 | `PrevectorFillVectorIndirectNontrivial`
| 32,535.05 | 30,736.09 | 0.4% | 353,823.31 | 103,464.53 | 3.420 | 79,871.80 | 0.2% | 0.40 | `PrevectorFillVectorIndirectTrivial`
util: prevector's move ctor and move assignment is `noexcept`:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 6,603.87 | 151,426.40 | 0.2% | 23,734.01 | 21,009.63 | 1.130 | 2,445.01 | 0.3% | 0.44 | `PrevectorFillVectorDirectNontrivial`
| 1,980.93 | 504,813.15 | 0.1% | 13,784.00 | 6,304.32 | 2.186 | 2,258.00 | 0.3% | 0.44 | `PrevectorFillVectorDirectTrivial`
| 19,110.54 | 52,327.15 | 0.1% | 139,816.41 | 51,987.72 | 2.689 | 28,512.18 | 0.1% | 0.43 | `PrevectorFillVectorIndirectNontrivial`
| 12,334.37 | 81,074.27 | 0.7% | 125,655.12 | 39,253.58 | 3.201 | 27,854.46 | 0.2% | 0.44 | `PrevectorFillVectorIndirectTrivial`
util: implement prevector's move ctor & move assignment
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 5,262.66 | 190,018.01 | 0.2% | 20,157.01 | 16,745.26 | 1.204 | 2,445.01 | 0.3% | 0.44 | `PrevectorFillVectorDirectNontrivial`
| 1,687.07 | 592,744.35 | 0.2% | 12,742.00 | 5,368.02 | 2.374 | 2,258.00 | 0.3% | 0.44 | `PrevectorFillVectorDirectTrivial`
| 17,930.80 | 55,769.95 | 0.1% | 136,237.69 | 47,903.31 | 2.844 | 28,512.02 | 0.2% | 0.42 | `PrevectorFillVectorIndirectNontrivial`
| 11,893.75 | 84,077.78 | 0.2% | 126,182.02 | 37,852.91 | 3.333 | 28,152.01 | 0.1% | 0.44 | `PrevectorFillVectorIndirectTrivial`
As can be seen, mostly thanks to just `noexcept` the benchmark becomes about 2 times faster because `std::vector` can now use move operations instead of having to fall back to copy everything
I had a look at how this change affects the other benchmarks, and they are all pretty much the same, the only noticable difference is `CCheckQueueSpeedPrevectorJob` goes from 364.56ns down to 346.21ns.
ACKs for top commit:
achow101:
ACK bfb9291a8661fe5b26c14ed755cfa89d27c37110
jonatack:
> Tested Approach ACK [bfb9291](https://github.com/bitcoin/bitcoin/commit/bfb9291a8661fe5b26c14ed755cfa89d27c37110), ~ACK modulo re-verifying the implementation of the last commit.
john-moffett:
Approach ACK bfb9291a8661fe5b26c14ed755cfa89d27c37110
theStack:
Tested and light code-review ACK bfb9291a8661fe5b26c14ed755cfa89d27c37110
Tree-SHA512: 242995d7cb2f8ebfa73177aa690a505f189d91edeb8cea3e34a41ca507c8cb17c65fe2a4e196fdafc5c6e89b2b2222627bfc9f5c316517de0857b7b5e9c58225
-rw-r--r-- | src/bench/prevector.cpp | 26 | ||||
-rw-r--r-- | src/prevector.h | 15 | ||||
-rw-r--r-- | src/validation.h | 6 |
3 files changed, 43 insertions, 4 deletions
diff --git a/src/bench/prevector.cpp b/src/bench/prevector.cpp index 59c4af086e..2524e215e4 100644 --- a/src/bench/prevector.cpp +++ b/src/bench/prevector.cpp @@ -80,6 +80,30 @@ static void PrevectorDeserialize(benchmark::Bench& bench) }); } +template <typename T> +static void PrevectorFillVectorDirect(benchmark::Bench& bench) +{ + bench.run([&] { + std::vector<prevector<28, T>> vec; + for (size_t i = 0; i < 260; ++i) { + vec.emplace_back(); + } + }); +} + + +template <typename T> +static void PrevectorFillVectorIndirect(benchmark::Bench& bench) +{ + bench.run([&] { + std::vector<prevector<28, T>> vec; + for (size_t i = 0; i < 260; ++i) { + // force allocation + vec.emplace_back(29, T{}); + } + }); +} + #define PREVECTOR_TEST(name) \ static void Prevector##name##Nontrivial(benchmark::Bench& bench) \ { \ @@ -96,3 +120,5 @@ PREVECTOR_TEST(Clear) PREVECTOR_TEST(Destructor) PREVECTOR_TEST(Resize) PREVECTOR_TEST(Deserialize) +PREVECTOR_TEST(FillVectorDirect) +PREVECTOR_TEST(FillVectorIndirect) diff --git a/src/prevector.h b/src/prevector.h index f36cfe4ff6..bcab1ff00c 100644 --- a/src/prevector.h +++ b/src/prevector.h @@ -264,8 +264,10 @@ public: fill(item_ptr(0), other.begin(), other.end()); } - prevector(prevector<N, T, Size, Diff>&& other) { - swap(other); + prevector(prevector<N, T, Size, Diff>&& other) noexcept + : _union(std::move(other._union)), _size(other._size) + { + other._size = 0; } prevector& operator=(const prevector<N, T, Size, Diff>& other) { @@ -276,8 +278,13 @@ public: return *this; } - prevector& operator=(prevector<N, T, Size, Diff>&& other) { - swap(other); + prevector& operator=(prevector<N, T, Size, Diff>&& other) noexcept { + if (!is_direct()) { + free(_union.indirect_contents.indirect); + } + _union = std::move(other._union); + _size = other._size; + other._size = 0; return *this; } diff --git a/src/validation.h b/src/validation.h index 7b215dea6b..8bc8842c54 100644 --- a/src/validation.h +++ b/src/validation.h @@ -43,6 +43,7 @@ #include <stdint.h> #include <string> #include <thread> +#include <type_traits> #include <utility> #include <vector> @@ -330,6 +331,11 @@ public: ScriptError GetScriptError() const { return error; } }; +// CScriptCheck is used a lot in std::vector, make sure that's efficient +static_assert(std::is_nothrow_move_assignable_v<CScriptCheck>); +static_assert(std::is_nothrow_move_constructible_v<CScriptCheck>); +static_assert(std::is_nothrow_destructible_v<CScriptCheck>); + /** Initializes the script-execution cache */ [[nodiscard]] bool InitScriptExecutionCache(size_t max_size_bytes); |