Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 23, 2015, 6:29 p.m., Ben Mahler wrote:
> > Where is the benchmark for this change? :(
> 
> Joerg Schad wrote:
> We benchmarked the code with a proprietary benchmark. I added 
> https://issues.apache.org/jira/browse/MESOS-3502 aiming at adding an 
> open-source benchmark for this.
> 
> Alexander Rukletsov wrote:
> Ben, I'm with you regarding benchmarking this change. But we thougt about 
> this patch as a hotfix improvement, which makes the behavior not worse than 
> the status quo. Basically, Jörg removed extra memory allocations and cleaned 
> up deletions from a linear array.
> 
> I think we still need a deeper refactoring here (similar to 
> `state.json`), with caching intermediate results or other ideas that can 
> drastically speed up the performance. This will be a bigger project, which 
> will definitely require a thorough benchmark.

FYI The benchmark has been published via MESOS-3493.


- Joerg


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


On Sept. 23, 2015, 12:37 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 23, 2015, 12:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Alexander Rukletsov]

> On Sept. 23, 2015, 6:29 p.m., Ben Mahler wrote:
> > Where is the benchmark for this change? :(
> 
> Joerg Schad wrote:
> We benchmarked the code with a proprietary benchmark. I added 
> https://issues.apache.org/jira/browse/MESOS-3502 aiming at adding an 
> open-source benchmark for this.

Ben, I'm with you regarding benchmarking this change. But we thougt about this 
patch as a hotfix improvement, which makes the behavior not worse than the 
status quo. Basically, Jörg removed extra memory allocations and cleaned up 
deletions from a linear array.

I think we still need a deeper refactoring here (similar to `state.json`), with 
caching intermediate results or other ideas that can drastically speed up the 
performance. This will be a bigger project, which will definitely require a 
thorough benchmark.


- Alexander


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


On Sept. 23, 2015, 12:37 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 23, 2015, 12:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 9, 2015, 3:46 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 250-271
> > 
> >
> > I think we can conflate this two cases, how about this:
> > ```
> > if (range->end() + 1 >= current.begin()) {
> >   range->set_end(std::max(range.end(), current.end()));
> >   deleteSet.insert(y);
> >   ++i;
> > } else {
> >   break;
> > }
> > ```
> 
> Joerg Schad wrote:
> Yes we could do this. I personally find the 2 distinct cases (especially 
> with the range comments) more readable.
> 
> Alexander Rukletsov wrote:
> I think we can also add range comments here to make it more readable : ). 
> Usually less branches and less code means faster execution. However, you (and 
> your shepherd) decide, means you can drop the issue.

refactored


- Joerg


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


On Sept. 23, 2015, 12:37 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 23, 2015, 12:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 23, 2015, 6:29 p.m., Ben Mahler wrote:
> > Where is the benchmark for this change? :(

We benchmarked the code with a proprietary benchmark. I added 
https://issues.apache.org/jira/browse/MESOS-3502 aiming at adding an 
open-source benchmark for this.


- Joerg


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


On Sept. 23, 2015, 12:37 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 23, 2015, 12:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Ben Mahler]

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


Where is the benchmark for this change? :(

- Ben Mahler


On Sept. 23, 2015, 12:37 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 23, 2015, 12:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Michael Park]

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

Ship it!


I've submitted this review request with the following minor changes, and 
removed some of the `std::` qualifications which were unnecessary.


src/common/values.cpp (line 23)


Removed ``, ``
Added ``, ``, `` ``, ``.



src/common/values.cpp (line 110)


`{` on a newline.



src/common/values.cpp (line 120)


Renamed `lhs`, `rhs` to `left`, `right`.



src/common/values.cpp (lines 162 - 163)


```
result->mutable_range()->DeleteSubrange(
count, result->range_size() - count);
```



src/common/values.cpp (line 188)


```
void coalesce(
Value::Ranges* result,
std::initializer_list addedRanges)
{
```



src/common/values.cpp (line 221)


(1) `{` on the newline.
(2) Renamed `lhs`, `rhs` to `result`, `addedRange`


- Michael Park


On Sept. 23, 2015, 12:37 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 23, 2015, 12:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 23, 2015, 12:37 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 23, 2015, 12:37 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 23, 2015, 12:37 p.m.)


Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Michael Park]

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



src/common/values.cpp (line 253)


`s/for/foreach/


- Michael Park


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Michael Park]

> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote:
> > src/common/values.cpp, line 204
> > 
> >
> > Why not pass by value?
> 
> Joerg Schad wrote:
> Not sure why you would want to pass either by value. Result we want to 
> use for the output and the ranges vector I also wouldn't copy.

Sorry, I was referring to the `ranges` vector here. I'm asking why not pass by 
value for the ranges vector because it's a stricter requirement than necessary 
for the input to be an rvalue.
In terms of the API of this function, what we care about is whether we have our 
own `ranges` that we can modify and such.
Binding with rvalue-ref is one way to do it, but as I mentioned, a stricter 
requirement than necessary. By taking it by value, if an actual temporary is 
passed the entire thing is optimized out,
if a `std::move`d variable is passed it's moved, and if an lvalue is passed it 
gets copied. I think these are the exact semantics we want within the scope of 
this function, and it's upto the caller to decide what to pass.


- Michael


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


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Michael Park]

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



src/common/values.cpp (lines 93 - 94)


`{` on the same line.



src/common/values.cpp (lines 204 - 224)


How about we simplify this to:

```cpp
std::vector ranges;
ranges.Reserve(rangesSum);

auto fill = [&ranges](const Value::Ranges& inputs) {
  foreach (const Value::Range& range, inputs) {
ranges.push_back({range.begin(), range.end()});
  }
};

fill(*lhs);
foreach(const Value::Ranges& range, rhs) {
  fill(range);
}
```


- Michael Park


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote:
> > src/common/values.cpp, line 301
> > 
> >
> > Can we just take a `Value::Ranges` here rather than 
> > `initializer_list`?
> > 
> > It looks like the only place we actually use this is in `operator+`, 
> > for which we can just follow the pattern for `operator-`.
> > 
> > ```cpp
> > coalesce(&result, left);
> > return result += result;
> > ```
> > 
> > I think it would also clean up this code significantly, as we wouldn't 
> > need the `rangesSum` loop, The `fill` function wouldn't have to be factored 
> > out, wouldn't need the `offset` math, etc.
> 
> Joerg Schad wrote:
> Would like to avoid coalescing twice, but couldnt we do something along 
> the lines 
> ```c++
> void coalesce(Value::Ranges* result, Value::Ranges ranges) {
>   size_t rangesSum = result->range_size() + ranges->ranges_size();
> ```
> 
> In that case I would probably leave fill factored out, but the rest 
> (rangesSum, offset) would be simplified.
> 
> Joerg Schad wrote:
> Na doesn't really help, will add a more detailed discussion of tradeoffs.

As discussed.


- Joerg


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


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote:
> > src/common/values.cpp, line 301
> > 
> >
> > Can we just take a `Value::Ranges` here rather than 
> > `initializer_list`?
> > 
> > It looks like the only place we actually use this is in `operator+`, 
> > for which we can just follow the pattern for `operator-`.
> > 
> > ```cpp
> > coalesce(&result, left);
> > return result += result;
> > ```
> > 
> > I think it would also clean up this code significantly, as we wouldn't 
> > need the `rangesSum` loop, The `fill` function wouldn't have to be factored 
> > out, wouldn't need the `offset` math, etc.
> 
> Joerg Schad wrote:
> Would like to avoid coalescing twice, but couldnt we do something along 
> the lines 
> ```c++
> void coalesce(Value::Ranges* result, Value::Ranges ranges) {
>   size_t rangesSum = result->range_size() + ranges->ranges_size();
> ```
> 
> In that case I would probably leave fill factored out, but the rest 
> (rangesSum, offset) would be simplified.

Na doesn't really help, will add a more detailed discussion of tradeoffs.


- Joerg


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


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote:
> > src/common/values.cpp, line 204
> > 
> >
> > Why not pass by value?

Not sure why you would want to pass either by value. Result we want to use for 
the output and the ranges vector I also wouldn't copy.


> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote:
> > src/common/values.cpp, lines 264-289
> > 
> >
> > Could we simply `Resize` and not worry about `DeleteSubrange`, 
> > `Reserve`, and `add_range`?
> > 
> > ```cpp
> > result->mutable_range()->Resize(count, {});
> > ```

afaik Resize is only available for RepeatedFields 
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.repeated_field#RepeatedField.Resize.details
 .
Ranges is a RepeatedPtrField which doesn't offer resize.
Please correct me if I am wrong.


> On Sept. 23, 2015, 12:58 a.m., Michael Park wrote:
> > src/common/values.cpp, line 301
> > 
> >
> > Can we just take a `Value::Ranges` here rather than 
> > `initializer_list`?
> > 
> > It looks like the only place we actually use this is in `operator+`, 
> > for which we can just follow the pattern for `operator-`.
> > 
> > ```cpp
> > coalesce(&result, left);
> > return result += result;
> > ```
> > 
> > I think it would also clean up this code significantly, as we wouldn't 
> > need the `rangesSum` loop, The `fill` function wouldn't have to be factored 
> > out, wouldn't need the `offset` math, etc.

Would like to avoid coalescing twice, but couldnt we do something along the 
lines 
```c++
void coalesce(Value::Ranges* result, Value::Ranges ranges) {
  size_t rangesSum = result->range_size() + ranges->ranges_size();
```

In that case I would probably leave fill factored out, but the rest (rangesSum, 
offset) would be simplified.


- Joerg


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


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joris Van Remoortere]

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

Ship it!


After addressing the outstanding issues.

- Joris Van Remoortere


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Michael Park]

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



src/common/values.cpp (lines 141 - 154)


This seems like an awkward `else if` and `else` style. Could we rearrange 
the comments a little to get the common structure back? For example,

```cpp
if (...) {
  ...
} else if (range.start > current.start) {
  // If we are starting farther ahead, then there are 2 cases:
  // Ranges are overlapping and we can merge them.
  if (range.start <= currentEnd + 1) {
...
  } else { // No overlap and we are adding a new range.
...
  }
}



src/common/values.cpp (lines 268 - 269)


This fits in one line


- Michael Park


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Michael Park]

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


I've got some clean-up suggestions, I think `lhs` and `rhs` variables should be 
renamed to `result` and `source` or something along those lines.


src/common/values.cpp (line 45)


`s/operator<< (/operator<<(`



src/common/values.cpp (line 109)


`s/protbuf/protobuf/`



src/common/values.cpp (line 111)


Why not pass by value?



src/common/values.cpp (lines 122 - 123)


How about `return std::tie(lhs.start, lhs.end) < std::tie(rhs.start, 
rhs.end);`?



src/common/values.cpp (lines 129 - 130)


How about `Range current = ranges.front();`,
and using `current.{start,end}` rather than `current{Start,End}`?



src/common/values.cpp (lines 155 - 156)


`ranges[count - 1] = current;` if you take the above suggestion.



src/common/values.cpp (lines 158 - 159)


`current = range;` if you take the above suggestion.



src/common/values.cpp (lines 170 - 186)


Could we simply `Resize` and not worry about `DeleteSubrange`, `Reserve`, 
and `add_range`?

```cpp
result->mutable_range()->Resize(count, {});
```



src/common/values.cpp (line 198)


Can we just take a `Value::Ranges` here rather than 
`initializer_list`?

It looks like the only place we actually use this is in `operator+`, for 
which we can just follow the pattern for `operator-`.

```cpp
coalesce(&result, left);
return result += result;
```

I think it would also clean up this code significantly, as we wouldn't need 
the `rangesSum` loop, The `fill` function wouldn't have to be factored out, 
wouldn't need the `offset` math, etc.


- Michael Park


On Sept. 22, 2015, 8:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 22, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 22, 2015, 8:55 p.m.)


Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 22, 2015, 6:42 p.m.)


Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
Toenshoff.


Changes
---

Refactoring as suggested by Joris.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 0318885336409f7cc9dbd4a3daa9b52db197bbd1 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 18, 2015, 6:39 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 18, 2015, 6:39 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 18, 2015, 6:39 a.m.)


Review request for mesos, Bernd Mathiske, Joris Van Remoortere, and Till 
Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 18, 2015, 5:17 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 18, 2015, 5:17 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 18, 2015, 5:17 a.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Changes
---

removed duplicated line...


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 17, 2015, 11:34 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 15, 2015, 8:46 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 15, 2015, 8:46 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 15, 2015, 8:46 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 10, 2015, 6:15 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 10, 2015, 6:15 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Alexander Rukletsov]

> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 442-444
> > 
> >
> > Let's let compiler do it's job: how about passing `left` by value and 
> > not creating a copy manually?
> 
> Joerg Schad wrote:
> Wanted to keep the (internal) interface consistent with the other 
> operators. Do you have a strong preference here?

I leave it to you and your shepherd. You may drop the issue.


- Alexander


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


On Sept. 10, 2015, 6:15 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 10, 2015, 6:15 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Alexander Rukletsov]

> On Sept. 9, 2015, 3:46 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 250-271
> > 
> >
> > I think we can conflate this two cases, how about this:
> > ```
> > if (range->end() + 1 >= current.begin()) {
> >   range->set_end(std::max(range.end(), current.end()));
> >   deleteSet.insert(y);
> >   ++i;
> > } else {
> >   break;
> > }
> > ```
> 
> Joerg Schad wrote:
> Yes we could do this. I personally find the 2 distinct cases (especially 
> with the range comments) more readable.

