Performance improvements and benchmark tweaks

fdbclient/IdempotencyId.cpp

@@ -50,7 +50,6 @@ void IdempotencyIdKVBuilder::add(const IdempotencyIdRef& id, uint16_t batchIndex
 Optional<KeyValue> IdempotencyIdKVBuilder::buildAndClear() {
 	ASSERT(impl->commitVersion.present());
 	if (!impl->batchIndexHighOrderByte.present()) {
-		*impl = IdempotencyIdKVBuilderImpl{};
 		return {};
 	}
 
@@ -61,7 +60,8 @@ Optional<KeyValue> IdempotencyIdKVBuilder::buildAndClear() {
 
 	Value v = impl->value.toValue();
 
-	*impl = IdempotencyIdKVBuilderImpl{};
+	impl->value = BinaryWriter(IncludeVersion());
+	impl->batchIndexHighOrderByte = Optional<uint8_t>();
 
 	Optional<KeyValue> result = KeyValue();
 	result.get().arena() = v.arena();

fdbclient/include/fdbclient/BuildIdempotencyIdMutations.h

@@ -36,22 +36,22 @@ void buildIdempotencyIdMutations(const std::vector<CommitTransactionRequest>& 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<int>(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<KeyValue> 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<KeyValue> kv = idempotencyKVBuilder.buildAndClear();
+		if (kv.present()) {
+			onKvReady(kv.get());
+		}
 	}
 }
 

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<KeyValue> buildAndClear();
 
 	~IdempotencyIdKVBuilder();

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<CommitTransactionRequest>(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 } });