Avoid unnecessary copies in PromiseStream #2915
Conversation
|
This is great. Do you have data on how much performance benefit this gives us? |
|
I see some test failures, e.g., Can you take a look? |
flow/flow.h
Outdated
| @@ -1009,6 +1013,9 @@ struct ActorSingleCallback : SingleCallback<ValueType> { | |||
| virtual void fire(ValueType const& value) { | |||
| static_cast<ActorType*>(this)->a_callback_fire(this, value); | |||
| } | |||
| virtual void fire(ValueType &&value) { | |||
There was a problem hiding this comment.
I don't think this has an effect currently since the callbacks generated by the actor compiler all take const references. Would you be ok with removing this for this PR and possibly including it in a followup that includes the actor compiler changes?
There was a problem hiding this comment.
This change is necessary for this PR. a_callback_fire takes a parameter by value, not by const reference.
|
@jzhou77 I can't seem to replicate those failures locally. passes for me. Do you know which unit test this is running for you? |
|
did you build in the docker image? Simulation tests sometimes don't reproduce. @jzhou77 did you build with make or cmake? |
|
I built outside of docker, with cmake. |
The error is here: I think it's probably built with |
|
Can you please symbolize the stack trace for us? Maybe we can find the issue without rebuilding with |
Unfortunately, I used Jenkins to run correctness, which doesn't keep debug symbols around. |
|
Thanks @jzhou77. |
|
Since this PR was approved, @atn34 found some additional optimizations which I've now added. So we should probably review again. |
I've run into this before when helping Senthil and Neelam with correctness runs. As all of the Apple human and infra done builds are done within the build docker image, to reproduce the failures, you need to build and run fdbserver within the build docker image. |
|
@xumengpanda We tested this with the Mako benchmark, and saw noticeable performance improvements:
|
This is awesome! |
| @@ -247,6 +247,15 @@ class RequestStream { | |||
| else | |||
| queue->send(value); | |||
| } | |||
|
|
|||
| void send(T&& value) const { | |||
There was a problem hiding this comment.
So, I think that this should work out that template<class T> T&& is a forwarding reference, so you can merge the const T& value and T&& cases into only the T&& case, and then use std::forward<T>(value) instead of std::move(value).
However, it's been... a while since I've had to look this up, so I'm going to merge this PR anyway, and if it turns out that we can shave some code off later, we can do that in a second PR.
There was a problem hiding this comment.
So, I think that this should work out that
template<class T> T&&is a forwarding reference
I think since T is not a template type parameter of send, value is not a forwarding reference. Here's an example from forwarding reference
template<class T> struct A {
template<class U>
A(T&& x, U&& y, int* p); // x is not a forwarding reference: T is not a
// type template parameter of the constructor,
// but y is a forwarding reference
};
No description provided.