perf(fdbclient): reduce arena allocations in MutationRef deep-copy constructors by pchintar · Pull Request #13270 · apple/foundationdb

pchintar · 2026-05-23T23:39:49Z

Closes issue #13269 .

Summary

Reduce arena allocation overhead in MutationRef deep-copy constructors by storing copied mutation payloads in a single arena allocation instead of allocating param1 and param2 independently.

Problem

Previously, both deep-copy constructors performed two separate arena allocations:

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) {}

Each StringRef(Arena&, ...) construction allocates independently inside the arena, causing every deep-copied mutation to perform two separate allocations.

This affects hot paths such as:

VectorRef<MutationRef>::emplace_back_deep
VectorRef<MutationRef>::push_back_deep
commit batching and mutation construction paths

Changes

Both deep-copy constructors now:

allocate a single contiguous arena buffer for both payloads
copy param1 and param2 into that shared allocation
preserve existing StringRef semantics and ownership behavior

Now arena payload allocations are reduced from two allocations per mutation to one allocation per mutation. No public APIs or serialization behavior are changed.

Benchmark

Benchmarks were run using:

bin/fdbclient_bench --benchmark_filter='bench_populate' --benchmark_repetitions=10 --benchmark_report_aggregates_only=true

Observed trends:

Speed improvements for high mutation-count workloads with small-to-medium payload sizes
largest gains occur in allocation-dominated cases where allocator overhead is a significant portion of total runtime
little to no change for very large payload sizes where memcpy bandwidth dominates

Testing

ninja fdbclient_bench
bin/fdbclient_bench --benchmark_filter='bench_populate' --benchmark_repetitions=10 --benchmark_report_aggregates_only=true

@akankshamahajan15 and @neethuhaneesha could you pls run the checks if you have the access to do so and review this PR? thnx

Ronitsabhaya75 · 2026-05-27T21:38:31Z

@pchintar thank you for this contribution unfortunately I don't have appropriate access for starting the CI run.

If you want I can review the work and give some suggestions.

You can wait for appropriate people to have look at it and start the Ci run. :)

pchintar · 2026-05-28T22:13:46Z

@Ronitsabhaya75 none of the others have replied yet, so what do I do now? I'm new here that's why I'm asking

Ronitsabhaya75 · 2026-05-28T22:18:37Z

@pchintar im sure they'll see this pr. Don't worry :)

saintstack · 2026-05-29T21:51:40Z

+	    param1 = StringRef(packed, from.param1.size());
+	    param2 = StringRef(packed + from.param1.size(), from.param2.size());
+    }
+


Could we get rid of the duplicated code? A little inline function?

Good point @saintstack I’ll factor the shared packing logic into a small helper and call it from both constructors.

@saintstack I've just updated & also fixed formatting with help of clang-format, so can you pls check it out now & run the checks? thnx

saintstack · 2026-05-30T00:16:02Z

+		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());


Copilot worried about case where totalSIze = 0; we'd end up doing nullptr arithmetic. Suggestion below. Thanks.

Suggested change

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

const size_t aSize = a.size();

const size_t bSize = b.size();

const size_t totalSize = aSize + bSize;

uint8_t* packed = totalSize ? new (to) uint8_t[totalSize] : nullptr;

if (aSize) {

memcpy(packed, a.begin(), aSize);

}

if (bSize) {

memcpy(packed + aSize, b.begin(), bSize);

}

param1 = StringRef(aSize ? packed : nullptr, aSize);

param2 = StringRef(bSize ? packed + aSize : nullptr, bSize);

@saintstack Yep, I've just fixed that, the totalSize == 0 case now explicitly uses nullptr for empty payloads.

…nstructors

foundationdb-ci · 2026-06-01T17:04:46Z

Result of foundationdb-pr-clang-ide on Linux RHEL 9

Commit ID: bf177cc
Duration 0:21:15
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2026-06-01T17:16:23Z

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: bf177cc
Duration 0:32:49
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2026-06-01T17:27:35Z

Result of foundationdb-pr-clang-arm on Linux CentOS 7

Commit ID: bf177cc
Duration 0:43:59
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2026-06-01T17:30:11Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: bf177cc
Duration 0:46:38
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2026-06-01T17:37:44Z

Result of foundationdb-pr on Linux RHEL 9

Commit ID: bf177cc
Duration 0:54:13
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2026-06-01T17:45:06Z

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

Commit ID: bf177cc
Duration 1:01:32
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2026-06-01T17:45:47Z

Result of foundationdb-pr-clang on Linux RHEL 9

Commit ID: bf177cc
Duration 1:02:15
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

saintstack · 2026-06-02T16:05:47Z

Thank you for the contribution @pchintar

pchintar · 2026-06-02T16:15:14Z

@saintstack, thank you. Could you kindly check my other PR #13273 if possible? I've fixed the formatting and the CI checks have passed for it as well.

saintstack reviewed May 29, 2026

View reviewed changes

pchintar force-pushed the perf/mutationref-packed-deep-copy branch 2 times, most recently from 9ceb592 to 9e438c4 Compare May 29, 2026 23:07

saintstack reviewed May 30, 2026

View reviewed changes

perf(fdbclient): reduce arena allocations in MutationRef deep-copy co…

bf177cc

…nstructors

pchintar force-pushed the perf/mutationref-packed-deep-copy branch from 9e438c4 to bf177cc Compare May 30, 2026 00:31

saintstack closed this Jun 1, 2026

saintstack reopened this Jun 1, 2026

saintstack approved these changes Jun 2, 2026

View reviewed changes

saintstack merged commit 5841daa into apple:main Jun 2, 2026
8 checks passed

Conversation

pchintar commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

Benchmark

Testing

Uh oh!

Ronitsabhaya75 commented May 27, 2026

Uh oh!

pchintar commented May 28, 2026

Uh oh!

Ronitsabhaya75 commented May 28, 2026

Uh oh!

saintstack May 29, 2026

Choose a reason for hiding this comment

Uh oh!

pchintar May 29, 2026

Choose a reason for hiding this comment

Uh oh!

pchintar May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saintstack May 30, 2026

Choose a reason for hiding this comment

Uh oh!

pchintar May 30, 2026

Choose a reason for hiding this comment

Uh oh!

foundationdb-ci commented Jun 1, 2026

Result of foundationdb-pr-clang-ide on Linux RHEL 9

Uh oh!

foundationdb-ci commented Jun 1, 2026

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Uh oh!

foundationdb-ci commented Jun 1, 2026

Result of foundationdb-pr-clang-arm on Linux CentOS 7

Uh oh!

foundationdb-ci commented Jun 1, 2026

Result of foundationdb-pr-macos on macOS Ventura 13.x

Uh oh!

foundationdb-ci commented Jun 1, 2026

Result of foundationdb-pr on Linux RHEL 9

Uh oh!

foundationdb-ci commented Jun 1, 2026

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

Uh oh!

foundationdb-ci commented Jun 1, 2026

Result of foundationdb-pr-clang on Linux RHEL 9

Uh oh!

Uh oh!

saintstack commented Jun 2, 2026

Uh oh!

pchintar commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pchintar commented May 23, 2026 •

edited

Loading

pchintar May 29, 2026 •

edited

Loading

pchintar commented Jun 2, 2026 •

edited

Loading