I think we can also add range comments here to make it more readable : ). 
Usually less branches and less code means faster execution. However, you (and 
your shepherd) decide, means you can drop the issue.


- Alexander


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


On Sept. 10, 2015, 6:15 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 10, 2015, 6:15 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 10, 2015, 6:15 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Till Toenshoff]

> On Sept. 10, 2015, 2:48 a.m., Till Toenshoff wrote:
> > Looks really good to me, thanks Joerg.

Just noticed the tests fail at a point that seems relevant to this code - 
please make sure you can recreate the buildbot failure and then fix it please :)


- Till


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


On Sept. 9, 2015, 8:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 9, 2015, 8:48 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Till Toenshoff]

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

Ship it!


Looks really good to me, thanks Joerg.


src/common/values.cpp (line 125)


I like this explicit way of calculating the size via a scoped variable.



src/common/values.cpp (lines 149 - 150)


:D



src/common/values.cpp (line 162)


Why this blank line?



src/common/values.cpp (line 179)


US english would be s/neighbouring/neighboring/



src/common/values.cpp (lines 210 - 211)


I think this would look less "noisy":
```
void addRange(
Value::Ranges* ranges, uint64_t begin, uint64_t end, int insertPosition)
```



src/tests/values_tests.cpp (line 37)


Fits a single line:
```
  extern void coalesce(Value::Ranges* ranges, const Value::Range& range);
```



src/tests/values_tests.cpp (line 39)


Fits a single line:
```
  extern void removeRanges(Value::Ranges* ranges, std::set* deleteSet);
```



src/tests/values_tests.cpp (lines 41 - 42)


How about this instead?
```
  extern void addRange(
  Value::Ranges* ranges, uint64_t begin, uint64_t end, int 
insertPosition);
```



src/tests/values_tests.cpp (line 48)


Not yours but IIUC then this is a blank line too many.



src/tests/values_tests.cpp (line 218)


Insert blank please.



src/tests/values_tests.cpp (line 253)


s/Overlap/overlap/



src/tests/values_tests.cpp (line 305)


s/overlap/overlaps/ ?



src/tests/values_tests.cpp (line 319)


Not sure if we should aim for commonwealth or us english on the 
neighbour/neighbor? :)



src/tests/values_tests.cpp (line 326)


s/N/n/



src/tests/values_tests.cpp (line 333)


s/collescing/coalescing/



src/tests/values_tests.cpp (line 340)


see above



src/tests/values_tests.cpp (line 376)


Hah, now you chose the US-english "neighbor" :D



src/tests/values_tests.cpp (line 390)


Quick descriptive comment please.



src/tests/values_tests.cpp (line 430)


Quick descriptive comment please.


- Till Toenshoff


On Sept. 9, 2015, 8:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 9, 2015, 8:48 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 9, 2015, 8:48 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 9, 2015, 3:46 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, line 184
> > 
> >
> > s/Remove/Removes
> > s/indexes/indices
> > 
> > Mind refining a comment a bit? It looks like it evolved during time and 
> > contains artifacts from previous versions : ).

