mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 64743: Enabled function sections.
Date Tue, 09 Jan 2018 16:10:35 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64743/#review195039
-----------------------------------------------------------


Fix it, then Ship it!




Thanks for adjusting this.

It would be great if you could update the ticket with e.g., benchmark results so people get
a first feel for whether it makes sense for them to enable this feature in production builds.


cmake/CompilationConfigure.cmake
Lines 260 (patched)
<https://reviews.apache.org/r/64743/#comment274163>

    Also wondering what more high-levely name we could pick here instead.



cmake/CompilationConfigure.cmake
Lines 264 (patched)
<https://reviews.apache.org/r/64743/#comment274160>

    Could you document this flag in `docs/configuration/cmake.md`?



cmake/CompilationConfigure.cmake
Lines 269 (patched)
<https://reviews.apache.org/r/64743/#comment274159>

    Similar to the autotools setup, we should emit a hard error on an additional `else` branch
here.



configure.ac
Lines 175 (patched)
<https://reviews.apache.org/r/64743/#comment274162>

    I wonder whether it would make sense to use a more high-level name for this (in the end
this is about using sections which can be gc'ed, not just function sections).



configure.ac
Lines 505 (patched)
<https://reviews.apache.org/r/64743/#comment274161>

    Could you document this flag in `docs/configuration/autotools.md`?



configure.ac
Lines 527 (patched)
<https://reviews.apache.org/r/64743/#comment274158>

    This is a little weird.
    
    As a user I wouldn't expect my configuration to be silently dropped if it is not supported,
but preferably a hard error until configuration and platform are compatible.
    
    Could you insert a check for compatibility of `enable_function_sections` and `function_sections`
here and error out if they are not incompatible?


- Benjamin Bannier


On Jan. 4, 2018, 10:48 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64743/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2018, 10:48 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8348
>     https://issues.apache.org/jira/browse/MESOS-8348
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> If we tell the compiler to place each function in a separate
> section, this allows the linker to garbage collect unused
> sections. This significantly decreases the size of the final
> build artifacts and provides some modest improvements in build
> times.
> 
> 
> Diffs
> -----
> 
>   cmake/CompilationConfigure.cmake 0ae05047265e33aefab6eef67a4243b6b00f0318 
>   configure.ac b6eb98bfe5539c68a416831c36c182ee907d9421 
> 
> 
> Diff: https://reviews.apache.org/r/64743/diff/5/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>


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