> On Oct. 16, 2014, 12:23 a.m., Ben Mahler wrote: > > A few higher level questions: > > > > (1) What motivated this? Concretely, which performance aspect of Mesos is > > this improving? In the past, we eliminated copies of our > > Option,Try,Future,Result family because we found that the copying of large > > Option,Try,Future,Result<Registry> types was degrading performance. In > > general there should be a well understood benefit, especially when we're > > increasing the esotericism of the code in the name of performance. :) > > > > (2) When is this better? When is this worse? It looks like None() options > > are now more expensive? Did you measure Option performance with any > > benchmarks? > > > > (3) Curious why you introduce the new public reset() method, since most > > callers use `option = None()`. Would be great to avoid introducing another > > way to do it.
1. The motivation for this is 3-fold: a. Reduce dynamic allocations. These can cause latency jitter as process lifetime grows. This kind of jitter can make it hard to grasp the upper bound of latency on certain operations under locks. This modification only moves the allocated space of T, it does not reduce or increase the number of actual construction / move calls unless the new move constructor is used. b. The commonly understood implication of Optional / Option / Nullable is that it augments the type field by 1 bit in order to allow representation of an unknown or null state. This is handy in cases where a type such as int64_t fully utilizes its 64 bit storage space, and representing unknown would otherwise require us to steal a number (such as INT64_MAX). This class should not take on the additional responsibility of managing memory for the augmented type. c. It can be very deceptive to a newcomer when Option<int64_t> does a dynamic allocation. Intuitively you would not expect a type such as int64_t to do a dynamic allocation or be expensive to copy. Naturally Option<BigType> would be expected to be expensive to copy, and so a developer would be more inclined to do something like std::shared_ptr<Option<BigType>>. 2. This change has the biggest impact for Options of small types such as Option<int> or Option<SmallStruct>. The stack allocation vs. dynamic allocation for small objects can be a 2-3 orders of magnitude cost difference. This is worse when have Options of large types such as Option<MegabyteBuffer>; but only in that this *could* be allocated on the stack; it is expected that one not do so (and rather use indirection around or inside the Option). You are correct that None() options are now more expensive, but only in memory size (and only when the type is larger than 4 bytes). In my experience we do not allocate a large amount of options on a single stack (if we had a large collection of them they would be in some container that is itself dynamically allocated). I do not have a benchmark for this specific change, although I have done it in the past for other projects. I can write one if you like. 3. I will make the reset() function private :-) - Joris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26476/#review56846 ----------------------------------------------------------- On Oct. 15, 2014, 9:29 p.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26476/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2014, 9:29 p.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > Remove dynamic allocations from Option class. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 47fe92c > > Diff: https://reviews.apache.org/r/26476/diff/ > > > Testing > ------- > > make check > support/mesos-style.py > valgrind (reduced allocation count) > > > Thanks, > > Joris Van Remoortere > >
