Skip to content

[core] Remove proto copies on object store transfers - #61093

Draft
dayshah wants to merge 9 commits into
ray-project:masterfrom
dayshah:obj-transfers
Draft

[core] Remove proto copies on object store transfers#61093
dayshah wants to merge 9 commits into
ray-project:masterfrom
dayshah:obj-transfers

Conversation

@dayshah

@dayshah dayshah commented Feb 17, 2026

Copy link
Copy Markdown
Contributor

Problem

The current default object store transfer protocol comes with some major problems due to the fact the object has to be copied into and out of protobufs when sending the request and when receiving the request.

The extra memory usage from these intermediate protobufs means:

  • We keep a 2GB cap on outgoing transfers. This cap is also calculated incorrectly for objects that are <5mb, as it assumes every chunk of an object is 5mb. Objects are automatically put in the object store when >100kb and can be explicitly put in at even smaller sizes.
  • When doing many -> one transfers of objects, the receiving node can end up having increased memory usage due to the accumulation of the protobufs that it receives.

Solution

dayshah and others added 6 commits February 15, 2026 16:23
Signed-off-by: dayshah <dhyeyhshah@gmail.com>
Signed-off-by: dayshah <dhyey2019@gmail.com>
Signed-off-by: dayshah <dhyey2019@gmail.com>
Signed-off-by: dayshah <dhyey2019@gmail.com>
Signed-off-by: dayshah <dhyey2019@gmail.com>
Signed-off-by: dayshah <dhyey2019@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a significant performance optimization by implementing a zero-copy path for object store transfers (Push RPC). This is achieved by using raw grpc::ByteBuffer instead of protobuf serialization for object chunks, avoiding memory copies. The changes are extensive, touching the RPC layer, object manager, and related components. A new wire protocol for raw pushes is defined, and corresponding client/server handlers are implemented.

My main feedback points are:

  1. The PushManager has been simplified, but its rate-limiting logic has been removed. This is a significant behavioral change that could have performance implications.
  2. There appears to be some dead code in the metrics accounting within SendObjectChunk.

Overall, this is a great improvement for performance. The changes are well-structured to support the new zero-copy mechanism.

Comment thread src/ray/object_manager/push_manager.cc
Comment on lines +517 to +521
if (from_disk) {
num_bytes_pushed_from_disk_ += chunk_ref->size;
} else {
num_bytes_pushed_from_plasma_ += chunk_ref->size;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This if (from_disk) block appears to be dead code. The zero-copy path via GetChunkRef is only taken for in-memory objects, for which from_disk will be false. When from_disk is true (for spilled objects), GetChunkRef returns std::nullopt, so this block is never reached. This can be simplified.

      num_bytes_pushed_from_plasma_ += chunk_ref->size;

@github-actions

github-actions Bot commented Mar 3, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@github-actions github-actions Bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 3, 2026
@dayshah dayshah added unstale A PR that has been marked unstale. It will not get marked stale again if this label is on it. and removed stale The issue is stale. It will be closed within 7 days unless there are further conversation labels Mar 4, 2026
Signed-off-by: dayshah <dhyey2019@gmail.com>
@dayshah dayshah added the go add ONLY when ready to merge, run all tests label Mar 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests unstale A PR that has been marked unstale. It will not get marked stale again if this label is on it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant