Skip to content

Stream ByteStream uploads segment by segment; closes #1565 - #1566

Open
vlad-ivanov-name wants to merge 1 commit into
meta:mainfrom
vlad-ivanov-name:vlad/lazy-bytestream-upload
Open

Stream ByteStream uploads segment by segment; closes #1565#1566
vlad-ivanov-name wants to merge 1 commit into
meta:mainfrom
vlad-ivanov-name:vlad/lazy-bytestream-upload

Conversation

@vlad-ivanov-name

@vlad-ivanov-name vlad-ivanov-name commented Oct 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What

  • upload_impl builds WriteRequest segments lazily from the reader and passes the stream to ByteStream.Write. Segments are still max_total_batch_size bytes, with finish_write on the last data segment, so the wire shape does not change.
  • A read error during the stream ends it without finish_write and fails the upload.
  • The committed_size check also accepts the full blob size. The REAPI text says a server that already has the blob answers early with the full size regardless of how much data the client sent. This worked before only because the whole blob was always read first.
  • After the RPC returned, the segment stream ends on its next poll. hyper drives the request body in its own task, so after an early server answer it would otherwise keep reading the file until the server resets the stream, and a read error in that window would fail an upload the server already accepted.

Why

The whole blob was collected into a Vec<WriteRequest> before the first byte was sent, so a large upload used as much memory as the blob. Now at most two segments are in memory.

Notes

  • A BoxStream passed to tonic's write directly fails with "implementation of Send is not general enough" at the upload_impl call, also with the closure parameter annotated. A small newtype that forwards poll_next avoids it.
  • Tests: the existing upload tests collect the stream first. Three new tests cover the early success answer, a read error mid-stream, and the stream ending after the RPC returned.
  • Measured against a filesystem-only nativelink at 36 ms RTT with zstd advertised: a 1 GiB upload went from 24-26 s to 16 s, and the daemon's peak RSS during the upload from 1.39 GiB to 74 MiB.

Closes #1565
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 13, 2026
@meta-codesync

meta-codesync Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

This pull request has been imported. If you are a Meta employee, you can view this in D124990313. (Because this pull request was imported automatically, there will not be any future comments.)
@vlad-ivanov-name
vlad-ivanov-name marked this pull request as ready for review October 13, 2026 15:18
@vlad-ivanov-name
Stream ByteStream uploads segment by segment
498e3e7 
What:
- upload_impl builds WriteRequest segments lazily from the reader and passes the stream to ByteStream.Write.
- A read error during the stream ends it without finish_write and fails the upload.
- The committed_size check also accepts the full blob size, which a server returns when it already has the blob.
- After the RPC returned, the segment stream ends on its next poll, because the transport can still drive it.

Why:
- The whole blob was collected into a Vec<WriteRequest> before the first byte was sent, so a large upload used as much memory as the blob.
@vlad-ivanov-name
vlad-ivanov-name force-pushed the vlad/lazy-bytestream-upload branch from 99b09ec to 498e3e7 Compare October 14, 2026 07:50
@vlad-ivanov-name

vlad-ivanov-name commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Updated code -- not sure if previous failure was a fluke but internal tests pass now
This was referenced Oct 14, 2026
Open
Open

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

1 participant

@vlad-ivanov-name