Bug 102827 – [EFL] Optimize binary size by removing dead sections on unix/gcc

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Laszlo Gombos]

Turn on -ffunction-sections and -fdata-sections gcc flags and --gc-sections linker flag.

[Laszlo Gombos]

Created attachment 175257 [details]
turn on -ffunction-sections -fdata-sections --gc-section

In my environment (EFL port, WebKit2, 64 bit Linux, gold linker) the saving on the stripped binary is about 1.88Mb (5.64% of the original binary size)

[Patrick R. Gansterer]

Comment on attachment 175257 [details]
turn on -ffunction-sections -fdata-sections --gc-section

View in context: https://bugs.webkit.org/attachment.cgi?id=175257&action=review

Only a style comment, since I don't use it on Linux.

> Source/cmake/OptionsCommon.cmake:39
> +    SET(CMAKE_C_FLAGS "-ffunction-sections -fdata-sections ${CMAKE_C_FLAGS}")
> +    SET(CMAKE_CXX_FLAGS "-ffunction-sections -fdata-sections ${CMAKE_CXX_FLAGS}")

Usually this is written as SET(VAR "${VAR} additonalStuff")

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 175257 [details]
turn on -ffunction-sections -fdata-sections --gc-section

View in context: https://bugs.webkit.org/attachment.cgi?id=175257&action=review

With regard to the compiler flags, gcc's man page says:

           Only use these options when there are significant benefits from
           doing so.  When you specify these options, the assembler and linker
           will create larger object and executable files and will also be
           slower.  You will not be able to use "gprof" on all systems if you
           specify this option and you may have problems with debugging if you
           specify both this option and -g.

This sounds quite alarming; what if you just change the linker flags? Is there much difference if you compare gold and ld?

With my packager hat on, when a program uses unusual compiler or linker flags by default it tends to trigger my spider-sense; is there any trade-off we're not aware of when enabling these flags?

> Source/cmake/OptionsCommon.cmake:37
> +IF (CMAKE_COMPILER_IS_GNUCC AND UNIX AND NOT APPLE)

This is similar to the check we have in the WEBKIT_SET_EXTRA_COMPILER_FLAGS macro, so I wonder why this was not just put there.

[Laszlo Gombos]

