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 52695: Harden libprocess
Date Wed, 02 Nov 2016 09:32:54 GMT

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



I would really like to see actual timings of e.g., an optimized build before and after introducing
these new flags, e.g., the runtime of `libprocess-tests` and `benchmarks`.


3rdparty/libprocess/Makefile.am (line 16)
<https://reviews.apache.org/r/52695/#comment224111>

    Remove note about `-Werror` which is not used.



3rdparty/libprocess/Makefile.am (lines 16 - 24)
<https://reviews.apache.org/r/52695/#comment224124>

    I think this would be easier to follow if you'd incrementially build up `AM_CXXFLAGS`
while explaining their effect, e.g.,
    
        # Enable common (and some language specific) warnings.
        AM_CXXFLAGS += -Wall
        # Warn when a comparison is made between signed and unsigned values.
        AM_CXXFLAGS += -Wsign-compare
        ...



3rdparty/libprocess/Makefile.am (line 29)
<https://reviews.apache.org/r/52695/#comment224110>

    Let's not suppress this valid and potentially useful diagnostic for the whole codebase.
It does not trigger a hard failure anyway.



3rdparty/libprocess/Makefile.am (line 30)
<https://reviews.apache.org/r/52695/#comment224112>

    I am not a big fan of unconditionally omitting frame pointers as this gives the optimizer
one less register to work with. Unfortunately one cannot easily tell the actual impact of
this from the info here. Is this strictly needed here or just nice to have?



3rdparty/libprocess/configure.ac (line 272)
<https://reviews.apache.org/r/52695/#comment224114>

    `s/!$/./`



3rdparty/libprocess/m4/ax_check_compile_flag.m4 (line 1)
<https://reviews.apache.org/r/52695/#comment224106>

    For future updates it would be great if we'd write down the autoconf-archive release this
file came from (it looks like the latest release containing it is `v2016.09.16`).


- Benjamin Bannier


On Nov. 1, 2016, 11:22 p.m., Aaron Wood wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52695/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2016, 11:22 p.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
>     https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Use a default set of flags to provide additional security and hardening to libprocess.
Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 7131989 
>   3rdparty/libprocess/configure.ac 1644035 
>   3rdparty/libprocess/m4/ax_check_compile_flag.m4 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52695/diff/
> 
> 
> Testing
> -------
> 
> Compared the benchmarks with and without the flags being used. Also did a comparsion
with the flags being used with and without optimizations and without the flags being used
with and without optimizations. Overall the performance hit was very small with a 3-8% overhead
(optimizations brings this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>


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