Skip to content
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.

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

[core] Remove grpc client thread number #49237

Merged

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Dec 12, 2024

This PR resolves #49240

From my understanding:

  • Threads are allocated at server side, by default each grpc request is handled by a different thread, coming out of a threadpool; the quota used in the grpc client constructor caps the max num
  • On client side, apart from the polling thread (all sync grpc calls are built upon async one, which bakes an eventloop in a polling thread), I'm not aware of any conditions which require extra threads inside of client library, since it leverages http2 and io multiplex underlying

Performance test commit: d631f40
Performance dashboard: https://b534fd88.us1a.app.preset.io/superset/dashboard/19/?native_filters_key=LosP6LvuIiFtoQC_7slPcR-99euapUl2IU9rKLMjMntJgZz6LtYnyGly9fhI5oJ1
Average perf metrics value for latency: 16.832991340999996

Release test for this branch: https://buildkite.com/ray-project/release/builds/27984#0193bd0f-098b-4df9-9504-e23263a6b30f
Perf latency metrics for this PR: 14.782282467999991

@dentiny dentiny changed the title remove grpc client thread number [core] Remove grpc client thread number Dec 12, 2024
@dentiny dentiny requested review from jjyao and dayshah December 12, 2024 23:48
@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Dec 12, 2024

channel_ = BuildChannel(address, port, argument);
stub_ = GrpcService::NewStub(channel_);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

change places where this constructor is used too? Are the other uses casting num_threads to bool rn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't find it used anywhere via simple grep, but I will wait for CI results.

Copy link
Contributor

Choose a reason for hiding this comment

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

grpc_clients_.emplace_back(new GrpcClient<ObjectManagerService>(
address, port, client_call_manager, num_connections_));
this seems to be the constructor used here, i think now it's just using one above this one instead and implicitly coverts the int to a bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we make sure we removed all usages of this constructor and didn't accidentally call the one above by converting int to bool?

Copy link
Contributor Author

@dentiny dentiny Dec 16, 2024

Choose a reason for hiding this comment

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

I made an extra test PR: #49283, which throws exception on the grpc client overload removed in this PR; and it turns out all CI tests pass.

@dentiny dentiny requested a review from dayshah December 13, 2024 01:46
@dentiny dentiny changed the title [core] Remove grpc client thread number [core] Remove unused grpc client constructor Dec 13, 2024
@dentiny dentiny changed the title [core] Remove unused grpc client constructor [core] Remove grpc client thread number Dec 13, 2024
Signed-off-by: hjiang <[email protected]>
@jjyao jjyao merged commit 361b7b4 into ray-project:master Dec 17, 2024
5 checks passed
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Dec 17, 2024
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.

Investigate grpc client thread number
3 participants