Re: Review Request 26476: Remove dynamic allocation from Option.
--- 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.
--- 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.
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,ResultRegistry 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 Optionint64_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 OptionBigType would be expected to be expensive to copy, and so a developer would be more inclined to do something like std::shared_ptrOptionBigType. 2. This change has the biggest impact for Options of small types such as Optionint or OptionSmallStruct. 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 OptionMegabyteBuffer; 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.
--- 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.
--- 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.
--- 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.
--- 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
Re: Review Request 26476: Remove dynamic allocation from Option.
--- 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,ResultRegistry 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.
--- 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.
--- 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.
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.
--- 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.
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. Optionstd::shared_ptrT or std::shared_ptrOptionT). 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.
--- 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.
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
