Bug 160544 – [GTK] Build the jhbuild with -O2 optimization level by default

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

It seems using the internal JHBuild is causing a big performance impact because we are building without compiler optimizations.

Some numbers:

Obtained n release build of WebKitGTK+/r204105 by running the performance test:

  $ Tools/Scripts/run-benchmark --platform gtk --browser minibrowser --plan animometer



1) With System libs (Debian 8 jessie):

Animometer:Score:Geometric: 252pt stdev=0.0%
          Animometer:Score:Geometric: 252pt stdev=0.0%
                    Canvas Arcs:Score: 256pt stdev=0.0%
                    Canvas Lines:Score: 846pt stdev=0.0%
                    Design:Score: 48.0pt stdev=0.0%
                    Focus:Score: 318pt stdev=0.0%
                    Images:Score: 219pt stdev=0.0%
                    Leaves:Score: 99.9pt stdev=0.0%
                    Multiply:Score: 358pt stdev=0.0%
                    Paths:Score: 900pt stdev=0.0%
                    Suits:Score: 174pt stdev=0.0%



2) With default (current) jhbuild

Animometer:Score:Geometric: 53.5pt stdev=0.0%
          Animometer:Score:Geometric: 53.5pt stdev=0.0%
                    Canvas Arcs:Score: 67.1pt stdev=0.0%
                    Canvas Lines:Score: 1.00pt stdev=0.0%
                    Design:Score: 14.3pt stdev=0.0%
                    Focus:Score: 301pt stdev=0.0%
                    Images:Score: 183pt stdev=0.0%
                    Leaves:Score: 32.2pt stdev=0.0%
                    Multiply:Score: 175pt stdev=0.0%
                    Paths:Score: 205pt stdev=0.0%
                    Suits:Score: 59.0pt stdev=0.0%



3) With default jhbuild and cairo with -O2:

          Animometer:Score:Geometric: 74.2pt stdev=0.0%
                    Canvas Arcs:Score: 171pt stdev=0.0%
                    Canvas Lines:Score: 1.00pt stdev=0.0%
                    Design:Score: 14.7pt stdev=0.0%
                    Focus:Score: 321pt stdev=0.0%
                    Images:Score: 198pt stdev=0.0%
                    Leaves:Score: 34.1pt stdev=0.0%
                    Multiply:Score: 259pt stdev=0.0%
                    Paths:Score: 558pt stdev=0.0%
                    Suits:Score: 86.6pt stdev=0.0%



4) With all jhbuild with -O2:

Animometer:Score:Geometric: 259pt stdev=0.0%
          Animometer:Score:Geometric: 259pt stdev=0.0%
                    Canvas Arcs:Score: 225pt stdev=0.0%
                    Canvas Lines:Score: 968pt stdev=0.0%
                    Design:Score: 53.6pt stdev=0.0%
                    Focus:Score: 332pt stdev=0.0%
                    Images:Score: 229pt stdev=0.0%
                    Leaves:Score: 102pt stdev=0.0%
                    Multiply:Score: 349pt stdev=0.0%
                    Paths:Score: 904pt stdev=0.0%
                    Suits:Score: 187pt stdev=0.0%


5) With all jhbuild with -O3:

Animometer:Score:Geometric: 267pt stdev=0.0%
          Animometer:Score:Geometric: 267pt stdev=0.0%
                    Canvas Arcs:Score: 259pt stdev=0.0%
                    Canvas Lines:Score: 1000pt stdev=0.0%
                    Design:Score: 53.1pt stdev=0.0%
                    Focus:Score: 311pt stdev=0.0%
                    Images:Score: 219pt stdev=0.0%
                    Leaves:Score: 109pt stdev=0.0%
                    Multiply:Score: 371pt stdev=0.0%
                    Paths:Score: 934pt stdev=0.0%
                    Suits:Score: 194pt stdev=0.0%

[Carlos Alberto Lopez Perez]

Created attachment 285311 [details]
Patch

[Michael Catanzaro]

Comment on attachment 285311 [details]
Patch

This is going to reduce backtrace quality on the debug bot. It also isn't the best solution: CMake should already be handling this for us. -DCMAKE_BUILD_TYPE=Release means to use -O3. -DCMAKE_BUILD_TYPE=Debug means to use -O0 -g. I think we need further investigation to see why this isn't working. If build-webkit isn't setting CMAKE_BUILD_TYPE, it should do so. If it is setting the build type, then we might be stripping the optimization flags away in a CMake file somewhere, which would be bad.

[Carlos Garcia Campos]

This is not about the flags used when building WebKit itself but its dependencies. I agree we should update the jhbuildrc so that it only affects the deps and not the WebKit build.

[Michael Catanzaro]

