Skip to content

Conversation

rueian
Copy link
Contributor

@rueian rueian commented Jun 25, 2025

Why are these changes needed?

Following the suggestion in #53782 (comment), this PR replace all unnecessary shared_ptrs with unique_ptrs and references in Raylet.

Hi @dayshah,
Could you take a look? Thanks!

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 :(

@rueian rueian force-pushed the raylet-no-sharedptr branch 9 times, most recently from f48c876 to 8d3ce74 Compare June 25, 2025 06:04
… references in raylet

Signed-off-by: Rueian <rueian@anyscale.com>
Signed-off-by: Rueian <rueiancsie@gmail.com>
@rueian rueian force-pushed the raylet-no-sharedptr branch from 8d3ce74 to 2ffa8f9 Compare June 25, 2025 06:20
@rueian rueian marked this pull request as ready for review June 25, 2025 15:50
@Copilot Copilot AI review requested due to automatic review settings June 25, 2025 15:50
Copy link
Contributor

@Copilot 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 replaces unnecessary std::shared_ptr usages with std::unique_ptr and raw references in the Raylet code to improve ownership clarity and reduce overhead.

  • Changed constructors and member fields to accept and store non-owning references for gcs::GcsClient and other interfaces.
  • Updated all call sites in tests, main.cc, and various Raylet modules to pass dereferenced pointers.
  • Adjusted the BUILD file to include the new :ray_mock target and consolidate compiler_param_file features.

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/ray/raylet/worker_pool.h Changed gcs_client from shared_ptr to reference
src/ray/raylet/worker_pool.cc Updated initializer list and calls to use reference
src/ray/raylet/worker_pool_test.cc Passed *mock_gcs_client_ into test constructor
src/ray/raylet/node_manager.h Refactored numerous params and members to references
src/ray/raylet/node_manager.cc Updated method bodies to use reference members
src/ray/raylet/raylet.h Switched gcs_client and node_manager to references
src/ray/raylet/raylet.cc Updated ctor and member access to reference style
src/ray/raylet/placement_group_resource_manager.{h,cc} Changed scheduler member to reference
src/ray/raylet/main.cc Switched shared_ptr to unique_ptr and passed references
src/ray/object_manager/ownership_object_directory.{h,cc} Converted gcs_client fields and calls to reference
src/ray/object_manager/object_manager.{h,cc} Updated gcs_client ownership to reference
BUILD.bazel Added :ray_mock and consolidated compiler_param_file features
Comments suppressed due to low confidence (2)

src/ray/raylet/worker_pool.h:324

  • [nitpick] The constructor parameter gcs_client is now a non-owning reference; consider updating the doc comment to note that the caller must guarantee its lifetime outlives the WorkerPool instance.
             gcs::GcsClient &gcs_client,

src/ray/raylet/node_manager.h:138

  • [nitpick] Update the parameter comment for gcs_client to clarify that this is a non-owning reference and must remain valid for the lifetime of NodeManager.
      gcs::GcsClient &gcs_client,

@dayshah dayshah added go add ONLY when ready to merge, run all tests labels Jun 25, 2025
Copy link
Contributor

@dayshah dayshah left a comment

Choose a reason for hiding this comment

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

thanks a lot for doing this!!! @edoakes for merge

@edoakes
Copy link
Collaborator

edoakes commented Jun 25, 2025

amazing, thanks @rueian

@edoakes edoakes enabled auto-merge (squash) June 25, 2025 20:09
@edoakes edoakes disabled auto-merge June 25, 2025 20:09
@edoakes edoakes enabled auto-merge (squash) June 25, 2025 20:09
@edoakes edoakes merged commit 427d5fa into ray-project:master Jun 25, 2025
8 checks passed
israbbani added a commit that referenced this pull request Jun 27, 2025
…ptrs and references in raylet (#54062)"

This reverts commit 427d5fa.
minerharry pushed a commit to minerharry/ray that referenced this pull request Jun 27, 2025
… references in raylet (ray-project#54062)

Following the suggestion in
ray-project#53782 (comment),
this PR replace all unnecessary shared_ptrs with unique_ptrs and
references in Raylet.

Signed-off-by: Rueian <rueian@anyscale.com>
Signed-off-by: Rueian <rueiancsie@gmail.com>
elliot-barn pushed a commit that referenced this pull request Jul 2, 2025
… references in raylet (#54062)

Following the suggestion in
#53782 (comment),
this PR replace all unnecessary shared_ptrs with unique_ptrs and
references in Raylet.

Signed-off-by: Rueian <rueian@anyscale.com>
Signed-off-by: Rueian <rueiancsie@gmail.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants