Re: Review Request 35694: Added helper constructors to hashmap.
On June 20, 2015, 7:35 p.m., Alexander Rukletsov wrote:
Why do we use different approaches (emplace() and insert()) in c-tors? Is
it possible to unify them for clarity? If not, mind leaving a comment
explaining the reasoning?
Great idea, I added a comment as to why we use 'insert' instead of 'emplace'
since we use 'emplace' everywhere else.
On June 20, 2015, 7:35 p.m., Alexander Rukletsov wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp, line 52
https://reviews.apache.org/r/35694/diff/3/?file=988938#file988938line52
Let's avoid re-creating iterator:
```
for (auto iterator = map.begin(), auto endIterator = map.end();
iterator != endIterator;
++iterator) {
```
Till Toenshoff wrote:
We never really do this kind of optimzation within mesos, or do we? I
briefly checked a couple of stout-files which dont try to avoid re-getting
the end of a list.
Given that it does not increase readability, I'ld suggest to first check
if this really was a serious gainer (some neat little profiling) and if so,
adapt our styleguide.
Alexander Rukletsov wrote:
It does not reduce readability either, why not gaining free performance
then?
And actually legal precedent is set: `slave.cpp:1262`, commit
ccf6c254a1620e512ec66f1e20644d47c12c6832
This isn't a precedent that we've set in the past. The example you showed Alex
looks like they were actually trying to account for the fact that the thing
we're iterating over is actually modified during the operation, which can cause
issues with determining what 'begin' and 'end' are. I'm going to keep this
simple for now.
- Benjamin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35694/#review88661
---
On June 24, 2015, 10 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35694/
---
(Updated June 24, 2015, 10 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
---
See summary.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp
4f90d3dcd880b95f22ea13c56a61c7f981eea57d
3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp
6a26d93a9a68ab18b7c9b25039a96b663a73a309
Diff: https://reviews.apache.org/r/35694/diff/
Testing
---
make check
Thanks,
Benjamin Hindman
Re: Review Request 35694: Added helper constructors to hashmap.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35694/ --- (Updated June 24, 2015, 10 p.m.) Review request for mesos and Till Toenshoff. Changes --- Updates from review comments. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 4f90d3dcd880b95f22ea13c56a61c7f981eea57d 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp 6a26d93a9a68ab18b7c9b25039a96b663a73a309 Diff: https://reviews.apache.org/r/35694/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35694: Added helper constructors to hashmap.
On June 20, 2015, 7:35 p.m., Alexander Rukletsov wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp, line 52
https://reviews.apache.org/r/35694/diff/3/?file=988938#file988938line52
Let's avoid re-creating iterator:
```
for (auto iterator = map.begin(), auto endIterator = map.end();
iterator != endIterator;
++iterator) {
```
Till Toenshoff wrote:
We never really do this kind of optimzation within mesos, or do we? I
briefly checked a couple of stout-files which dont try to avoid re-getting
the end of a list.
Given that it does not increase readability, I'ld suggest to first check
if this really was a serious gainer (some neat little profiling) and if so,
adapt our styleguide.
It does not reduce readability either, why not gaining free performance then?
And actually legal precedent is set: `slave.cpp:1262`, commit
ccf6c254a1620e512ec66f1e20644d47c12c6832
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35694/#review88661
---
On June 20, 2015, 7:18 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35694/
---
(Updated June 20, 2015, 7:18 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
---
See summary.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp
4f90d3dcd880b95f22ea13c56a61c7f981eea57d
3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp
6a26d93a9a68ab18b7c9b25039a96b663a73a309
Diff: https://reviews.apache.org/r/35694/diff/
Testing
---
make check
Thanks,
Benjamin Hindman
Re: Review Request 35694: Added helper constructors to hashmap.
On June 20, 2015, 7:35 p.m., Alexander Rukletsov wrote:
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp, line 52
https://reviews.apache.org/r/35694/diff/3/?file=988938#file988938line52
Let's avoid re-creating iterator:
```
for (auto iterator = map.begin(), auto endIterator = map.end();
iterator != endIterator;
++iterator) {
```
We never really do this kind of optimzation within mesos, or do we? I briefly
checked a couple of stout-files which dont try to avoid re-getting the end of a
list.
Given that it does not increase readability, I'ld suggest to first check if
this really was a serious gainer (some neat little profiling) and if so, adapt
our styleguide.
On June 20, 2015, 7:35 p.m., Alexander Rukletsov wrote:
3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp, line 32
https://reviews.apache.org/r/35694/diff/3/?file=988939#file988939line32
Let's expand the test for move c-tor as well.
+1
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35694/#review88661
---
On June 20, 2015, 7:18 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35694/
---
(Updated June 20, 2015, 7:18 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
---
See summary.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp
4f90d3dcd880b95f22ea13c56a61c7f981eea57d
3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp
6a26d93a9a68ab18b7c9b25039a96b663a73a309
Diff: https://reviews.apache.org/r/35694/diff/
Testing
---
make check
Thanks,
Benjamin Hindman
Re: Review Request 35694: Added helper constructors to hashmap.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35694/#review88774 --- Ship it! 3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp (line 59) https://reviews.apache.org/r/35694/#comment141358 s/from a/from an/ - Till Toenshoff On June 20, 2015, 7:18 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35694/ --- (Updated June 20, 2015, 7:18 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 4f90d3dcd880b95f22ea13c56a61c7f981eea57d 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp 6a26d93a9a68ab18b7c9b25039a96b663a73a309 Diff: https://reviews.apache.org/r/35694/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35694: Added helper constructors to hashmap.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35694/ --- (Updated June 20, 2015, 7:06 p.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-2832 https://issues.apache.org/jira/browse/MESOS-2832 Repository: mesos Description --- See summary. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 4f90d3dcd880b95f22ea13c56a61c7f981eea57d Diff: https://reviews.apache.org/r/35694/diff/ Testing --- make check Thanks, Benjamin Hindman
Re: Review Request 35694: Added helper constructors to hashmap.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35694/#review88661
---
Why do we use different approaches (emplace() and insert()) in c-tors? Is it
possible to unify them for clarity? If not, mind leaving a comment explaining
the reasoning?
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp (line 52)
https://reviews.apache.org/r/35694/#comment141248
Let's avoid re-creating iterator:
```
for (auto iterator = map.begin(), auto endIterator = map.end();
iterator != endIterator;
++iterator) {
```
3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp (line 32)
https://reviews.apache.org/r/35694/#comment141249
Let's expand the test for move c-tor as well.
- Alexander Rukletsov
On June 20, 2015, 7:18 p.m., Benjamin Hindman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35694/
---
(Updated June 20, 2015, 7:18 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
---
See summary.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp
4f90d3dcd880b95f22ea13c56a61c7f981eea57d
3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp
6a26d93a9a68ab18b7c9b25039a96b663a73a309
Diff: https://reviews.apache.org/r/35694/diff/
Testing
---
make check
Thanks,
Benjamin Hindman
