You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2019/05/01 01:19:02 UTC
Review Request 70576: Made `RandomSorter::Node::clientPath()` return
a const reference.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70576/
-----------------------------------------------------------
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
Re: Review Request 70576: Made `RandomSorter::Node::clientPath()`
return a const reference.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 70576: Made `RandomSorter::Node::clientPath()`
return a const reference.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70576/#review214972
-----------------------------------------------------------
FAIL: Failed to get dependent review IDs for the current patch.
Failed command: `python.exe D:\DCOS\mesos\mesos\support\get-review-ids.py -r 70576 -o C:\Users\jenkins\AppData\Local\Temp\mesos_dependent_review_ids`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3301/mesos-review-70576
Relevant logs:
- [get-review-ids.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3301/mesos-review-70576/logs/get-review-ids.log):
```
Dependent review: https://reviews.apache.org/api/review-requests/70419/
Dependent review: https://reviews.apache.org/api/review-requests/70418/
The review request 70418 is already submitted
Dependent review: https://reviews.apache.org/api/review-requests/70419/
Traceback (most recent call last):
File "D:\DCOS\mesos\mesos\support\get-review-ids.py", line 62, in <module>
main()
File "D:\DCOS\mesos\mesos\support\get-review-ids.py", line 51, in main
review_ids = handler.get_dependent_review_ids(review_request)
File "D:\DCOS\mesos\mesos\support\common.py", line 93, in get_dependent_review_ids
self._review_ids(review_request, review_ids)
File "D:\DCOS\mesos\mesos\support\common.py", line 62, in _review_ids
self._review_ids(dependent_review, review_ids)
File "D:\DCOS\mesos\mesos\support\common.py", line 62, in _review_ids
self._review_ids(dependent_review, review_ids)
File "D:\DCOS\mesos\mesos\support\common.py", line 61, in _review_ids
"field." % review_request["id"])
common.ReviewError: Circular dependency detected for review 70418. Please fix the 'depends_on' field.
```
- Mesos Reviewbot Windows
On April 30, 2019, 6:19 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70576/
> -----------------------------------------------------------
>
> (Updated April 30, 2019, 6:19 p.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
>
>
Re: Review Request 70576: Made `RandomSorter::Node::clientPath()`
return a const reference.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70576/#review214971
-----------------------------------------------------------
Bad review!
Reviews applied: [70576, 70419, 70418]
Error:
Circular dependency detected for review 70419.Please fix the 'depends_on' field.
- Mesos Reviewbot
On April 30, 2019, 6:19 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70576/
> -----------------------------------------------------------
>
> (Updated April 30, 2019, 6:19 p.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
>
>