(In reply to comment #3)
> (From update of attachment 175257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175257&action=review
 
> > Source/cmake/OptionsCommon.cmake:37
> > +IF (CMAKE_COMPILER_IS_GNUCC AND UNIX AND NOT APPLE)
> 
> This is similar to the check we have in the WEBKIT_SET_EXTRA_COMPILER_FLAGS macro, so I wonder why this was not just put there.

I am not quite sure what is the best way to determine which rule goes to WebKitHelpers.cmake and OptionsCommon.cmake. OptionsCommon.cmake has similar rules as well (see for example the rule to set no-keep-memory linker flag in certain conditions.

Should I move the "no-keep-memory linker flag" to WebKitHelpers.cmake as well ?

It would be great to add a few lines description to each file to help to understand what goes where.

I will reflect on your other comments once I have some data. Thanks for the help.

[Laszlo Gombos]

(In reply to comment #2)
> (From update of attachment 175257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175257&action=review

> > +    SET(CMAKE_CXX_FLAGS "-ffunction-sections -fdata-sections ${CMAKE_CXX_FLAGS}")
> 
> Usually this is written as SET(VAR "${VAR} additonalStuff")

Patrick, I think the order actually matters and variables that can be tweaked by the user as well should be prepended by the build system and not appended, see also bug 103101. 

You have a point that the patch was not consistent as the compiler flags were prepended but the linker flags were appended - I will change the linker flags so that they are prepended as well.

[Laszlo Gombos]

(In reply to comment #3)
> (From update of attachment 175257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175257&action=review

> With regard to the compiler flags, gcc's man page says:
> 
>            Only use these options when there are significant benefits from
>            doing so. 

From my experience this optimization usually carries significant benefit for large projects where there is potential for dead symbols and where binary size matters. In case of WebKit EFL port I think that ~ 2Mb binary size saving is a significant benefit. 

From other WebKit ports the chromium port (see http://src.chromium.org/viewvc/chrome/trunk/src/build/common.gypi) and Qt port (see http://trac.webkit.org/browser/trunk/Source/WebCore/WebCore.pri) have the same optimization turned on.

Mozilla has it enabled as well - http://hg.mozilla.org/mozilla-central/file/f42a83257653/build/autoconf/compiler-opts.m4.

> When you specify these options, the assembler and linker
>            will create larger object and executable files and will also be
>            slower.

I ran some performance benchmarks (sunspider, v8, dromaeo DOM core tests) and have not been able to observe a difference in execution speed and memory usage (these tests fluctuate quite a bit and the results were always in the same range). If you can think of a particular case where you expect a regression, please let us know and we should try to test it as I certainly could not test it all.

I was also expecting an increased build/linking time, but at least for incremental linking I could not find an evidence of that (only tried with gold).

> You will not be able to use "gprof" on all systems if you specify this option

This is a valid point and we should be able to work around it in the build system (like the other projects did). See also the related discussion about the precendence of user specified build options.

> and [...] you may have problems with debugging if you
>            specify both this option and -g.

I see no compelling reason to turn this on for debug builds. I will change the patch to only enable the flag for release builds.
 
> This sounds quite alarming; what if you just change the linker flags?

These flags should really go hand in hand - this is probably one reason why it is not the default in gcc. The saving in size for the linker-flags-only build is only about 236kB.

> Is there much difference if you compare gold and ld?

Not much. Gold saves 28kB more.

> With my packager hat on, when a program uses unusual compiler or linker flags by default it tends to trigger my spider-sense; is there any trade-off we're not aware of when enabling these flags?

I agree that as most compiler flags this is probably also a trade-off. In WebKit most of the really hot code is already optimized (e.g. JIT). In addition to these new flags in the patch the build system for EFL uses -O3 (which the CMake default) which makes performance a preference over binary size so in this combination of flags I could not demonstrate any performance difference for WebKit.

I would recommend to use these flags in any production environment - and becase of that I think we should also have it set in the buildbot so that it is tested. If some distribution/packager decides that this is not what they want they can just overrule it using environment variables. To disable these options one can use the following build command (I tested it):

build-webkit --efl --cmakearg="-DCMAKE_C_FLAGS='-fno-function-sections -fno-data-sections' -DCMAKE_CXX_FLAGS='-fno-function-sections -fno-data-sections' -DCMAKE_SHARED_LINKER_FLAGS='-Wl,--no-gc-sections'"

[Laszlo Gombos]

Ming or Rob what do you think we should do for the BlackBerry port (which is the only other port using CMake and GCC) ? 

Should this be introduced initially only for the EFL port or we should turn this on for the BlackBerry port as well ?  

Do you have some numbers that you can share in therm of benefits in the BlackBerry environment ?

[Laszlo Gombos]

Created attachment 175870 [details]
turn on optimization only for release builds and make sure that it can be disabled by a packager

[WebKit Review Bot]

Comment on attachment 175870 [details]
turn on optimization only for release builds and make sure that it can be disabled by a packager

Attachment 175870 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14984338

New failing tests:
svg/W3C-SVG-1.1/animate-elem-78-t.svg

[Ming Xie]

I tried this out with our BlackBerry port, and I see the binary size went up. :-(

I talked to our tools guy, and he said, "These functions can lead to bloat if there's no sections to garbage collect."

Let's leave this out from the BlackBerry port if you guys want to land this now.

[Ryuan Choi]

(In reply to comment #10)
> I tried this out with our BlackBerry port, and I see the binary size went up. :-(
> 
> I talked to our tools guy, and he said, "These functions can lead to bloat if there's no sections to garbage collect."
> 
> Let's leave this out from the BlackBerry port if you guys want to land this now.

In tizen, it reduced binary size about 3Mb

[Laszlo Gombos]

Comment on attachment 175870 [details]
turn on optimization only for release builds and make sure that it can be disabled by a packager

Removing r? as this patch needs to be reworked. I plan to create a patch that turns this optimization on just for the EFL port after I run a few more tests.

[Laszlo Gombos]

(In reply to comment #10)
> I tried this out with our BlackBerry port, and I see the binary size went up. :-(
> 
> I talked to our tools guy, and he said, "These functions can lead to bloat if there's no sections to garbage collect."
> 
> Let's leave this out from the BlackBerry port if you guys want to land this now.

Thanks Ming, I appreciate for checking this. It seems that there is plenty of garbage to collect for the EFL port at the moment (and this might change over time). It is certainly interesting to see such a big difference in different ports and different tool chain.

[Laszlo Gombos]

Created attachment 177640 [details]
patch for the EFL port only

[Gyuyoung Kim]

Comment on attachment 177640 [details]
patch for the EFL port only

I agree to land this for EFL and Tizen platform. CMake guys might want to have a final look.

[Gyuyoung Kim]

Comment on attachment 177640 [details]
patch for the EFL port only

I'd like to set r+ again after getting cmake guys informal review.

[Laszlo Gombos]

Created attachment 178021 [details]
for cq to land

Rebased.

[WebKit Review Bot]

Comment on attachment 178021 [details]
for cq to land

Clearing flags on attachment: 178021

Committed r136854: <http://trac.webkit.org/changeset/136854>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.