[core] (1/n ray.get) Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get #57691

israbbani · 2025-10-14T06:33:19Z

This is 1/n in a series of PRs to fix #54007.

CoreWorker::Get has two parts

CoreWorkerMemoryStore::Get (to wait for objects to become ready anywhere in the cluster)
PlasmaStoreProvider::Get (to fetch objects into local plasma and return a ptr to shm).

The CoreWorker tries to implement cooperative scheduling by yielding CPU back to the raylet if it's blocked. The only time does this in practice is when it called CoreWorkerMemoryStore::Get. The rational (as discussed in #12912) is that the worker is not using any resources.

PlasmaStoreProvider::Get does not yield CPU by notifying the raylet that it's blocked, but instead calls NotifyWorkerUnblocked. This is a bug. It does this to clean up an inflight or completed "Get" request from the worker.

In this PR, I clean up PlasmaStoreProvider::Get so it

No longer calls NotifyWorkerUnblocked sometimes (with some convoluted checking to see if we're executing a NORMAL_TASK on the main thread or an ACTOR_CREATION_TASK).
Instead calls CancelGetRequest on (almost) all exits from the function. This is because even if PlasmaStoreProvider::Get is successful, it still needs to clean up the "Get" request on the raylet.
Removes unnecessary parameters.

…ecause it never calls NotifyWorkerBlocked. Therefore, it doesn't have to release resources and only cancel inflight Pull requests. Signed-off-by: irabbani <[email protected]>

Signed-off-by: irabbani <[email protected]>

edoakes · 2025-10-16T13:26:03Z

Instead calls CancelGetRequest on (almost) all exits from the function. This is because even if PlasmaStoreProvider::Get is successful, it still needs to clean up the "Get" request on the raylet.

Were you able to figure out why this is necessary?

edoakes · 2025-10-16T13:28:55Z

src/ray/core_worker/store_provider/plasma_store_provider.cc

+  // objects contain an exception, clean up the Get request in the raylet
+  // and early exit.
+  if (remaining.empty() || got_exception) {
+    return raylet_ipc_client_->CancelGetRequest();


Note that there is a behavior difference here. This will now call CancelGetRequest from background threads in normal tasks where previously NotifyWorkerUnblocked was not called. This can exacerbate the "accidental cancelation" problem where one thread cancels outstanding get requests from others. I'm OK with this as an intermediate state since the next PR will presumably implement the get request ID logic.

edoakes · 2025-10-16T13:29:36Z

src/ray/core_worker/store_provider/plasma_store_provider.h

             int64_t timeout_ms,
-             const WorkerContext &ctx,
-             absl::flat_hash_map<ObjectID, std::shared_ptr<RayObject>> *results,
-             bool *got_exception);


got_exception was unused at the callsite? 🤔

edoakes · 2025-10-16T13:30:30Z

src/ray/core_worker/store_provider/plasma_store_provider.cc

  return store_client_->GetExperimentalMutableObject(object_id, mutable_object);
 }