FYI both indexes and indices are correct (see 
http://forum.wordreference.com/threads/index-plural-indexes-vs-indices.321709/).


> On Sept. 9, 2015, 3:46 p.m., Alexander Rukletsov wrote:
> > include/mesos/values.hpp, line 21
> > 
> >
> > ```
> > #include 
> > #include 
> > ```

Should be cpp, or?


> On Sept. 9, 2015, 3:46 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, line 198
> > 
> >
> > `nextRemove` cannot be greater than `nextElement`, because you do 
> > `deleteSet.insert(nextElement);` at the end. The only exception is the 
> > first iteration. How about initializing `nextElement = *deleteSet.begin() + 
> > 1` and remove this line?

We also have to erease the first element in deleteSet later, but ok.


> On Sept. 9, 2015, 3:46 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 250-271
> > 
> >
> > I think we can conflate this two cases, how about this:
> > ```
> > if (range->end() + 1 >= current.begin()) {
> >   range->set_end(std::max(range.end(), current.end()));
> >   deleteSet.insert(y);
> >   ++i;
> > } else {
> >   break;
> > }
> > ```

Yes we could do this. I personally find the 2 distinct cases (especially with 
the range comments) more readable.


- Joerg


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


On Sept. 8, 2015, 7:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 7:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Alexander Rukletsov]

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



include/mesos/values.hpp (line 21)


```
#include 
#include 
```



include/mesos/values.hpp (lines 58 - 59)


Empty line, please.



src/common/values.cpp (line 91)


s/Remove/Removes
s/indexes/indices

Mind refining a comment a bit? It looks like it evolved during time and 
contains artifacts from previous versions : ).



src/common/values.cpp (line 95)


I would pass set by pointer here to explicitly document that it may be 
changed in the function. In that case the caller will have to `&deleteSet` 
which hopefully make them look closer to the signature. Also, let's mention 
that in the comment.



src/common/values.cpp (line 105)


`nextRemove` cannot be greater than `nextElement`, because you do 
`deleteSet.insert(nextElement);` at the end. The only exception is the first 
iteration. How about initializing `nextElement = *deleteSet.begin() + 1` and 
remove this line?



src/common/values.cpp (line 136)


s/next/nested ?



src/common/values.cpp (lines 157 - 178)


I think we can conflate this two cases, how about this:
```
if (range->end() + 1 >= current.begin()) {
  range->set_end(std::max(range.end(), current.end()));
  deleteSet.insert(y);
  ++i;
} else {
  break;
}
```



src/common/values.cpp (lines 268 - 273)


Can we insert a new range into an appropriate position? This should be 
`O(n)` instead of `O(n log n)`


- Alexander Rukletsov


On Sept. 8, 2015, 7:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 7:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 7:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 7:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, line 231
> > 
> >
> > An idea to re-use code here. This function boils down to inserting an 
> > element into a sorted range and then performing one step of coalescing from 
> > this element till the end of the sequence. We can keep the insetion 
> > "virual" and move the elements once during the second coalescing pass (see 
> > my comment above).

New code does only insert a new range if not overlapping/neighboring with 
existing ranges, otherwise existing ranges are updated.


- Joerg


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


On Sept. 8, 2015, 7:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 7:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 8, 2015, 7:25 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Changes
---

Added missing dot at end of comment.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 8, 2015, 7:22 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Changes
---

Improved the deleting of ranges in coalesce(ranges).


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 2:33 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 2:33 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 8, 2015, 2:33 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 277-278
> > 
> >
> > Since you're editing here, mind explaining what an "urange" is?

As the comment above states it is an Un-coalesced range, will try to highlight 
that more.


> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, line 665
> > 
> >
> > Just to make sure, you have added just `2` lines to this function (not 
> > easy on RB):
> > ```
> >   sort(ranges);
> >   coalesce(ranges);
> > ```
> > Correct?

And removed one temporary Ranges object (see line 72 in previous version).


> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 442-444
> > 
> >
> > Let's let compiler do it's job: how about passing `left` by value and 
> > not creating a copy manually?

