You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2014/10/09 03:35:13 UTC
Review Request 26476: Remove dynamic allocation from Option.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/
-----------------------------------------------------------
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
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Oct. 9, 2014, 4:41 a.m., Jie Yu wrote:
> > Flying by. You may wanna take a look at:
> > https://github.com/facebook/folly/blob/master/folly/Optional.h
> >
> > Not sure if we can use unstricted union? Does g++44 supports that?
According to https://gcc.gnu.org/projects/cxx0x.html unrestricted unions are supported from gcc4.6+. In-place new is an old c-ism.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review55951
-----------------------------------------------------------
On Oct. 9, 2014, 1:35 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 1:35 a.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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review55951
-----------------------------------------------------------
Flying by. You may wanna take a look at:
https://github.com/facebook/folly/blob/master/folly/Optional.h
Not sure if we can use unstricted union? Does g++44 supports that?
- Jie Yu
On Oct. 9, 2014, 1:35 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 1:35 a.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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review55944
-----------------------------------------------------------
Patch looks great!
Reviews applied: [26476]
All tests passed.
- Mesos ReviewBot
On Oct. 9, 2014, 1:35 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 1:35 a.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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review56798
-----------------------------------------------------------
High-level: LGTM
Have a couple of nits and we should be good to go.
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/26476/#comment97221>
As it's is a boolean expression, we wrap differently: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Boolean_Expressions
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/26476/#comment97219>
t != NULL or isSome()
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/26476/#comment97217>
s/!t/t == NULL/
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/26476/#comment97218>
s/t/t != NULL/
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/26476/#comment97216>
reduce 2 spaces left
- Niklas Nielsen
On Oct. 8, 2014, 6:35 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 6:35 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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Oct. 9, 2014, 3:45 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp, line 131
> > <https://reviews.apache.org/r/26476/diff/1/?file=716336#file716336line131>
> >
> > This makes Option arbitrarily large which could be problematic where we copy it (we can't assume move semantics).
> >
> > I don't understand the benefit of this change. We have so many dynamic allocations throughout the code-base, it seems like a strange place to focus attention.
In the original implementation of Option, a large T would still be deep copied; it would just be done on the heap. To avoid large copies one should use a reference counted abstraction such as shared_ptr (e.g. Option<std::shared_ptr<T>> or std::shared_ptr<Option<T>>). Option is meant to augment a type with 1 extra bit of (nullable / unknownable, whichever you prefer) state.
Tackling Option is one way of reducing a significant number of dynamic allocations as it is a heavily used library. Option is also something that is commonly assumed to be a light-weight abstraction; so it is less of a surprise if it doesn't have an underlying dynamic allocation.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review55945
-----------------------------------------------------------
On Oct. 9, 2014, 1:35 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 1:35 a.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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review55945
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/26476/#comment96309>
std::move is not in the C++11 tests in configure.ac so it cannot yet be used.
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/26476/#comment96310>
This makes Option arbitrarily large which could be problematic where we copy it (we can't assume move semantics).
I don't understand the benefit of this change. We have so many dynamic allocations throughout the code-base, it seems like a strange place to focus attention.
- Dominic Hamon
On Oct. 8, 2014, 6:35 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 6:35 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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Cody Maloney <co...@mesosphere.io>.
> On Oct. 9, 2014, 2:05 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp, line 131
> > <https://reviews.apache.org/r/26476/diff/1/?file=716336#file716336line131>
> >
> > std::unique_ptr would also be an option as it can be moved on Option copy. Would that be a less intrusive change?
That would effectively make Option a pointer to a pointer... Better would be to do something like:
```
class Option : std::unique_ptr {
...
}
```
But overall making the optional thing live in place is preferable. In the mesos codebase there aren't any really large objects (Objects with a lot of members, or large in-place array members).
Anything that grows / tends to get really large (We move about some big strings), all have external storage from the base class, and this object stays small.
In terms of perf, linear copies of an object in place are fast, and practically copying around the object a whole lot is waayyy faster than if we have a single cache miss looking up the pointer. If you want a real world example of this, std::vector is almost always faster than std::list, unless you have very large objects (http://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html)
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review55977
-----------------------------------------------------------
On Oct. 9, 2014, 1:35 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 1:35 a.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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review55977
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp
<https://reviews.apache.org/r/26476/#comment96328>
std::unique_ptr would also be an option as it can be moved on Option copy. Would that be a less intrusive change?
- Dominic Hamon
On Oct. 8, 2014, 6:35 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 6:35 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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> 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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review56846
-----------------------------------------------------------
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.
- Ben Mahler
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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review57910
-----------------------------------------------------------
Patch looks great!
Reviews applied: [26476]
All tests passed.
- Mesos ReviewBot
On Oct. 22, 2014, 7:34 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 7:34 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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/
-----------------------------------------------------------
(Updated Nov. 6, 2014, 7:13 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Add the JIRA issue for this.
Bugs: MESOS-1991
https://issues.apache.org/jira/browse/MESOS-1991
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
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review60162
-----------------------------------------------------------
Seems this one got stuck for a while now. The reasons appear to be founded in the doubts expressed in the above reviews. I would highly recommend creating a JIRA (if there is none yet), find a shepherd and document the above concerns and your argumentation in such JIRA. These points would most likely also form a great base for further optimizing in similar cases.
- Till Toenshoff
On Oct. 22, 2014, 7:34 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26476/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 7:34 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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/
-----------------------------------------------------------
(Updated Oct. 22, 2014, 7:34 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Make reset private.
Repository: mesos-git
Description
-------
Remove dynamic allocations from Option class.
Diffs (updated)
-----
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
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26476/#review56876
-----------------------------------------------------------
Patch looks great!
Reviews applied: [26476]
All tests passed.
- Mesos ReviewBot
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
>
>
Re: Review Request 26476: Remove dynamic allocation from Option.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
fix style issues.
Repository: mesos-git
Description
-------
Remove dynamic allocations from Option class.
Diffs (updated)
-----
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