You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2018/08/06 10:30:24 UTC
Review Request 68224: Augmented `Statistics` to work with any
collection.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/
-----------------------------------------------------------
Review request for mesos, Benno Evers and Benjamin Mahler.
Repository: mesos
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
Diff: https://reviews.apache.org/r/68224/diff/1/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/#review207045
-----------------------------------------------------------
Overall looks good, just a couple of minor comments below:
3rdparty/libprocess/include/process/statistics.hpp
Lines 32-36 (original), 34-39 (patched)
<https://reviews.apache.org/r/68224/#comment290229>
It would be great to have a TODO here to remove the time series specialization by having time series use an iterator adaptor (e.g. https://www.boost.org/doc/libs/1_51_0/libs/range/doc/html/range/reference/adaptors/reference/map_values.html)
3rdparty/libprocess/include/process/statistics.hpp
Lines 60-64 (patched)
<https://reviews.apache.org/r/68224/#comment290224>
We should just have the caller do this using an iterator adaptor (e.g. they have a `transform` one akin to what you're suggesting here, but they also have one that directly provides map values: https://www.boost.org/doc/libs/1_51_0/libs/range/doc/html/range/reference/adaptors/reference/map_values.html) rather than push it down into the library
3rdparty/libprocess/include/process/statistics.hpp
Lines 96-97 (patched)
<https://reviews.apache.org/r/68224/#comment290225>
Let's take it by rvalue reference here and std::move it in?
Alternatively, we can call this `fromSorted`, take it by const ref, and require that the caller sort, but there's a cost to checking that it's indeed sorted if we wanted to be safe here
3rdparty/libprocess/src/tests/statistics_tests.cpp
Lines 94 (patched)
<https://reviews.apache.org/r/68224/#comment290226>
It's tough to tell if this is the right p90, could we update the data to make it more obviously right? E.g. p90 -> 90
- Benjamin Mahler
On Aug. 9, 2018, 2:20 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2018, 2:20 p.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
> 3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
>
>
> Diff: https://reviews.apache.org/r/68224/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/#review207037
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/include/process/statistics.hpp
Lines 75 (patched)
<https://reviews.apache.org/r/68224/#comment290199>
Let's verify that iterators can be re-used after computing the distance:
```
static_assert(!std::is_same<
typename std::iterator_traits<It>::iterator_category,
std::input_iterator_tag>::value)
```
(Or even add another `enable_if` to the signature, if you prefer)
- Benno Evers
On Aug. 9, 2018, 2:20 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2018, 2:20 p.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
> 3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
>
>
> Diff: https://reviews.apache.org/r/68224/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 22, 2018, 2:30 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/statistics.hpp
> > Line 108 (original), 148 (patched)
> > <https://reviews.apache.org/r/68224/diff/3/?file=2072396#file2072396line161>
> >
> > Whoops? Can you remove this stray from the diff?
Nope, it participates in type resolution : )
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/#review207724
-----------------------------------------------------------
On Aug. 14, 2018, 12:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2018, 12:48 p.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
> 3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
>
>
> Diff: https://reviews.apache.org/r/68224/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/#review207724
-----------------------------------------------------------
Fix it, then Ship it!
Looks good!
3rdparty/libprocess/include/process/statistics.hpp
Lines 61-71 (patched)
<https://reviews.apache.org/r/68224/#comment291216>
It would be great to send this to mpark on slack to get some feedback, as I'd love to use it in many other places (e.g. process::collect/await). Looks ok to me though as is (albeit a tough read!).
3rdparty/libprocess/include/process/statistics.hpp
Line 108 (original), 148 (patched)
<https://reviews.apache.org/r/68224/#comment291215>
Whoops? Can you remove this stray from the diff?
3rdparty/libprocess/src/tests/statistics_tests.cpp
Lines 94-95 (patched)
<https://reviews.apache.org/r/68224/#comment291214>
Nice!
- Benjamin Mahler
On Aug. 14, 2018, 12:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2018, 12:48 p.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
> 3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
>
>
> Diff: https://reviews.apache.org/r/68224/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/
-----------------------------------------------------------
(Updated Aug. 14, 2018, 12:48 p.m.)
Review request for mesos, Benno Evers and Benjamin Mahler.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
Diff: https://reviews.apache.org/r/68224/diff/3/
Changes: https://reviews.apache.org/r/68224/diff/2-3/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/
-----------------------------------------------------------
(Updated Aug. 9, 2018, 2:20 p.m.)
Review request for mesos, Benno Evers and Benjamin Mahler.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
Diff: https://reviews.apache.org/r/68224/diff/2/
Changes: https://reviews.apache.org/r/68224/diff/1-2/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 6, 2018, 10:59 a.m., Benno Evers wrote:
> > 3rdparty/libprocess/include/process/statistics.hpp
> > Lines 60 (patched)
> > <https://reviews.apache.org/r/68224/diff/1/?file=2068399#file2068399line65>
> >
> > Aren't you missing a `::value` after `std::is_same<>`, assuming you're going for SFINAE here?
> >
> > Also, what's the reason for being so strict? Shouldn't `std::is_convertible<>` be enough?
`std::enable_if` was indeed missing. Thanks!
> On Aug. 6, 2018, 10:59 a.m., Benno Evers wrote:
> > 3rdparty/libprocess/include/process/statistics.hpp
> > Lines 68 (patched)
> > <https://reviews.apache.org/r/68224/diff/1/?file=2068399#file2068399line73>
> >
> > I wonder if we should just reserve `std::distance(first, last)` if we don't get `InputIterator`s.
> >
> > Intuitively it feels like even in the worst case, doing an additional iteration should be faster than the additional memory allocations.
Yeah, we can reserve always.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/#review206875
-----------------------------------------------------------
On Aug. 9, 2018, 2:20 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2018, 2:20 p.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
> 3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
>
>
> Diff: https://reviews.apache.org/r/68224/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/#review206875
-----------------------------------------------------------
3rdparty/libprocess/include/process/statistics.hpp
Lines 60 (patched)
<https://reviews.apache.org/r/68224/#comment289972>
Aren't you missing a `::value` after `std::is_same<>`, assuming you're going for SFINAE here?
Also, what's the reason for being so strict? Shouldn't `std::is_convertible<>` be enough?
3rdparty/libprocess/include/process/statistics.hpp
Lines 68 (patched)
<https://reviews.apache.org/r/68224/#comment289973>
I wonder if we should just reserve `std::distance(first, last)` if we don't get `InputIterator`s.
Intuitively it feels like even in the worst case, doing an additional iteration should be faster than the additional memory allocations.
3rdparty/libprocess/src/tests/statistics_tests.cpp
Lines 28 (patched)
<https://reviews.apache.org/r/68224/#comment289974>
This is adding more characters than just writing out the namespace in the test ;)
- Benno Evers
On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 10:30 a.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
> 3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
>
>
> Diff: https://reviews.apache.org/r/68224/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 7, 2018, 11:46 p.m., Benjamin Mahler wrote:
> > Can we just have a single function instead of keeping the specialized TimeSeries overload? At some point we're going to have to do the copy elimination here so we'll probably take random access iterators and index directly rather than copying into a vector.
Of course we can, but this will require adding iterator suport for `TimeSeries` which is orthogonal to this patch. We can't eliminate copying data to the internal vector because we have to modify it (sort).
If `TimeSeries` had iterator support, we could simply have had the following:
```
template <typename It, typename Accessor>
static Option<Statistics<T>> from(It first, It last, Accessor accessor)
{
std::vector<T> values;
values.reserve(std::distance(first, last));
std::transform(
first, last, values.begin(), std::back_inserter(values), accessor);
return from(values);
}
```
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/#review206961
-----------------------------------------------------------
On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 10:30 a.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
> 3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
>
>
> Diff: https://reviews.apache.org/r/68224/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68224: Augmented `Statistics` to work with any
collection.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68224/#review206961
-----------------------------------------------------------
Can we just have a single function instead of keeping the specialized TimeSeries overload? At some point we're going to have to do the copy elimination here so we'll probably take random access iterators and index directly rather than copying into a vector.
- Benjamin Mahler
On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 10:30 a.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/statistics.hpp e9f1fc23bf83f92a2e7de94dba0df48272cc3394
> 3rdparty/libprocess/src/tests/statistics_tests.cpp a2a780bf9de018c823b68aa48977fd9fd1b8a064
>
>
> Diff: https://reviews.apache.org/r/68224/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>