mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Cody Maloney" <c...@mesosphere.io>
Subject Re: Review Request 32558: Improve compile time of mesos by splitting flags
Date Tue, 31 Mar 2015 02:24:43 GMT


> 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() { ... }
>     ```

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


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


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