mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70576: Made `RandomSorter::Node::clientPath()` return a const reference.
Date Wed, 01 May 2019 17:15:50 GMT

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



I think this is the right interface since no copying is needed to produce the path, but it
would be good to expand on where the copies are being eliminated, I didn't see any valid cases
just skimming through the code.

The only reason I held off on a ship it is that it seems like it's the right interface change
for the DRF sorter too, and I'm wondering why we don't want to keep the code consistent there.


src/master/allocator/sorter/drf/sorter.hpp
Lines 272 (patched)
<https://reviews.apache.org/r/70576/#comment301249>

    I'm curious why is this left as a TODO? Doesn't seem nice to have the sorter implementations
diverge if we can keep them in sync pretty easily.



src/master/allocator/sorter/random/sorter.cpp
Line 392 (original), 392 (patched)
<https://reviews.apache.org/r/70576/#comment301250>

    Where are we avoiding the string copy?
    
    If the original code was written "properly":
    
    ```
          string path = client->clientPath();
          CHECK(!result.contains(path));
          result.emplace(std::move(path), client->allocation.resources.at(slaveId));
    ```
    
    Then this change wouldn't save a copy


- Benjamin Mahler


On May 1, 2019, 1:19 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70576/
> -----------------------------------------------------------
> 
> (Updated May 1, 2019, 1:19 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This avoids a string copy.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/sorter/drf/sorter.hpp 91a9d668b87079158f7072780dc86bb08865166e

>   src/master/allocator/sorter/random/sorter.hpp 55e22d7705f163fe47d5aa47416ee0714c5a87c0

>   src/master/allocator/sorter/random/sorter.cpp 813f5b55d38dd9fa822de53ee944c3f72251a69d

> 
> 
> Diff: https://reviews.apache.org/r/70576/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> See benchmark result in https://reviews.apache.org/r/70497/
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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