Skip to content

[core][2/N] Avoid unnecessary deserialization/serialization of ObjectId#53574

Merged
edoakes merged 2 commits intoray-project:masterfrom
kevin85421:fix-gpu-objects-regression
Jun 9, 2025
Merged

[core][2/N] Avoid unnecessary deserialization/serialization of ObjectId#53574
edoakes merged 2 commits intoray-project:masterfrom
kevin85421:fix-gpu-objects-regression

Conversation

@kevin85421
Copy link
Copy Markdown
Member

@kevin85421 kevin85421 commented Jun 5, 2025 •
edited
Loading

Why are these changes needed?

task.ArgId(i).Binary()
  • ArgId(i) deserializes binary to ObjectId.
  • Binary() serializes ObjectId to binary.

speedup ~17% (#53574 (comment))

  • Rename ArgId to GetArgObjectId
  • Add GetArgObjectIdBinary

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@kevin85421
avoid deser/ser
1338731 
Signed-off-by: Kai-Hsun Chen <kaihsun@anyscale.com>
@kevin85421 kevin85421 added the go add ONLY when ready to merge, run all tests label Jun 5, 2025
@kevin85421
Copy link
Copy Markdown
Member Author

kevin85421 commented Jun 5, 2025 •
edited
Loading

stage_2_time: 346 -> 287 (speedup ~17%)

success = 1
  | stage_0_time = 7.181008815765381
  | stage_1_time = 127.03232049942017
  | stage_1_avg_iteration_time = 12.703226566314697
  | stage_1_max_iteration_time = 13.292524814605713
  | stage_1_min_iteration_time = 11.232972383499146
  | stage_2_time = 287.988343000412
  | stage_2_avg_iteration_time = 57.597555589675906
  | stage_2_max_iteration_time = 95.33804368972778
  | stage_2_min_iteration_time = 33.9080331325531
  | stage_3_creation_time = 1.769193172454834
  | stage_3_time = 1875.9398827552795
  | stage_4_spread = 0.5662058252500037

@kevin85421 kevin85421 changed the title [WIP][core] fix performance regression [core][2/N] Avoid unnecessary deserialization/serialization of ObjectId Jun 5, 2025
@kevin85421 kevin85421 marked this pull request as ready for review June 5, 2025 06:53
Copilot AI review requested due to automatic review settings June 5, 2025 06:53
@kevin85421 kevin85421 requested a review from a team June 5, 2025 06:53
Copilot AI reviewed Jun 5, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new method to fetch raw ObjectID binaries directly and updates call sites to avoid redundant ObjectID serialization/deserialization.

  • Add GetArgRawObjectId to TaskSpecification for efficient access to the argument’s binary ID
  • Update core_worker.cc and task_executor.cc to use the new raw ID method instead of ObjectID::Binary()
  • Include documentation and method signature changes in task_spec.h/.cc

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/ray/core_worker/core_worker.cc Replaced ArgId(i).Binary() with GetArgRawObjectId(i)
src/ray/common/task/task_spec.h Added documentation and declaration for GetArgRawObjectId
src/ray/common/task/task_spec.cc Implemented GetArgRawObjectId
cpp/src/ray/runtime/task/task_executor.cc Replaced ArgId(i).Binary() with GetArgRawObjectId(i)
Comments suppressed due to low confidence (3)

src/ray/common/task/task_spec.h:344

  • Clarify in the doc comment that this returns the binary string matching ObjectID::Binary(), so readers know it’s the same format.
/// Get the raw object ID of the argument at the given index.

src/ray/common/task/task_spec.h:348

  • [nitpick] Consider renaming this method to something like ArgIdBinary or GetArgIdString to clearly distinguish it from ArgId, which returns an ObjectID.
std::string GetArgRawObjectId(size_t arg_index) const;

src/ray/common/task/task_spec.cc:293

  • No unit tests cover this new method. Adding tests for both reference and inlined argument cases will ensure GetArgRawObjectId behaves as expected.
std::string TaskSpecification::GetArgRawObjectId(size_t arg_index) const {

edoakes
edoakes reviewed Jun 5, 2025
View reviewed changes
@dayshah
Copy link
Copy Markdown
Contributor

dayshah commented Jun 5, 2025

oh wow i didn't expect this to have such a big impact, what does stage_2 time actually measure

@kevin85421
rename
87660de 
Signed-off-by: Kai-Hsun Chen <kaihsun@anyscale.com>
@kevin85421
Copy link
Copy Markdown
Member Author

kevin85421 commented Jun 9, 2025

oh wow i didn't expect this to have such a big impact, what does stage_2 time actually measure

ray/release/nightly_tests/stress_tests/test_many_tasks.py

Line 77 in c54437c

def stage2(smoke=False):

It passes 20 (smoke=True) or 500 (smoke=False) object refs for small objects to a Ray task

@kevin85421 kevin85421 requested a review from edoakes June 9, 2025 05:57
kevin85421
kevin85421 commented Jun 9, 2025
View reviewed changes
src/ray/common/task/task_spec.cc
!message_->args(arg_index).is_inlined();
}

ObjectID TaskSpecification::ArgId(size_t arg_index) const {
Copy link
Copy Markdown
Member Author

@kevin85421 kevin85421 Jun 9, 2025

Choose a reason for hiding this comment

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

main changes

@kevin85421 kevin85421 mentioned this pull request Jun 9, 2025
Merged
8 tasks
@edoakes edoakes merged commit 63ea48b into ray-project:master Jun 9, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@edoakes edoakes edoakes approved these changes

Assignees

@jjyao jjyao

@edoakes edoakes

Labels

go add ONLY when ready to merge, run all tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kevin85421 @dayshah @edoakes @jjyao