Wanted to keep the (internal) interface consistent with the other operators. Do 
you have a strong preference here?


> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 185-186
> > 
> >
> > s/is/are
> > 
> > I would suggest we write high level comments. How about this: "We 
> > assume ranges are sorted in ascending order of the lower endpoint."

ranges is the Ranges object i.e. is, and otherwise I personally find 
range.begin() a more precise description


- Joerg


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


On Sept. 7, 2015, 6:04 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 7, 2015, 6:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Alexander Rukletsov]

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


I would suggest to write a summarizing comment about how coalescing works, 
preferably with algorithmic complexity : ). This may help us to evaluate 
performance and maybe tune the algorithm later on when we have more usage 
experience and feedback from operators.


src/common/values.cpp (line 91)


Mention it's an in-place operation, please



src/common/values.cpp (lines 92 - 93)


s/is/are

I would suggest we write high level comments. How about this: "We assume 
ranges are sorted in ascending order of the lower endpoint."



src/common/values.cpp (line 94)


A high level comment.

`RepeatedPtrField` is a linear data structure, removing elements from it 
causes relocating all elements beyond the one being deleted.

I would suggest one of the alternatives:
* create a copy
* do two passes, instead of deleting mark the ranges and remove them at 
once during the second pass.

Anyway, a short comment summarizing the algorithm would be great for 
posterity!



src/common/values.cpp (line 109)


s/delete range/delete current ?

How about rewriting this in active voice for clarity? "If `range` subsumes 
`current`, delete `current`"



src/common/values.cpp (line 138)


An idea to re-use code here. This function boils down to inserting an 
element into a sorted range and then performing one step of coalescing from 
this element till the end of the sequence. We can keep the insetion "virual" 
and move the elements once during the second coalescing pass (see my comment 
above).



src/common/values.cpp (line 140)


Let's pull it up to the function comment.



src/common/values.cpp (line 165)


Want to transform this into a high level comment before the block?



src/common/values.cpp (lines 171 - 174)


Nice diagram! Helps a lot.



src/common/values.cpp (lines 184 - 185)


Since you're editing here, mind explaining what an "urange" is?



src/common/values.cpp (line 237)


I though you don't like post-increments : )



src/common/values.cpp (lines 344 - 346)


Let's let compiler do it's job: how about passing `left` by value and not 
creating a copy manually?



src/common/values.cpp (line 360)


Does it make sense to optimize out this call? IIUC, `Values::Ranges` should 
be always in sorted and coalesced state ("normalized") unless modified manually 
via protobuf methods. We can require normalization for `operator+=()` to 
succeed. Similar to how you require sorting for coalescing. I would suggest 
let's define invariants (or pre-conditions and post-conditions) for some 
crucial methods, like operators, `sort()`, `coalesce()` and document them.

If you prefer to leave it, then IIUC you should also `sort()` before 
`coalesce()`.



src/common/values.cpp (line 546)


Just to make sure, you have added just `2` lines to this function (not easy 
on RB):
```
  sort(ranges);
  coalesce(ranges);
```
Correct?



src/common/values.cpp (lines 630 - 637)


I've got confused by two syntacticly same by semantically different 
`begin()`s and `end()`s. Let's leave a note for posterity hinting the 
difference.

Also, `clang-format` suggests the following:
```
void sort(Value::Ranges* ranges)
{
  std::sort(ranges->mutable_range()->begin(),
ranges->mutable_range()->end(),
[](const Value::Range& a,
   const Value::Range& b) { return a.begin() < b.begin(); });
}
```



src/tests/values_tests.cpp (line 128)


Not sure I follow the comment. Did you mean you cannot use `parse()` 
because it will `coalesce()`, while you want to test `sort()` in isolation?



src/tests/values_tests.cpp (line 131)


Let's document what range is created here, like in the `RangesCoalesce` 
test.



src/tests/values_tests.cpp (line 158)


How about an ov

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 7, 2015, 6:04 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 7, 2015, 6:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 7, 2015, 6:04 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 7, 2015, 4:57 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Bugs: MESOS-3051
https://issues.apache.org/jira/browse/MESOS-3051


Repository: mesos


Description (updated)
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad