mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Cody Maloney" <c...@mesosphere.io>
Subject Re: Review Request 31700: Reserved memory for JSON arrays where appropriate.
Date Wed, 11 Mar 2015 23:05:51 GMT


> On March 11, 2015, 9:38 p.m., Ben Mahler wrote:
> > src/master/http.cpp, line 471
> > <https://reviews.apache.org/r/31700/diff/2/?file=888170#file888170line471>
> >
> >     Do you know if a move is helpful here or if the compiler is already optimizing
this?
> >     
> >     ```
> >     object.values["slaves"] = std::move(array);
> >     ```
> >     
> >     Don't do it in this change, we need to measure and I'm just curious :)
> 
> Alexander Rukletsov wrote:
>     I believe the compiler won't optimize this, since `array` is an lvalue and may be
used after this assignment. One thing how we can try to persuade the compiler to move without
writing `std::move()` is to wrap array population in a function that returns an array, this
will be a rvalue then and c++11-capable compiler *should* choose the right overload, that
takes a rvalue ref: http://www.cplusplus.com/reference/map/map/insert/.
>     
>     But everything more complex than `.reserve()` should be benchmarked, on all official
supported compilers if possible. I'll play with this when I have some free cycles.
> 
> Cody Maloney wrote:
>     From writing my own JSON code, the std::move here is definitely necessary to do this
as quickly and cheaply as possible.
>     
>     Using std::move here I think is actually the right thing to do, rather than trying
to convince the compiler via other means, it's explicitly saying to the reader "This variable
you were using earlier, it is having it's contents ripped out of it".
>     
>     Putting it into a function and guaranteeing NRVO or RVO fire so that you get the
"cheap" move insertion has a lot of things people can disturb only slightly and end up breaking
it.

Updating the instance in <stout/protobuf.hpp> where the JSON::Array is copied to be
a move would also probably help significantly.

The way JSON::Protobuf's api is setup with Operator() forces a copy of the JSON::Object after
it is fully constructed which is rather expensive. Would be good to make JSON::Protobuf()
just be a function which can then use the object internally, and move out the result rather
than forcing the full object copy.


- Cody


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76133
-----------------------------------------------------------


On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31700/
> -----------------------------------------------------------
> 
> (Updated March 11, 2015, 10:40 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2353
>     https://issues.apache.org/jira/browse/MESOS-2353
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9 
>   src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24 
> 
> Diff: https://reviews.apache.org/r/31700/diff/
> 
> 
> Testing
> -------
> 
> make check (OS X 10.9.5, Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message