(In reply to comment #3)
> This is not about the flags used when building WebKit itself but its
> dependencies. 

Good point, my review is wrong.

> I agree we should update the jhbuildrc so that it only affects
> the deps and not the WebKit build.

I'm not sure how to do this, though; we have module_extra_env, but surely don't want to open-code every module name except WebKit there, and I see no other way to exclude WebKit from receiving the flags. I guess we could use -O2 and just live with reduced backtrace quality, it's not a big deal.

[Michael Catanzaro]

So I don't think it's possible to fix in the jhbuildrc, but since we don't actually use 'jhbuild build' to build WebKit, probably all we need to do is append other optimization flags to CFLAGS from somewhere in build-webkit after entering the jhbuild environment. Maybe we already do this.

[Carlos Alberto Lopez Perez]

(In reply to comment #4)
> I'm not sure how to do this, though; we have module_extra_env, but surely
> don't want to open-code every module name except WebKit there, and I see no
> other way to exclude WebKit from receiving the flags. 

This is news to me...

Are the environment variables from Tools/gtk/jhbuildrc leaking into the WebKit build process? Then this patch is wrong

The idea is to pass -O2 only when building the jhbuild.

[Michael Catanzaro]

(In reply to comment #6)
> This is news to me...
> 
> Are the environment variables from Tools/gtk/jhbuildrc leaking into the
> WebKit build process? Then this patch is wrong

Yes, the environment you set in jhbuildrc will be set inside the jhbuild environment that we build in (as required to use the dependencies we've built).

[Carlos Alberto Lopez Perez]

(In reply to comment #7)
> (In reply to comment #6)
> > This is news to me...
> > 
> > Are the environment variables from Tools/gtk/jhbuildrc leaking into the
> > WebKit build process? Then this patch is wrong
> 
> Yes, the environment you set in jhbuildrc will be set inside the jhbuild
> environment that we build in (as required to use the dependencies we've
> built).

And it seems that setting those variables there is what caused this problem.

I have tested this patch:

diff --git a/Tools/gtk/jhbuildrc b/Tools/gtk/jhbuildrc
index 29323b3..9b210a7 100644
--- a/Tools/gtk/jhbuildrc
+++ b/Tools/gtk/jhbuildrc
@@ -37,8 +37,5 @@ buildroot = os.path.join(os.path.dirname(checkoutroot), "Build")
 # For the layout tests: path where llvmpipe/software-only mesa libraries are installed.
 os.environ['LLVMPIPE_LIBGL_PATH'] = os.path.abspath(os.path.join(prefix, 'softGL', 'lib'))
 
-os.environ['CFLAGS'] = '-Wno-error'
-os.environ['CXXFLAGS'] = '-Wno-error'
-
 # For building gstreamer plugins on the Mac.
 os.environ['OBJCFLAGS'] = '-Wno-error'


And rebuild the JHBuild, without defining any CFLAGS/CXXFLAGS environment variable, and now I get this performance numbers:

Animometer:Score:Geometric: 261pt stdev=0.0%
          Animometer:Score:Geometric: 261pt stdev=0.0%
                    Canvas Arcs:Score: 256pt stdev=0.0%
                    Canvas Lines:Score: 1.02Kpt stdev=0.0%
                    Design:Score: 50.7pt stdev=0.0%
                    Focus:Score: 318pt stdev=0.0%
                    Images:Score: 219pt stdev=0.0%
                    Leaves:Score: 103pt stdev=0.0%
                    Multiply:Score: 368pt stdev=0.0%
                    Paths:Score: 906pt stdev=0.0%
                    Suits:Score: 181pt stdev=0.0%



Which is on par with forcing -O2.


And it seems that autotools based build system set by default -O2  *unless* there is an environment variable CCFLAGS/CXXFLAGS defined. In which case they pick that variable, and we are just passing "-Wno-error" since r197713 <http://trac.webkit.org/r197713>.


Check this quick test with gtk+:

gtk+ $ ./configure
gtk+ $ grep -P '^(CFLAGS|CXXFLAGS)' Makefile
CFLAGS = -g -O2 -Wall
CFLAGS_FOR_BUILD = -g -O2
CXXFLAGS = -g -O2

        ^^ If my environment is clean configure by default it sets -O2

gtk+ $ CFLAGS="-Wno-error" CXXFLAGS="-Wno-error" ./configure
gtk+ $ grep -P '^(CFLAGS|CXXFLAGS)' Makefile
CFLAGS = -Wno-error -Wall
CFLAGS_FOR_BUILD = -Wno-error
CXXFLAGS = -Wno-error

        ^^ If my environment contains CFLAGs it sets mine and not -O2.


I think we should avoid defining any CFLAG/CXXFLAG variable in Tools/gtk/jhbuildrc, because this variables are also leaking into the build process of WebKit, which don't seems like a good idea to me.

but I'm open to hear any other suggestion. Any idea/opinion?

[Carlos Alberto Lopez Perez]

... and I have just checked that we are also setting CXXFLAGS=-pipe CFLAGS=-pipe on the GTK+ bots locally ...

... which explains why there isn't any performance drop on http://perf.webkit.org after r197713, because the bots have been running with locally set cflags for years ...

... i'm going to remove any cflags variable from the local environment of the bots

[Michael Catanzaro]

(In reply to comment #8)
> I think we should avoid defining any CFLAG/CXXFLAG variable in
> Tools/gtk/jhbuildrc, because this variables are also leaking into the build
> process of WebKit, which don't seems like a good idea to me.

Well we really don't want to get rid of -Wno-error, but you could try setting it using autogenargs rather than the environment, that way they should get used only for the dependencies:

autogenargs = 'CFLAGS=-Wno-error CXXFLAGS=-Wno-error'

I think that would probably work.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #8)
> > I think we should avoid defining any CFLAG/CXXFLAG variable in
> > Tools/gtk/jhbuildrc, because this variables are also leaking into the build
> > process of WebKit, which don't seems like a good idea to me.
> 
> Well we really don't want to get rid of -Wno-error, but you could try
> setting it using autogenargs rather than the environment, that way they
> should get used only for the dependencies:
> 
> autogenargs = 'CFLAGS=-Wno-error CXXFLAGS=-Wno-error'
> 
> I think that would probably work.

autogenargs = '--disable-Werror'

There's an option for that in jhbuild now:

disable_Werror = True

but it's true by default, let's update the jhbuild rev if needed.

[Michael Catanzaro]

(In reply to comment #11)
> autogenargs = '--disable-Werror'

Won't work, that just passes --disable-Werror on to configure scripts, which ignore it unless they're using a recent version of AX_COMPILER_FLAGS.

[Carlos Alberto Lopez Perez]

(In reply to comment #10)
> (In reply to comment #8)
> > I think we should avoid defining any CFLAG/CXXFLAG variable in
> > Tools/gtk/jhbuildrc, because this variables are also leaking into the build
> > process of WebKit, which don't seems like a good idea to me.
> 
> Well we really don't want to get rid of -Wno-error, but you could try
> setting it using autogenargs rather than the environment, that way they
> should get used only for the dependencies:
> 
> autogenargs = 'CFLAGS=-Wno-error CXXFLAGS=-Wno-error'
> 
> I think that would probably work.

I think it won't work, because when exporting an environment variable you have to put it at the start of the command line. And JHBuild passes autogenargs after 
%(srcdir)s/%(autogen-sh)s

You could maybe play with the autogen-template but it feels too much hacky for me and prone to break some module on the moduleset already defining its own autogen-template.

I have two ideas going forward:

#1. Export some magic environment variable on the script build-webkit and then on jhbuildrc check for that variable. So we only export this CFLAGS when that variable is not set (will indicate the user is doing any other thing than building webkit).

#2. Check on the file jhbuildrc if jhbuild has been called to run a command or not, because build-webkit ends calling "jhbuild [..] run cmake [..]" to build webkit.

[Michael Catanzaro]

(In reply to comment #13) 
> I think it won't work, because when exporting an environment variable you
> have to put it at the start of the command line. And JHBuild passes
> autogenargs after 
> %(srcdir)s/%(autogen-sh)s

They don't need to be in the environment of configure; they'll get passed as arguments. If you run ./configure --help one some configure script you'll see:

"""
Usage: ./configure [OPTION]... [VAR=VALUE]...

To assign environment variables (e.g., CC, CFLAGS...), specify them as
VAR=VALUE.  See below for descriptions of some of the useful variables.
"""

> #1. Export some magic environment variable on the script build-webkit and
> then on jhbuildrc check for that variable. So we only export this CFLAGS
> when that variable is not set (will indicate the user is doing any other
> thing than building webkit).

That would work (but try my suggestion above first).

> #2. Check on the file jhbuildrc if jhbuild has been called to run a command
> or not, because build-webkit ends calling "jhbuild [..] run cmake [..]" to
> build webkit.

I don't understand this suggestion.

[Carlos Alberto Lopez Perez]

(In reply to comment #14)
> (In reply to comment #13) 
> > I think it won't work, because when exporting an environment variable you
> > have to put it at the start of the command line. And JHBuild passes
> > autogenargs after 
> > %(srcdir)s/%(autogen-sh)s
> 
> They don't need to be in the environment of configure; they'll get passed as
> arguments. If you run ./configure --help one some configure script you'll
> see:
> 
> """
> Usage: ./configure [OPTION]... [VAR=VALUE]...
> 
> To assign environment variables (e.g., CC, CFLAGS...), specify them as
> VAR=VALUE.  See below for descriptions of some of the useful variables.
> """
> 

I tried this, and it don't works with some packages that run ./autogen.sh as configure script, and their autogen.sh script doesn't handle properly arguments with spaces even when this arguments were passed quoted.

This is true at least for gstreamer, gst-plugins-* and openwebrtc.

Check this example:

gstreamer-1.8.0 $ ./autogen.sh --enable-introspection CFLAGS="-Wno-error -O2" CXFLAGS="-Wno-error -O2"

+ passing argument --enable-introspection to configure
+ passing argument CFLAGS=-Wno-error to configure
+ passing argument -O2 to configure
+ passing argument CXFLAGS=-Wno-error to configure
+ passing argument -O2 to configure
+ options passed to configure:  --enable-introspection CFLAGS=-Wno-error -O2 CXFLAGS=-Wno-error -O2
[....]
./configure --enable-maintainer-mode --enable-gtk-doc --enable-docbook --enable-failing-tests --enable-poisoning --enable-introspection CFLAGS=-Wno-error -O2 CXFLAGS=-Wno-error -O2
configure: error: unrecognized option: `-O2'
Try `./configure --help' for more information
  configure failed


For the gstreamer case the work-around will be to change autogen-sh to ./configure in the jhbuild template, but for the openwebrtc this won't work as they checkout from git and a pre-generated configure script is not shipped.

I could try to go the hard path of trying to fix each one of the autogen.sh scripts.... but nothing grants that the next module someone try to adds also has this problem.

> > #1. Export some magic environment variable on the script build-webkit and
> > then on jhbuildrc check for that variable. So we only export this CFLAGS
> > when that variable is not set (will indicate the user is doing any other
> > thing than building webkit).
> 
> That would work (but try my suggestion above first).
> 
> > #2. Check on the file jhbuildrc if jhbuild has been called to run a command
> > or not, because build-webkit ends calling "jhbuild [..] run cmake [..]" to
> > build webkit.
> 
> I don't understand this suggestion.

In Tools/gtk/jhbuildrc you can check how the program was invoked by inspecting sys.argv. 

When Tools/Scripts/update-webkitgtk-libs is invoked, what ends being executed is:

sys.argv = [/${WEBKITPATH}/WebKitBuild/DependenciesGTK/Root/bin/jhbuild', '--exit-on-error', '--no-interact', '-f', '/${WEBKITPATH}/Tools/jhbuild/../../Tools/gtk/jhbuildrc', 'build']

When Tools/Scripts/build-webkit --gtk --release is invoked, what ends being executed is:

sys.argv = ['/${WEBKITPATH}/WebKitBuild/DependenciesGTK/Root/bin/jhbuild', '--exit-on-error', '--no-interact', '-f', '/${WEBKITPATH}/Tools/jhbuild/../../Tools/gtk/jhbuildrc', 'run', 'cmake', '--build', '$/{WEBKITPATH}/WebKitBuild/Release', '--config', 'Release', '--', '-j8']


So, if 'run' is one of the options passed in sys.argv you know you are not building the jhbuild.

[Michael Catanzaro]

(In reply to comment #15) 
> In Tools/gtk/jhbuildrc you can check how the program was invoked by
> inspecting sys.argv. 

I'm fine with any of the proposed solutions that keep -Wno-error working. This one seems cleanest.

[Carlos Alberto Lopez Perez]

I have tested also the differences between passing -g1, passing -g or not passing any -g flag.

with *FLAGS = '-Wno-error -O2' $ du -hs WebKitBuild/DependenciesGTK/
3.6G	WebKitBuild/DependenciesGTK/

with *FLAGS = '-Wno-error -O2 -g1' $ du -hs WebKitBuild/DependenciesGTK/
3.9G	WebKitBuild/DependenciesGTK/

with *FLAGS = '-Wno-error -O2 -g' $ du -hs WebKitBuild/DependenciesGTK/
8.3G	WebKitBuild/DependenciesGTK/


-g1 provides basic debug information, which is handy when debugging some traceback that ends in the JHBuild. 


-g would provide better debug information, but it doubles the resources in disk space needed. So I guess that given that the intent is Debug webkit itself and not the libraries, -g1 seems the best tradeoff.

[Carlos Alberto Lopez Perez]

Created attachment 285581 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 285582 [details]
Patch

trivial: fix quote usage in comment

[Carlos Alberto Lopez Perez]

Comment on attachment 285582 [details]
Patch

Clearing flags on attachment: 285582

Committed r204279: <http://trac.webkit.org/changeset/204279>

[Carlos Alberto Lopez Perez]

All reviewed patches have been landed.  Closing bug.