Skip to content

[WIP][core] ReferenceProtoTable cleanup #60611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
edoakes wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

[WIP][core] ReferenceProtoTable cleanup #60611

edoakes wants to merge 5 commits into from

Conversation

@edoakes
Copy link
Collaborator

@edoakes edoakes commented Jan 30, 2026

No description provided.

@edoakes
fix
479d877 
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@edoakes edoakes added the go add ONLY when ready to merge, run all tests label Jan 30, 2026
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jan 30, 2026
View reviewed changes
Copy link
Contributor

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

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 provides a nice cleanup by removing the ReferenceProtoTable type alias and its associated conversion function ReferenceTableToProto. The logic is simplified by directly populating the ReferenceTableProto protobuf message. Additionally, the introduction of an encountered set in GetAndClearLocalBorrowersInternal is a great improvement to prevent infinite recursion on cyclic object dependencies. The changes look solid and improve code maintainability. I have one minor suggestion for removing some dead code.
@edoakes
fix
c57bda6 
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@edoakes
fix
83aa7db 
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@edoakes
fix
35ec121 
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@edoakes
fix
46b7331 
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
edoakes
edoakes commented Jan 30, 2026
View reviewed changes
src/ray/core_worker/reference_counter.cc
Comment on lines -1362 to -1363
&borrowed_refs));
for (const auto &[id, ref] : borrowed_refs) {
Copy link
Collaborator Author

@edoakes edoakes Jan 30, 2026

Choose a reason for hiding this comment

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

this was moved inside GetAndClearLocalBorrowersInternal for implementation convenience
edoakes
edoakes commented Jan 30, 2026
View reviewed changes
src/ray/core_worker/reference_counter.cc
}

void ReferenceCounter::Reference::ToProto(rpc::ObjectReferenceCount *ref,
bool deduct_local_ref) const {
Copy link
Collaborator Author

@edoakes edoakes Jan 30, 2026

Choose a reason for hiding this comment

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

Found it to be a better separation of concerns to avoid any control flow inside the proto serialization method.
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

1 participant

@edoakes