> On March 27, 2015, 4:41 p.m., Timothy Chen wrote: > > src/slave/flags.cpp, line 29 > > <https://reviews.apache.org/r/32558/diff/3/?file=907340#file907340line29> > > > > How about keeping the same style with namespaces? Just to be consistent > > with everywhere else in the code base. > > Cody Maloney wrote: > What do you mean by this? Do a bunch of "using namespace declarations"? > That will make the code longer, and the translation less direct than it > currently is. Also if it is 'using namespace' then a bunch of the explicit > namespace prefixes previously in the code should be removed, which makes this > a less direct translation. Currently it is just copy/base from header to > source file the body of the function, then rework the includes to not include > duplicates. Anything more in this move and I think the odds of introducing a > bug in what should be mechanical code motion increases significantly. > > Michael Park wrote: > I think he means: > > ```cpp > namespace mesos { > namespace internal { > namespace slave { > > Flags::Flags() { ... } > > } // namespace slave { > } // namespace internal { > } // namespace mesos { > ``` > > As opposed to: > > ```cpp > mesos::internal::slave::Flags::Flags() { ... } > ``` > > Cody Maloney wrote: > I can restructure it like that, I think it obscures what is happening and > what could possibly be happening if you aren't seeing this diff (There is one > very specific function being defined, it is the only function). > > Reading through the style guide (Mesos, Google) I don't see any rules > around this. > > Cody Maloney wrote: > One more thing to think abou: if I switch to stepping into the namespaces > like that, all the code I'm copy/pasting which references stuff in the > specific namespaces I'm now inside should have those namespaces removed, > wihch considerably complicates the code change / move removes the mechanical > aspect of it. If I don't change the namespaces with that I've introduced > another inconsistency.
I think it's fine to leave as is, as I realize we don't usually do this in cpp files anyways. I'll drop this. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/#review78071 ----------------------------------------------------------- On March 31, 2015, 2:16 a.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32558/ > ----------------------------------------------------------- > > (Updated March 31, 2015, 2:16 a.m.) > > > Review request for mesos, Joris Van Remoortere and Michael Park. > > > Bugs: MESOS-292 > https://issues.apache.org/jira/browse/MESOS-292 > > > Repository: mesos > > > Description > ------- > > Split the mesos master, slave flags into header + source file to > improve compile time significantly. Should be no functional changes. > > Largely copy-paste, with a little reworking to remove unnecessary > headers from the .hpp, and order headers a little more reliably > (as well as remove duplicate includes) in the .cpp. > > # Impact of these changes > `time make check -j8` > Before: > make check 2732.93s user 103.89s system 514% cpu 9:11.63 total > > After: > make check 2421.18s user 96.60s system 506% cpu 8:16.67 total > > 4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, > Intel i7-4790K. > > The numbers aren't incredibly stable (Other software running, > overclocking enabled, etc). They are a good general measure though of > speedup. > > I do a `make check` rather than just `make` because that is what devs > do in a day-to-day flow, and if runtime is significantly impacted, it > will show up. > > Test steps: > ``` > # Warm cache > ../configure --disable-python --disable-java > make check > # Timed build > rm -rf * > ../configure --disable-python --disable-java > time make check > ``` > > Note: Similar, likely greater improvements in compile time would happen > if stout were made to not be header only. Specifically <stout/os.hpp>. > > > Diffs > ----- > > src/Makefile.am 56ed9d9f047ebc3507c8ba235bba4e1ffcf76cd3 > src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 > src/logging/flags.cpp PRE-CREATION > src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b > src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd > src/master/flags.cpp PRE-CREATION > src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef > src/slave/flags.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/32558/diff/ > > > Testing > ------- > > make check on ArchLinux with GCC 4.9.2 > make distcheck CentOS 7 > > > Thanks, > > Cody Maloney > >