[core] change all callbacks to move to save copies. #46971
Conversation
what about just microbenchmark release tests? |
|
started the microbenchmark |
|
Here is the result for microbenchmark (BEFORE = ray 2.22.0, AFTER = this PR, higher is better) The numbers are persistent across runs (assume all microbenchmarks are the same thing). Notably, there's significant improvement (40%) for @rkooo567 do you have a gut feeling on why there are regressions and how we may save it? |
rynewang
requested review from
hongpeng-guo,
justinvyu,
matthewdeng,
raulchen,
woshiyyya,
ericl,
scv119,
c21,
amogkam,
scottjlee,
bveeramani,
stephanie-wang,
omatthew98,
a team,
richardliaw and
edoakes
as code owners
Signed-off-by: Ruiyang Wang <[email protected]> fix cpp test Signed-off-by: Ruiyang Wang <[email protected]> more moves Signed-off-by: Ruiyang Wang <[email protected]> change all callbacks to move to save copies Signed-off-by: Ruiyang Wang <[email protected]> move in cython (unfortunately not 0 copy) Signed-off-by: Ruiyang Wang <[email protected]>
rynewang
force-pushed
the
move-callbacks
branch
from
412bfd3
to
3cd6f81
Compare
rynewang
removed request for
a team,
ericl,
scv119,
stephanie-wang,
raulchen and
justinvyu
rynewang
removed request for
matthewdeng,
richardliaw,
c21,
scottjlee,
amogkam,
edoakes,
hongpeng-guo,
omatthew98,
bveeramani and
woshiyyya
|
Latest PR, similar gains, same
|
|
Can we check why some non-client tests have regressed? |
Signed-off-by: Ruiyang Wang <[email protected]>
|
looks like the numbers that are fluctuating < +-10% are flaky. I suggest we first merge and wait for several day's numbers to see if there are consistent regressions. |
| auto copied = reply; | ||
| callback(status, std::move(copied)); |
There was a problem hiding this comment.
Is it possible to avoid the manual copy by supporting both
callback(status, reply) // The reply is still needed, don't move, make a copy
and
callback(status, std::move(reply)) // The reply is no longer needed, can move
There was a problem hiding this comment.
I think the purpose is to force a move. If we support both const& and &&, we can always forget to move and accidentally make copies. So I'd stay with explicit copies.
We have many callbacks in gRPC async handling, each of them takes a shape `std::function<void(Status, const T&)>`. Notice the const ref may be later copied again into caller's data structures, some times multiple times for nested callbacks. Changes the signature to pass by rvalue reference (T&&) to save copies. This involves GcsClient, RayletClient and GcsSubscriber callers. This should work well since almost all replies are used only once, including the SubscriberCallback, except for `NodeInfoAccessor::HandleNotification` which both saves it to `node_cache_` and passes it to the listeners, where we have to make a copy. It's a pity we don't have a one-click performance test to verify gains of this PR. Signed-off-by: Ruiyang Wang <[email protected]> Signed-off-by: Dev <[email protected]>
We have many callbacks in gRPC async handling, each of them takes a shape
std::function<void(Status, const T&)>. Notice the const ref may be later copied again into caller's data structures, some times multiple times for nested callbacks.Changes the signature to pass by rvalue reference (T&&) to save copies. This involves GcsClient, RayletClient and GcsSubscriber callers. This should work well since almost all replies are used only once, including the SubscriberCallback, except for
NodeInfoAccessor::HandleNotificationwhich both saves it tonode_cache_and passes it to the listeners, where we have to make a copy.It's a pity we don't have a one-click performance test to verify gains of this PR.