mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 38158: Refactored Value::Ranges coalesce().
Date Mon, 07 Sep 2015 21:24:05 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/38158/#comment154064>

    Mention it's an in-place operation, please



src/common/values.cpp (lines 92 - 93)
<https://reviews.apache.org/r/38158/#comment154066>

    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)
<https://reviews.apache.org/r/38158/#comment154069>

    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)
<https://reviews.apache.org/r/38158/#comment154068>

    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)
<https://reviews.apache.org/r/38158/#comment154070>

    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)
<https://reviews.apache.org/r/38158/#comment154067>

    Let's pull it up to the function comment.



src/common/values.cpp (line 165)
<https://reviews.apache.org/r/38158/#comment154071>

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



src/common/values.cpp (lines 171 - 174)
<https://reviews.apache.org/r/38158/#comment154072>

    Nice diagram! Helps a lot.



src/common/values.cpp (lines 184 - 185)
<https://reviews.apache.org/r/38158/#comment154073>

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



src/common/values.cpp (line 237)
<https://reviews.apache.org/r/38158/#comment154074>

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



src/common/values.cpp (lines 344 - 346)
<https://reviews.apache.org/r/38158/#comment154075>

    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)
<https://reviews.apache.org/r/38158/#comment154063>

    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)
<https://reviews.apache.org/r/38158/#comment154062>

    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)
<https://reviews.apache.org/r/38158/#comment154058>

    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)
<https://reviews.apache.org/r/38158/#comment154044>

    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)
<https://reviews.apache.org/r/38158/#comment154045>

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



src/tests/values_tests.cpp (line 158)
<https://reviews.apache.org/r/38158/#comment154047>

    How about an overlapping range, like `[1-4, 3-6]`? I see you check this in the next one,
but since you have an isolated test here, why not slightly modifying the range to test one
more condition?



src/tests/values_tests.cpp (lines 176 - 177)
<https://reviews.apache.org/r/38158/#comment154046>

    Don't you want to do a full check here?



src/tests/values_tests.cpp (lines 244 - 245)
<https://reviews.apache.org/r/38158/#comment154048>

    It looks like you never used `parsed2`, nor compare you `parsed` and `parsed1`. Do you
want to use just `parsed` and `expected`?



src/tests/values_tests.cpp (lines 251 - 252)
<https://reviews.apache.org/r/38158/#comment154049>

    A typo?



src/tests/values_tests.cpp (line 255)
<https://reviews.apache.org/r/38158/#comment154050>

    Kill the newline, please.



src/tests/values_tests.cpp (line 271)
<https://reviews.apache.org/r/38158/#comment154051>

    one space between the arguments, please!



src/tests/values_tests.cpp (lines 273 - 274)
<https://reviews.apache.org/r/38158/#comment154052>

    Is it enough to check the size?



src/tests/values_tests.cpp (line 280)
<https://reviews.apache.org/r/38158/#comment154053>

    I'm not sure you should double 'p' in this case. Not a native speaker though.


- Alexander Rukletsov


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message