mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 35694: Added helper constructors to hashmap.
Date Wed, 24 Jun 2015 22:01:20 GMT


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


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