> On April 2, 2015, 7:45 p.m., Ben Mahler wrote: > > Looks good Cody, just curious about many of the includes in the .cpp flag > > files, had a hard time telling how these were relevant.
Mainly just things left over from copying from the old. Removed most. Precise comments below. > On April 2, 2015, 7:45 p.m., Ben Mahler wrote: > > src/master/flags.hpp, lines 405-406 > > <https://reviews.apache.org/r/32558/diff/4/?file=910387#file910387line405> > > > > Where is the mesos.hpp include for these? Looks like you removed it? It was included implicitly by <mesos/module/module.hpp>. Added it back. > On April 2, 2015, 7:45 p.m., Ben Mahler wrote: > > src/master/flags.cpp, lines 19-21 > > <https://reviews.apache.org/r/32558/diff/4/?file=910388#file910388line19> > > > > I can't tell how these are relevant to this file, is there something > > I'm missing? <stout/flags.hpp>: Implicitly included through 'logging/flags.hpp'. That include may go away in time. This is included explicitly since we use add() which does a lot of template intantiation which depends directly on <stout/flags.hpp> Dropped json, protobuf. What they were once used for is covered by "common/parse.hpp". > On April 2, 2015, 7:45 p.m., Ben Mahler wrote: > > src/master/flags.cpp, lines 23-24 > > <https://reviews.apache.org/r/32558/diff/4/?file=910388#file910388line23> > > > > How are these being used in this file? Removed. Included via header now. > On April 2, 2015, 7:45 p.m., Ben Mahler wrote: > > src/master/flags.cpp, line 30 > > <https://reviews.apache.org/r/32558/diff/4/?file=910388#file910388line30> > > > > Ditto here, where is this relevant? Dropped. Not needed in this file, had copied the old includes over and reworked them to follow proper include order. > On April 2, 2015, 7:45 p.m., Ben Mahler wrote: > > src/slave/flags.cpp, line 19 > > <https://reviews.apache.org/r/32558/diff/4/?file=910390#file910390line19> > > > > Why do you need this? Same as why it is in "master/flags.hpp". > On April 2, 2015, 7:45 p.m., Ben Mahler wrote: > > src/slave/flags.cpp, line 21 > > <https://reviews.apache.org/r/32558/diff/4/?file=910390#file910390line21> > > > > Why do you need this? Fixed. > On April 2, 2015, 7:45 p.m., Ben Mahler wrote: > > src/slave/flags.cpp, line 25 > > <https://reviews.apache.org/r/32558/diff/4/?file=910390#file910390line25> > > > > What's this one needed for? Fixed. - Cody ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/#review78703 ----------------------------------------------------------- On April 9, 2015, 6:27 p.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32558/ > ----------------------------------------------------------- > > (Updated April 9, 2015, 6:27 p.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 9c01f5d6c692f835100e7cade928748cc4763cc8 > 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 > >
