Skip to content

perf(orchestrator): coalesce NBD header + max payload into one read (rebase of #3373) - #3726

Closed
AdaAibaby wants to merge 1 commit into
e2b-dev:mainfrom
AdaAibaby:perf/nbd-coalesce-header-read
Closed

AdaAibaby wants to merge 1 commit into
e2b-dev:mainfrom
AdaAibaby:perf/nbd-coalesce-header-read

Conversation

@AdaAibaby

Copy link
Copy Markdown
Contributor

Rebased follow-up to #3373, which @ryan-sana closed with: "please rebase onto current main and include a before/after measurement." This is that rebase, with the measurement.

Problem (still present on current main)

dispatchBufferSize is exactly 4 MiB — the maximum typical NBD data payload. But every request also carries a 28-byte header, so a max-size (4 MiB) write does not fit the buffer whole:

Read #1: [28B header][4MiB - 28B payload]   ← fills the 4 MiB buffer exactly
Read #2: [remaining 28B of payload]         ← one extra syscall

The September NBD dispatch refactor (ad11877aa, 0c21aa227) reshaped how commands are passed and added a sync.Pool, but left the buffer sizing untouched — the TODO acknowledging this is still verbatim on main:

// TODO: Look into optimizing the buffer reads by increasing the buffer size by 28 bytes,
// to account for a request that is 28 bytes of header + 4MB of data ...
dispatchBufferSize = 4 * 1024 * 1024

Fix

  • Add nbdRequestHeaderSize = 28 and use it for the bare 28 literals in the dispatch loop (header completeness check, header slice, the three rp += 28).
  • Widen dispatchBufferSize to 4*1024*1024 + nbdRequestHeaderSize so [28B header + 4 MiB payload] is taken in a single Read.
  • Remove the resolved TODO and the now-inaccurate deadlock comment in the write path.

Before/after measurement (as requested)

Measured in-repo with a counting socket that feeds one 4 MiB write (header + full payload already buffered, as a busy kernel socket delivers it), counting the Read calls that returned payload bytes:

buffer size Read calls for one 4 MiB write
before (main) 4194304 2
after (this PR) 4194332 1

One syscall saved per max-size write. For I/O-heavy sandboxes doing large sequential block writes this is thousands of requests. Memory cost: 28 bytes per sandbox connection buffer.

Test

TestDispatchMaxSizeWriteSingleRead pins the behaviour: a 4 MiB write (header + full payload as one chunk) is consumed in exactly one Read, and the payload reaches the backend with the correct offset and full data integrity. Existing TestDispatchWriteZeroesReadLoopStall still passes; go build and go vet are clean.

Closes #3355. Supersedes #3373.

dispatchBufferSize was exactly 4 MiB, the maximum typical NBD data payload.
Every request also carries a 28-byte header, so a max-size (4 MiB) write did
not fit the buffer whole: the first Read filled 4 MiB (header + 4 MiB-28 of
payload) and a second Read was needed for the trailing 28 payload bytes. The
code carried a TODO acknowledging exactly this.

Widen dispatchBufferSize by one header (4*1024*1024 + nbdRequestHeaderSize) so
[28-byte header + 4 MiB payload] lands in a single Read. Introduce the named
nbdRequestHeaderSize constant for the header width and use it in place of the
bare 28 literals in the dispatch loop, and drop the resolved TODO and the now-
inaccurate deadlock comment in the write path.

Measured on this repo with a counting socket feeding one 4 MiB write
(header + full payload buffered), reads that returned payload bytes:

  before (buffer 4194304): 2 Reads
  after  (buffer 4194332): 1 Read

One syscall saved per max-size write; for I/O-heavy sandboxes doing large
sequential writes this is thousands of requests. Memory cost is 28 bytes per
sandbox connection buffer.

Rebased onto current main at the maintainers invitation on e2b-dev#3373 (the Sept NBD
dispatch refactor left the buffer sizing untouched). Includes the requested
before/after measurement as a pinned test (TestDispatchMaxSizeWriteSingleRead).
Closes e2b-dev#3355.
@ryan-sana

Copy link
Copy Markdown
Contributor

This still does not say how frequent the two system call scenarios would be and what the cost of two system calls is and how it impacts the overall performance. Is the a numbers that backs this?

Most of the time, we work with larger values than 4MB. This is only a problem if we deal with very small blocks which isn't a general case.

@ryan-sana ryan-sana closed this Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(orchestrator): increase NBD dispatch buffer by 28 bytes to coalesce header + max payload in one read

3 participants