Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Joris Van Remoortere
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42762/ --- Review request for mesos and Michael Park. Repository: mesos Description

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Benjamin Bannier
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42762/#review116231 --- Was any of this confirm by profiling? In standard C++ there is an

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Neil Conway
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42762/#review116232 --- src/common/roles.cpp (line 59)

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Benjamin Bannier
> On Jan. 26, 2016, 1:29 a.m., Neil Conway wrote: > > src/common/roles.cpp, line 59 > > > > > > Would `static constexpr` be better? A `string` cannot be `constexpr` (could perform dynamic allocations). - Benjamin

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Neil Conway
> On Jan. 26, 2016, 12:29 a.m., Neil Conway wrote: > > src/common/roles.cpp, line 59 > > > > > > Would `static constexpr` be better? > > Benjamin Bannier wrote: > A `string` cannot be `constexpr` (could perform d

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Joris Van Remoortere
> On Jan. 26, 2016, 12:28 a.m., Benjamin Bannier wrote: > > Was any of this confirm by profiling? In standard C++ there is an > > overloaded `operator==` for `std::string` which can compare with a `const > > char*`. Yes confirmed by profiler. It it constructing the strings per iteration. - J

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Anand Mazumdar
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42762/#review116256 --- Fix it, then Ship it! LGTM. Puzzled me for a bit at why the tem

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Mesos ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42762/#review116269 --- Patch looks great! Reviews applied: [42761, 42762] Passed comman

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-25 Thread Joris Van Remoortere
> On Jan. 26, 2016, 2:14 a.m., Anand Mazumdar wrote: > > src/common/roles.cpp, line 62 > > > > > > Not yours: Can we remove the period at the end of each of the error > > messages? > > > > We dump them with

Re: Review Request 42762: Improved performance of 'roles::validate'.

2016-01-26 Thread Benjamin Bannier
> On Jan. 26, 2016, 3:14 a.m., Anand Mazumdar wrote: > > LGTM. Puzzled me for a bit at why the temporary is being constructed > > (assuming the correct `operator==` for `const char*` was being invoked) > > till I looked up here: > > > > https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%