From 8c22b7b779e8b83492bef84779c83c8d81565b1a Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Wed, 12 Oct 2022 13:33:48 -0700 Subject: [PATCH] Performance improvements and benchmark tweaks --- fdbclient/IdempotencyId.cpp | 4 +-- .../fdbclient/BuildIdempotencyIdMutations.h | 24 ++++++++--------- fdbclient/include/fdbclient/IdempotencyId.h | 6 ++--- flowbench/BenchIdempotencyIds.cpp | 27 ++++++++++++++----- 4 files changed, 37 insertions(+), 24 deletions(-) diff --git a/fdbclient/IdempotencyId.cpp b/fdbclient/IdempotencyId.cpp index adfacb3cbe2..69f4a9a1366 100644 --- a/fdbclient/IdempotencyId.cpp +++ b/fdbclient/IdempotencyId.cpp @@ -50,7 +50,6 @@ void IdempotencyIdKVBuilder::add(const IdempotencyIdRef& id, uint16_t batchIndex Optional IdempotencyIdKVBuilder::buildAndClear() { ASSERT(impl->commitVersion.present()); if (!impl->batchIndexHighOrderByte.present()) { - *impl = IdempotencyIdKVBuilderImpl{}; return {}; } @@ -61,7 +60,8 @@ Optional IdempotencyIdKVBuilder::buildAndClear() { Value v = impl->value.toValue(); - *impl = IdempotencyIdKVBuilderImpl{}; + impl->value = BinaryWriter(IncludeVersion()); + impl->batchIndexHighOrderByte = Optional(); Optional result = KeyValue(); result.get().arena() = v.arena(); diff --git a/fdbclient/include/fdbclient/BuildIdempotencyIdMutations.h b/fdbclient/include/fdbclient/BuildIdempotencyIdMutations.h index 3735d2702ca..73ee0d57fb0 100644 --- a/fdbclient/include/fdbclient/BuildIdempotencyIdMutations.h +++ b/fdbclient/include/fdbclient/BuildIdempotencyIdMutations.h @@ -36,22 +36,22 @@ void buildIdempotencyIdMutations(const std::vector& tr uint8_t committedValue, bool locked, const OnKVReady& onKvReady) { + idempotencyKVBuilder.setCommitVersion(commitVersion); for (int h = 0; h < trs.size(); h += 256) { - idempotencyKVBuilder.setCommitVersion(commitVersion); - for (int l = 0; h + l < trs.size() && l < 256; ++l) { + int end = std::min(trs.size() - h, 256); + for (int l = 0; l < end; ++l) { uint16_t batchIndex = h + l; - if (!(committed[batchIndex] == committedValue && (!locked || trs[batchIndex].isLockAware()))) { - continue; - } - const auto& idempotency_id = trs[batchIndex].idempotencyId; - if (idempotency_id.valid()) { - idempotencyKVBuilder.add(idempotency_id, batchIndex); - } - Optional kv = idempotencyKVBuilder.buildAndClear(); - if (kv.present()) { - onKvReady(kv.get()); + if ((committed[batchIndex] == committedValue && (!locked || trs[batchIndex].isLockAware()))) { + const auto& idempotency_id = trs[batchIndex].idempotencyId; + if (idempotency_id.valid()) { + idempotencyKVBuilder.add(idempotency_id, batchIndex); + } } } + Optional kv = idempotencyKVBuilder.buildAndClear(); + if (kv.present()) { + onKvReady(kv.get()); + } } } diff --git a/fdbclient/include/fdbclient/IdempotencyId.h b/fdbclient/include/fdbclient/IdempotencyId.h index c21cf47b404..19a66a897ef 100644 --- a/fdbclient/include/fdbclient/IdempotencyId.h +++ b/fdbclient/include/fdbclient/IdempotencyId.h @@ -148,10 +148,10 @@ static_assert(sizeof(IdempotencyIdRef) == 16); struct IdempotencyIdKVBuilder : NonCopyable { IdempotencyIdKVBuilder(); void setCommitVersion(Version commitVersion); - // All calls to add must share the same high order byte of batchIndex + // All calls to add must share the same high order byte of batchIndex (until the next call to buildAndClear) void add(const IdempotencyIdRef& id, uint16_t batchIndex); - // Must call setCommitVersion before calling buildAndClear. After calling buildAndClear, this object is in the same - // state as if it were just default constructed. + // Must call setCommitVersion before calling buildAndClear. After calling buildAndClear, this object is ready to + // start a new kv pair for the high order byte of batchIndex. Optional buildAndClear(); ~IdempotencyIdKVBuilder(); diff --git a/flowbench/BenchIdempotencyIds.cpp b/flowbench/BenchIdempotencyIds.cpp index e9b9ebeda33..8a4f3824501 100644 --- a/flowbench/BenchIdempotencyIds.cpp +++ b/flowbench/BenchIdempotencyIds.cpp @@ -18,13 +18,18 @@ * limitations under the License. */ - #include "benchmark/benchmark.h" #include "fdbclient/BuildIdempotencyIdMutations.h" -static void bench_add_idempotency_ids_absent(benchmark::State& state) { +// We don't want the compiler to know that this is always false. It is though. +static bool getRuntimeFalse() { + return deterministicRandom()->randomInt(0, 2) == -1; +} + +static void bench_add_idempotency_ids(benchmark::State& state) { auto numTransactions = state.range(0); + auto idSize = state.range(1); auto trs = std::vector(numTransactions); IdempotencyIdKVBuilder idempotencyKVBuilder; Version commitVersion = 0; @@ -33,14 +38,22 @@ static void bench_add_idempotency_ids_absent(benchmark::State& state) { for (auto& c : committed) { c = deterministicRandom()->coinflip() ? committedValue : 0; } - // Don't want the compiler to know the value of locked, but it's always false in this benchmark - bool locked = deterministicRandom()->randomInt(0, 2) == -1; + for (auto& tr : trs) { + if (idSize > 0) { + auto id = makeString(idSize, tr.arena); + deterministicRandom()->randomBytes(mutateString(id), idSize); + tr.idempotencyId = IdempotencyIdRef(tr.arena, IdempotencyIdRef(id)); + } + } + bool locked = getRuntimeFalse(); for (auto _ : state) { buildIdempotencyIdMutations( - trs, idempotencyKVBuilder, commitVersion++, committed, committedValue, locked, []() { - ASSERT(false); // Shouldn't be called since there are not valid idempotency ids in this benchmark + trs, idempotencyKVBuilder, commitVersion++, committed, committedValue, locked, [](const KeyValue&) { + benchmark::DoNotOptimize(0); }); } + state.counters["TimePerTransaction"] = benchmark::Counter( + state.iterations() * numTransactions, benchmark::Counter::kIsRate | benchmark::Counter::kInvert); } -BENCHMARK(bench_add_idempotency_ids_absent)->Ranges({ { 1, 1 << 15 } }); +BENCHMARK(bench_add_idempotency_ids)->ArgsProduct({ benchmark::CreateRange(1, 16384, 4), { 0, 16, 255 } });