[core][scalability] Offload RPC Finish to a thread pool. #30131
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Yi Cheng <[email protected]>
fishbone
force-pushed
the
offload-to-rpc
branch
from
ae3e6be
to
af06b65
Compare
|
Still benchmarking... For shuffle_20gb_with_state_api_1668017686, the avg latency dropped for 50%. |
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
|
new - old: |
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
|
I also plan to increase the number of thread for client/server gcs gRPC to hardward concurrenty / 4 by default. I'll have a benchmark and comparison later. |
|
hmmm CI failed. let me check. |
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
rkooo567
approved these changes
Nov 10, 2022
| } else { | ||
| const auto &destroyed_actor_iter = destroyed_actors_.find(actor_id); | ||
| if (destroyed_actor_iter != destroyed_actors_.end()) { | ||
| reply->unsafe_arena_set_allocated_actor_table_data( |
There was a problem hiding this comment.
This is unrelated to the PR right? Is it to use Arena on actor related RPCs?
There was a problem hiding this comment.
No, it is related. See here we send Finish in another thread which means these data needs to be alive until they got sent.
Here arena::create with a placement new it'll create a shared ptr so that it's shared across threads.
Otherwise, it's going to crash.
src/ray/rpc/server_call.cc
Outdated
| namespace ray { | ||
| namespace rpc { | ||
|
|
||
| static std::unique_ptr<boost::asio::thread_pool> executor_; |
There was a problem hiding this comment.
Maybe we should join this?
There was a problem hiding this comment.
also is this thread-safe?
There was a problem hiding this comment.
I'm not sure, but I feel no need to join (also a little bit tricky to join it. we can do).
Right now, it only calls Finish which will send event to the cq. I think grpc stop should handle this.
But I can't be 100% confident in this one.
I think the issue for the join is that, when should we join. This is a shared global thread pool among servers. So it should be joined after all thread pool destructed. And also the last one to join (it's a global vars).
| // this server call might be deleted | ||
| SendReply(status); | ||
| boost::asio::post(GetServerCallExecutor(), | ||
| [this, status]() { SendReply(status); }); |
There was a problem hiding this comment.
Can you comment SendReply is thread-safe?
There was a problem hiding this comment.
No, SendReply is not thread safe. Here at the same time, only one SendReply is going to be called per object.
There was a problem hiding this comment.
hmm seems SendReply is accessing shared state in this object, would this cause thread safety issue?
There was a problem hiding this comment.
Yes, but I don't think that's the cause of the test failure
It's because grpc shutdown not taking care of in-flight requests.
But the read and write is also an issue here. Haven't seen anything wrong with it yet.
I'm going to hold the PR for a while. Still targeting 2.2.
rkooo567
added
the
@author-action-required
fishbone
removed
the
@author-action-required
Signed-off-by: Yi Cheng <[email protected]>
|
ASAN test failed. Maybe related to the join issue mentioned by @rkooo567 . Let me debug it. |
fishbone
added
the
@author-action-required
scv119
reviewed
Nov 11, 2022
src/ray/rpc/server_call.cc
Outdated
|
|
||
| static std::unique_ptr<boost::asio::thread_pool> executor_; | ||
|
|
||
| boost::asio::thread_pool &GetServerCallExecutor() { |
There was a problem hiding this comment.
use Meyer's singleton?
https://laristra.github.io/flecsi/src/developer-guide/patterns/meyers_singleton.html
There was a problem hiding this comment.
boost::asio::thread_pool &GetServerCallExecutor() {
static boost::asio::thread_pool thread_pool{::RayConfig::instance().num_server_call_thread()};
return thread_pool;
}
scv119
reviewed
Nov 11, 2022
| if (registered_actor_iter != registered_actors_.end()) { | ||
| reply->unsafe_arena_set_allocated_actor_table_data( | ||
| registered_actor_iter->second->GetMutableActorTableData()); | ||
| ptr = google::protobuf::Arena::Create<std::shared_ptr<GcsActor>>( |
There was a problem hiding this comment.
is the shared_ptr or the actual data being created on the Arena though?
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
Signed-off-by: Yi Cheng <[email protected]>
stephanie-wang
added a commit
that referenced
this pull request
Nov 29, 2022
)" This reverts commit b79e5b0.
WeichenXu123
pushed a commit
to WeichenXu123/ray
that referenced
this pull request
Dec 19, 2022
…#30131) Right now, GCS/Raylet only has one thread running there. That thread is likely to become a bottleneck when load increased. For request like kv, it's really cheap, but the RPC overhead is actually big compared with the cheap operations. This potentially can cost a lot of issues and we only have one thread in the GCS/Raylet which makes the things worse. Before moving to multi-threading GCS/Raylet, one thing we can do is to execute Finish in a dedicated thread pool. Finish did a lot of things, like serialize the message which might be expensive. And Finish is easily to be offloaded from the main thread, so we can get a lot of gains. Signed-off-by: Weichen Xu <[email protected]>
WeichenXu123
pushed a commit
to WeichenXu123/ray
that referenced
this pull request
Dec 19, 2022
…-project#30131)" (ray-project#30737) This reverts commit b79e5b0. Signed-off-by: Weichen Xu <[email protected]>
tamohannes
pushed a commit
to ju2ez/ray
that referenced
this pull request
Jan 25, 2023
…-project#30131)" (ray-project#30737) This reverts commit b79e5b0. Signed-off-by: tmynn <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Right now, GCS/Raylet only has one thread running there. That thread is likely to become a bottleneck when load increased.
For request like kv, it's really cheap, but the RPC overhead is actually big compared with the cheap operations.
This potentially can cost a lot of issues and we only have one thread in the GCS/Raylet which makes the things worse.
Before moving to multi-threading GCS/Raylet, one thing we can do is to execute Finish in a dedicated thread pool.
Finish did a lot of things, like serialize the message which might be expensive. And Finish is easily to be offloaded from the main thread, so we can get a lot of gains.
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.