-Status UnblockIfNeeded(


can't describe how happy I am to see this in the red

edoakes · 2025-10-16T13:31:28Z

src/ray/core_worker/store_provider/plasma_store_provider.cc

+  // objects contain an exception, clean up the Get request in the raylet
+  // and early exit.


Suggested change

// objects contain an exception, clean up the Get request in the raylet

// and early exit.

// objects contain an exception, cancel the Get request and exit early.

…rovider::Get (ray-project#57691) This is 1/n in a series of PRs to fix ray-project#54007. CoreWorker::Get has two parts * CoreWorkerMemoryStore::Get (to wait for objects to become ready _anywhere_ in the cluster) * PlasmaStoreProvider::Get (to fetch objects into local plasma and return a ptr to shm). The CoreWorker tries to implement cooperative scheduling by yielding CPU back to the raylet if it's blocked. The only time does this in practice is when it called CoreWorkerMemoryStore::Get. The rational (as discussed in ray-project#12912) is that the worker is not using any resources. PlasmaStoreProvider::Get does not yield CPU by notifying the raylet that it's blocked, but instead calls NotifyWorkerUnblocked. This is a bug. It does this to clean up an inflight or completed "Get" request from the worker. In this PR, I clean up PlasmaStoreProvider::Get so it * No longer calls NotifyWorkerUnblocked sometimes (with some convoluted checking to see if we're executing a NORMAL_TASK on the main thread or an ACTOR_CREATION_TASK). * Instead calls CancelGetRequest on (almost) all exits from the function. This is because even if PlasmaStoreProvider::Get is successful, it still needs to clean up the "Get" request on the raylet. * Removes unnecessary parameters. --------- Signed-off-by: irabbani <[email protected]>

…rovider::Get (ray-project#57691) This is 1/n in a series of PRs to fix ray-project#54007. CoreWorker::Get has two parts * CoreWorkerMemoryStore::Get (to wait for objects to become ready _anywhere_ in the cluster) * PlasmaStoreProvider::Get (to fetch objects into local plasma and return a ptr to shm). The CoreWorker tries to implement cooperative scheduling by yielding CPU back to the raylet if it's blocked. The only time does this in practice is when it called CoreWorkerMemoryStore::Get. The rational (as discussed in ray-project#12912) is that the worker is not using any resources. PlasmaStoreProvider::Get does not yield CPU by notifying the raylet that it's blocked, but instead calls NotifyWorkerUnblocked. This is a bug. It does this to clean up an inflight or completed "Get" request from the worker. In this PR, I clean up PlasmaStoreProvider::Get so it * No longer calls NotifyWorkerUnblocked sometimes (with some convoluted checking to see if we're executing a NORMAL_TASK on the main thread or an ACTOR_CREATION_TASK). * Instead calls CancelGetRequest on (almost) all exits from the function. This is because even if PlasmaStoreProvider::Get is successful, it still needs to clean up the "Get" request on the raylet. * Removes unnecessary parameters. --------- Signed-off-by: irabbani <[email protected]> Signed-off-by: xgui <[email protected]>

…rovider::Get (#57691) This is 1/n in a series of PRs to fix #54007. CoreWorker::Get has two parts * CoreWorkerMemoryStore::Get (to wait for objects to become ready _anywhere_ in the cluster) * PlasmaStoreProvider::Get (to fetch objects into local plasma and return a ptr to shm). The CoreWorker tries to implement cooperative scheduling by yielding CPU back to the raylet if it's blocked. The only time does this in practice is when it called CoreWorkerMemoryStore::Get. The rational (as discussed in #12912) is that the worker is not using any resources. PlasmaStoreProvider::Get does not yield CPU by notifying the raylet that it's blocked, but instead calls NotifyWorkerUnblocked. This is a bug. It does this to clean up an inflight or completed "Get" request from the worker. In this PR, I clean up PlasmaStoreProvider::Get so it * No longer calls NotifyWorkerUnblocked sometimes (with some convoluted checking to see if we're executing a NORMAL_TASK on the main thread or an ACTOR_CREATION_TASK). * Instead calls CancelGetRequest on (almost) all exits from the function. This is because even if PlasmaStoreProvider::Get is successful, it still needs to clean up the "Get" request on the raylet. * Removes unnecessary parameters. --------- Signed-off-by: irabbani <[email protected]> Signed-off-by: elliot-barn <[email protected]>

This reverts commit 3af4650. Revert "[core] (1/n ray.get) Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get (#57691)" This reverts commit 6539477. Signed-off-by: irabbani <[email protected]>

…rovider::Get (ray-project#57691) This is 1/n in a series of PRs to fix ray-project#54007. CoreWorker::Get has two parts * CoreWorkerMemoryStore::Get (to wait for objects to become ready _anywhere_ in the cluster) * PlasmaStoreProvider::Get (to fetch objects into local plasma and return a ptr to shm). The CoreWorker tries to implement cooperative scheduling by yielding CPU back to the raylet if it's blocked. The only time does this in practice is when it called CoreWorkerMemoryStore::Get. The rational (as discussed in ray-project#12912) is that the worker is not using any resources. PlasmaStoreProvider::Get does not yield CPU by notifying the raylet that it's blocked, but instead calls NotifyWorkerUnblocked. This is a bug. It does this to clean up an inflight or completed "Get" request from the worker. In this PR, I clean up PlasmaStoreProvider::Get so it * No longer calls NotifyWorkerUnblocked sometimes (with some convoluted checking to see if we're executing a NORMAL_TASK on the main thread or an ACTOR_CREATION_TASK). * Instead calls CancelGetRequest on (almost) all exits from the function. This is because even if PlasmaStoreProvider::Get is successful, it still needs to clean up the "Get" request on the raylet. * Removes unnecessary parameters. --------- Signed-off-by: irabbani <[email protected]>

…rovider::Get (ray-project#57691) This is 1/n in a series of PRs to fix ray-project#54007. CoreWorker::Get has two parts * CoreWorkerMemoryStore::Get (to wait for objects to become ready _anywhere_ in the cluster) * PlasmaStoreProvider::Get (to fetch objects into local plasma and return a ptr to shm). The CoreWorker tries to implement cooperative scheduling by yielding CPU back to the raylet if it's blocked. The only time does this in practice is when it called CoreWorkerMemoryStore::Get. The rational (as discussed in ray-project#12912) is that the worker is not using any resources. PlasmaStoreProvider::Get does not yield CPU by notifying the raylet that it's blocked, but instead calls NotifyWorkerUnblocked. This is a bug. It does this to clean up an inflight or completed "Get" request from the worker. In this PR, I clean up PlasmaStoreProvider::Get so it * No longer calls NotifyWorkerUnblocked sometimes (with some convoluted checking to see if we're executing a NORMAL_TASK on the main thread or an ACTOR_CREATION_TASK). * Instead calls CancelGetRequest on (almost) all exits from the function. This is because even if PlasmaStoreProvider::Get is successful, it still needs to clean up the "Get" request on the raylet. * Removes unnecessary parameters. --------- Signed-off-by: irabbani <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>

[core] Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get b…

341f5fd

…ecause it never calls NotifyWorkerBlocked. Therefore, it doesn't have to release resources and only cancel inflight Pull requests. Signed-off-by: irabbani <[email protected]>

israbbani added core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests labels Oct 14, 2025

israbbani and others added 2 commits October 14, 2025 16:41

Merge branch 'master' into irabbani/ray-get-1

07827b2

i have no words

ff25b97

Signed-off-by: irabbani <[email protected]>

israbbani changed the title ~~[core] Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get~~ [core] (1/n ray.get) Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get Oct 16, 2025

add documentation and cleaning up unused args

8565025

Signed-off-by: irabbani <[email protected]>

israbbani marked this pull request as ready for review October 16, 2025 06:48

israbbani requested a review from a team as a code owner October 16, 2025 06:48

comments

f1c3b78

Signed-off-by: irabbani <[email protected]>

edoakes approved these changes Oct 16, 2025

View reviewed changes

edoakes merged commit 6539477 into master Oct 16, 2025
6 checks passed

edoakes deleted the irabbani/ray-get-1 branch October 16, 2025 14:34

israbbani mentioned this pull request Oct 26, 2025

[core] Reverting thread-safe ray.get because of perf regression #58177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[core] (1/n ray.get) Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get #57691

[core] (1/n ray.get) Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get #57691

Uh oh!

israbbani commented Oct 14, 2025 •

edited

Loading

Uh oh!

edoakes commented Oct 16, 2025

Uh oh!

edoakes Oct 16, 2025

Uh oh!

edoakes Oct 16, 2025

Uh oh!

edoakes Oct 16, 2025

Uh oh!

edoakes Oct 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// objects contain an exception, clean up the Get request in the raylet
		// and early exit.

	// objects contain an exception, clean up the Get request in the raylet
	// and early exit.
	// objects contain an exception, cancel the Get request and exit early.

[core] (1/n ray.get) Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get #57691

[core] (1/n ray.get) Removing NotifyWorkerUnblocked from PlasmaStoreProvider::Get #57691

Uh oh!

Conversation

israbbani commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edoakes commented Oct 16, 2025

Uh oh!

edoakes Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

edoakes Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

edoakes Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

edoakes Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

israbbani commented Oct 14, 2025 •

edited

Loading