> On Jan. 14, 2016, 9:03 a.m., Benjamin Bannier wrote: > > Was the decision to optimize here driven by profiling? > > > > Given the proposed implementation I wonder if this was worth the risk. I > > can only see this being called for single element `queues` of `Events` > > which should not be too expensive to copy, especially given how little we > > worry about e.g., creating temporary `strings` elsewhere.
My main motivation for making the change was driven by the semantics of how the callback function processes the data here: ``` void received(queue<Event> events) { while (!events.empty()) { Event event = events.front(); events.pop(); switch (event.type()) { } } ``` Since, the callback function always has to invoke a `.pop()`. The argument `queue<Event> events` has to be passed on by value. So, we were effectively, copying the same object twice. Also, after performing the `move`, we were invoking the assignment operation that did not have a precondition associated with it for `std::queue` and hence seemed safe to make the change. But, the queue can at best have a few events (at max). In most cases being `== 1` as you pointed out, except for the cases where the callback function takes some time and we queued more events from the master in the interim. I am discarding this for now. We can always revisit this later. - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42273/#review114441 ----------------------------------------------------------- On Jan. 13, 2016, 11:06 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42273/ > ----------------------------------------------------------- > > (Updated Jan. 13, 2016, 11:06 p.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > This trivial fix invokes `std::move` on the `queue` object to not perform a > copy before invoking the `async(..)` call. > > > Diffs > ----- > > src/scheduler/scheduler.cpp a17872b46ec600e0fae6c43247ccb63f5ee55ac0 > > Diff: https://reviews.apache.org/r/42273/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >