From 98bd30e87bd4f5c3418f3a1cbf227d3c160f5d72 Mon Sep 17 00:00:00 2001 From: pchintar <89355405+pchintar@users.noreply.github.com> Date: Sun, 24 May 2026 04:59:45 +0530 Subject: [PATCH 1/2] perf(fdbclient): reduce arena allocations in MutationRef deep-copy constructors --- .../include/fdbclient/CommitTransaction.h | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/fdbclient/include/fdbclient/CommitTransaction.h b/fdbclient/include/fdbclient/CommitTransaction.h index 66905c0ae25..af9638d720d 100644 --- a/fdbclient/include/fdbclient/CommitTransaction.h +++ b/fdbclient/include/fdbclient/CommitTransaction.h @@ -106,10 +106,38 @@ struct MutationRef { MutationRef() : type(MAX_ATOMIC_OP), corrupted(false) {} MutationRef(Type t, StringRef a, StringRef b) : type(t), param1(a), param2(b), corrupted(false) {} - MutationRef(Arena& to, Type t, StringRef a, StringRef b) - : type(t), param1(to, a), param2(to, b), corrupted(false) {} - MutationRef(Arena& to, const MutationRef& from) - : type(from.type), param1(to, from.param1), param2(to, from.param2), corrupted(false) {} + MutationRef(Arena& to, Type t, StringRef a, StringRef b) : type(t), corrupted(false) { + const int totalSize = a.size() + b.size(); + uint8_t* packed = totalSize ? new (to) uint8_t[totalSize] : nullptr; + + if (a.size()) { + memcpy(packed, a.begin(), a.size()); + } + + if (b.size()) { + memcpy(packed + a.size(), b.begin(), b.size()); + } + + param1 = StringRef(packed, a.size()); + param2 = StringRef(packed + a.size(), b.size()); + } + + MutationRef(Arena& to, const MutationRef& from) : type(from.type), corrupted(false) { + const int totalSize = from.param1.size() + from.param2.size(); + uint8_t* packed = totalSize ? new (to) uint8_t[totalSize] : nullptr; + + if (from.param1.size()) { + memcpy(packed, from.param1.begin(), from.param1.size()); + } + + if (from.param2.size()) { + memcpy(packed + from.param1.size(), from.param2.begin(), from.param2.size()); + } + + param1 = StringRef(packed, from.param1.size()); + param2 = StringRef(packed + from.param1.size(), from.param2.size()); + } + int totalSize() const { return OVERHEAD_BYTES + param1.size() + param2.size() + (checksum.present() ? sizeof(uint32_t) + 1 : 1) + (accumulativeChecksumIndex.present() ? sizeof(uint16_t) + 1 : 1); From 3d0286dd950c4926ec03a52a6e0fe3c62a436d87 Mon Sep 17 00:00:00 2001 From: pchintar <89355405+pchintar@users.noreply.github.com> Date: Sun, 24 May 2026 21:32:34 +0530 Subject: [PATCH 2/2] perf(kvstore): Reduce arena allocations for range read results --- .../include/fdbclient/CommitTransaction.h | 36 +++---------------- fdbclient/include/fdbclient/FDBTypes.h | 17 +++++++-- .../kvstore/KeyValueStoreRocksDB.actor.cpp | 16 +++++---- .../KeyValueStoreShardedRocksDB.actor.cpp | 16 +++++---- 4 files changed, 39 insertions(+), 46 deletions(-) diff --git a/fdbclient/include/fdbclient/CommitTransaction.h b/fdbclient/include/fdbclient/CommitTransaction.h index af9638d720d..66905c0ae25 100644 --- a/fdbclient/include/fdbclient/CommitTransaction.h +++ b/fdbclient/include/fdbclient/CommitTransaction.h @@ -106,38 +106,10 @@ struct MutationRef { MutationRef() : type(MAX_ATOMIC_OP), corrupted(false) {} MutationRef(Type t, StringRef a, StringRef b) : type(t), param1(a), param2(b), corrupted(false) {} - MutationRef(Arena& to, Type t, StringRef a, StringRef b) : type(t), corrupted(false) { - const int totalSize = a.size() + b.size(); - uint8_t* packed = totalSize ? new (to) uint8_t[totalSize] : nullptr; - - if (a.size()) { - memcpy(packed, a.begin(), a.size()); - } - - if (b.size()) { - memcpy(packed + a.size(), b.begin(), b.size()); - } - - param1 = StringRef(packed, a.size()); - param2 = StringRef(packed + a.size(), b.size()); - } - - MutationRef(Arena& to, const MutationRef& from) : type(from.type), corrupted(false) { - const int totalSize = from.param1.size() + from.param2.size(); - uint8_t* packed = totalSize ? new (to) uint8_t[totalSize] : nullptr; - - if (from.param1.size()) { - memcpy(packed, from.param1.begin(), from.param1.size()); - } - - if (from.param2.size()) { - memcpy(packed + from.param1.size(), from.param2.begin(), from.param2.size()); - } - - param1 = StringRef(packed, from.param1.size()); - param2 = StringRef(packed + from.param1.size(), from.param2.size()); - } - + MutationRef(Arena& to, Type t, StringRef a, StringRef b) + : type(t), param1(to, a), param2(to, b), corrupted(false) {} + MutationRef(Arena& to, const MutationRef& from) + : type(from.type), param1(to, from.param1), param2(to, from.param2), corrupted(false) {} int totalSize() const { return OVERHEAD_BYTES + param1.size() + param2.size() + (checksum.present() ? sizeof(uint32_t) + 1 : 1) + (accumulativeChecksumIndex.present() ? sizeof(uint16_t) + 1 : 1); diff --git a/fdbclient/include/fdbclient/FDBTypes.h b/fdbclient/include/fdbclient/FDBTypes.h index ea728b8f724..c8792a6b857 100644 --- a/fdbclient/include/fdbclient/FDBTypes.h +++ b/fdbclient/include/fdbclient/FDBTypes.h @@ -455,10 +455,23 @@ struct KeyValueRef { KeyRef key; ValueRef value; KeyValueRef() {} + KeyValueRef(const KeyRef& key, const ValueRef& value) : key(key), value(value) {} - KeyValueRef(Arena& a, const KeyValueRef& copyFrom) : key(a, copyFrom.key), value(a, copyFrom.value) {} + + KeyValueRef(Arena& a, const KeyRef& key, const ValueRef& value) { + StringRef storage = makeString(key.size() + value.size(), a); + uint8_t* dst = mutateString(storage); + + key.copyTo(dst); + value.copyTo(dst + key.size()); + + this->key = KeyRef(storage.begin(), key.size()); + this->value = ValueRef(storage.begin() + key.size(), value.size()); + } + + KeyValueRef(Arena& a, const KeyValueRef& copyFrom) : KeyValueRef(a, copyFrom.key, copyFrom.value) {} + bool operator==(const KeyValueRef& r) const { return key == r.key && value == r.value; } - bool operator!=(const KeyValueRef& r) const { return key != r.key || value != r.value; } int expectedSize() const { return key.expectedSize() + value.expectedSize(); } diff --git a/fdbserver/kvstore/KeyValueStoreRocksDB.actor.cpp b/fdbserver/kvstore/KeyValueStoreRocksDB.actor.cpp index 43619e1f456..da6b9c06160 100644 --- a/fdbserver/kvstore/KeyValueStoreRocksDB.actor.cpp +++ b/fdbserver/kvstore/KeyValueStoreRocksDB.actor.cpp @@ -1916,9 +1916,11 @@ struct RocksDBKeyValueStore : IKeyValueStore { auto cursor = readIter.iter; cursor->Seek(toSlice(a.keys.begin)); while (cursor->Valid() && toStringRef(cursor->key()) < a.keys.end) { - KeyValueRef kv(toStringRef(cursor->key()), toStringRef(cursor->value())); - accumulatedBytes += sizeof(KeyValueRef) + kv.expectedSize(); - result.push_back_deep(result.arena(), kv); + KeyRef key = toStringRef(cursor->key()); + ValueRef value = toStringRef(cursor->value()); + + accumulatedBytes += sizeof(KeyValueRef) + key.expectedSize() + value.expectedSize(); + result.emplace_back_deep(result.arena(), key, value); // Calling `cursor->Next()` is potentially expensive, so short-circut here just in case. if (result.size() >= a.rowLimit || accumulatedBytes >= a.byteLimit) { break; @@ -1949,9 +1951,11 @@ struct RocksDBKeyValueStore : IKeyValueStore { cursor->Prev(); } while (cursor->Valid() && toStringRef(cursor->key()) >= a.keys.begin) { - KeyValueRef kv(toStringRef(cursor->key()), toStringRef(cursor->value())); - accumulatedBytes += sizeof(KeyValueRef) + kv.expectedSize(); - result.push_back_deep(result.arena(), kv); + KeyRef key = toStringRef(cursor->key()); + ValueRef value = toStringRef(cursor->value()); + + accumulatedBytes += sizeof(KeyValueRef) + key.expectedSize() + value.expectedSize(); + result.emplace_back_deep(result.arena(), key, value); // Calling `cursor->Prev()` is potentially expensive, so short-circut here just in case. if (result.size() >= -a.rowLimit || accumulatedBytes >= a.byteLimit) { break; diff --git a/fdbserver/kvstore/KeyValueStoreShardedRocksDB.actor.cpp b/fdbserver/kvstore/KeyValueStoreShardedRocksDB.actor.cpp index 4e0e98fdaf2..890d71351c3 100644 --- a/fdbserver/kvstore/KeyValueStoreShardedRocksDB.actor.cpp +++ b/fdbserver/kvstore/KeyValueStoreShardedRocksDB.actor.cpp @@ -1111,9 +1111,11 @@ int readRangeInDb(PhysicalShard* shard, auto* cursor = readIter->iter.get(); cursor->Seek(toSlice(range.begin)); while (cursor->Valid() && toStringRef(cursor->key()) < range.end) { - KeyValueRef kv(toStringRef(cursor->key()), toStringRef(cursor->value())); - accumulatedBytes += sizeof(KeyValueRef) + kv.expectedSize(); - result->push_back_deep(result->arena(), kv); + KeyRef key = toStringRef(cursor->key()); + ValueRef value = toStringRef(cursor->value()); + + accumulatedBytes += sizeof(KeyValueRef) + key.expectedSize() + value.expectedSize(); + result->emplace_back_deep(result->arena(), key, value); // Calling `cursor->Next()` is potentially expensive, so short-circut here just in case. if (result->size() >= rowLimit || accumulatedBytes >= byteLimit) { break; @@ -1128,9 +1130,11 @@ int readRangeInDb(PhysicalShard* shard, cursor->Prev(); } while (cursor->Valid() && toStringRef(cursor->key()) >= range.begin) { - KeyValueRef kv(toStringRef(cursor->key()), toStringRef(cursor->value())); - accumulatedBytes += sizeof(KeyValueRef) + kv.expectedSize(); - result->push_back_deep(result->arena(), kv); + KeyRef key = toStringRef(cursor->key()); + ValueRef value = toStringRef(cursor->value()); + + accumulatedBytes += sizeof(KeyValueRef) + key.expectedSize() + value.expectedSize(); + result->emplace_back_deep(result->arena(), key, value); // Calling `cursor->Prev()` is potentially expensive, so short-circuit here just in case. if (result->size() >= -rowLimit || accumulatedBytes >= byteLimit) { break;