Bug 102647 – [CMake] Add an option to build AllInOne files

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

[Laszlo Gombos]

AllInOne files help to reduce "macro bloat" and makes the resulting binary smaller. This could be useful both in development and production environments.

AllInOne files should be turn off by default for development on WebKit.org trunk to make sure that incremental builds are fast for development.

[Laszlo Gombos]

Created attachment 174895 [details]
1st try

For EFL default release build (build-webkit --efl --cmakearg="-DENABLE_ALLINONE=TRUE") the WebKit2 binary is 1.95% (840kB) smaller.

This is the first step to introduce this optimization to get a feedback on the direction. The Mac and Qt (and perhaps other) ports have this option as well.

[WebKit Review Bot]

Comment on attachment 174895 [details]
1st try

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

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Patrick R. Gansterer]

Comment on attachment 174895 [details]
1st try

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

> Source/WebCore/ChangeLog:10
> +        when ENABLE_ALLINONE cmake option is set.

AFAIK there is no ENABLE(ALLINONE) in the C++ files, so i'd suggest a name without ENABLE_ prefix (BTW: even it is a C++ define, then it should be USE() and not ENABLE())
Did you thought about reading the *AllInOne-Files, and let CMake remove the include files from its source list? this would avoid maintainig the list twice. I don't know if it is practicable, only an idea.

> Source/WebCore/ChangeLog:12
> +        No new tests (OOPS!).

no reason to inlcude this line

> Source/WebCore/CMakeLists.txt:2246
> +  LIST(APPEND WebCore_SOURCES

4 spaces indent

[Raphael Kubo da Costa (:rakuco)]

Looking at the logs for these AllInOne.cpp files they were generally created to address build problems on Win32.

I didn't get what you meant by "macro bloat" in this case.

The -DENABLE_ALLINONE approach doesn't look very clean to me -- IMHO we should either always use these files or not use them (the Mac approach seems to be to always use AllInOne files on Win32 in release mode and the regular files in all other configurations).

[Alexis Menard (darktears)]

(In reply to comment #4)
> Looking at the logs for these AllInOne.cpp files they were generally created to address build problems on Win32.
> 
> I didn't get what you meant by "macro bloat" in this case.
> 
> The -DENABLE_ALLINONE approach doesn't look very clean to me -- IMHO we should either always use these files or not use them (the Mac approach seems to be to always use AllInOne files on Win32 in release mode and the regular files in all other configurations).

I tend to agree with Raphael. From what I recall AllinOne files were added to workaround the fact that the windows 32 bits linker couldn't handle linking WebKit. Compare it to linking a debug version of WebKit on a 32 bits machine with 2GB of RAM. A point of time the link will fail because the kernel will kill ld. I think they were added back then to actually make the linking possible. I don't think they bring any benefits maybe by side effects. I'm not that good with Windows in depth bit but that's what I remember. Now is that even valid for modern MSVC, even on 32 bits? Good question that would need testing.

[Laszlo Gombos]

(In reply to comment #5)

> Now is that even valid for modern MSVC, even on 32 bits? Good question that would need testing.

My main interest is optimize the Linux release binary for EFL (not for Windows at the moment) and other CMake based ports that are interested to share this optimization.

As mentioned in comment#2, this build flag brings in 800 Kb binary size reduction at the moment - which is a nice property for limited/embedded systems (also helps with over the air binary distribution).

[Laszlo Gombos]

(In reply to comment #1)
> Created an attachment (id=174895) [details]
> 1st try
> 
> For EFL default release build (build-webkit --efl --cmakearg="-DENABLE_ALLINONE=TRUE") the WebKit2 binary is 1.95% (840kB) smaller.

After stripping the binaries the gain is even bigger on Linux release binary for the EFL port - 876kB - (2.58% gain).

[Laszlo Gombos]

Comment on attachment 174895 [details]
1st try

> After stripping the binaries the gain is even bigger on Linux release binary for the EFL port - 876kB - (2.58% gain).

Taking this idea one step further further - after stripping _and_ introducing binary optimization (see bug 102827) the gain on binary size saving seems to increase to 923kb. The important observation here that the 2 optimization technique (bug 102647 and bug 102827) seems to be independent.

As a side note AllInOne build option also seems to decrease the build time (for me about 10%) for a full (non-incremental) build.

I will update the patch to reflect on comments.

[Gergő Balogh]

Created attachment 217046 [details]
patch

New cmakearg added (AllInOne), it could be used to enable AllInOne files during build (AllInOne=true). Details could be logged with AllInOne=debug.

[Patrick R. Gansterer]

Comment on attachment 217046 [details]
patch

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

i'd suggest a WebCore_ALLINONE_SOURCES variable and handle it in the WEBKIT_WRAP_SOURCES macro.
do the many status messages provide a real benefit? they look a kind of printf-debugging to me (just for implementing the allinone stuff for cmake)

> Source/WebCore/CMakeLists.txt:2558
> +set(AllInOne "" CACHE STRING "If true, enables all-in-one files.")

if its a global switch it should be move to the root cmake file

> Source/WebCore/CMakeLists.txt:2559
> +if (AllInOne)

usually global variables are all uppercase

> Source/WebCore/CMakeLists.txt:2592
> +    #list(GET allInOnes 0 allInOne)

remove comment?

> Source/WebCore/CMakeLists.txt:2601
> +    endforeach (allInOne)

please remove the characters from the ()

> Source/WebCore/CMakeLists.txt:2602
> +endif (AllInOne)

too

> Source/cmake/AllInOneSupport.cmake:1
> +cmake_minimum_required(VERSION 2.8)

imho not a good idea to spred this across the whole code

[Csaba Osztrogonác]

Created attachment 256476 [details]
WIP patch

The build works on EFL, the patch reduced the number of cmake targets from 5250 to 4396, reduced the build time from 13 mins to 10 mins, reduced the stripped binary size with 900Kb (2%).

[WebKit Commit Bot]

Attachment 256476 [details] did not pass style-queue:


ERROR: Source/WebCore/CMakeLists.txt:2793:  Alphabetical sorting problem. "#FIXME: css/MediaAllInOne.cpp have some build errors, need bug report" should be before "css/CSSAllInOne.cpp".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:2794:  Alphabetical sorting problem. "#        css/MediaAllInOne.cpp" should be before "#FIXME: css/MediaAllInOne.cpp have some build errors, need bug report".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:2808:  Alphabetical sorting problem. "accessibility/AccessibilityAllInOne.cpp" should be before "svg/SVGAllInOne.cpp".  [list/order] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Laszlo Gombos]

I am still interested to review and land this if this is up for review. Thanks.

[Csaba Osztrogonác]

(In reply to comment #13)
> I am still interested to review and land this if this is up for review.
> Thanks.

Cool, I'm going to upload the updated version for review next week.

[Alex Christensen]

bug147484 can be landed separately, but we will need this.
Please make the default on Windows to use the all in ones.

[Alex Christensen]

This adds several seconds to the CMake generation time.  I would prefer putting them in separate lists, but it's no big deal if everyone agrees that this is significantly easier to maintain.

[Csaba Osztrogonác]

(In reply to comment #16)
> This adds several seconds to the CMake generation time.  I would prefer
> putting them in separate lists, but it's no big deal if everyone agrees that
> this is significantly easier to maintain.

I measured the cmake generation time, but this change added only 
1-2 seconds. Have you measured significant slowdown on Windows?

Nowadays we notice build failures regularly when folks miss to
add a new source file to the related allinone file. Breaking
the cmake source list to several pieces would confuse folks
where to add a new cpp file and would cause more build failures.

So I still prefer this patch to duplicated allinone source list.

I'm going to update and polish this patch and upload for review today.

[Csaba Osztrogonác]

Created attachment 258058 [details]
Patch

tested on EFL only

[Csaba Osztrogonác]

(In reply to comment #18)
> Created attachment 258058 [details]
> Patch
> 
> tested on EFL only

forgot to mention that fix in bug147557 is needed 
to have working all-in-one build on EFL.

[Csaba Osztrogonác]

Created attachment 258059 [details]
Patch for EWS

enabled on GTK and EFL too for EWS testing, not for landing

[Csaba Osztrogonác]

(In reply to comment #20)
> Created attachment 258059 [details]
> Patch
> enabled on GTK and EFL too for EWS testing, not for landing

Build is happy with enabled and disabled all-in-one files on EFL/GTK.
Alex, could you check the Windows/WinCairo build?

[Alex Christensen]

Comment on attachment 258058 [details]
Patch

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

r=me. This added 2-3 seconds to my build.  Not bad, but if we got everything in the respective directories included in the *AllInOne.cpp, we could just have separate properly-named lists in WebCore/CMakeLists.txt and it would be quite easy to keep track of which files need to be added to the *AllInOne.cpp.  This could be done in a future patch, though.

> Source/cmake/OptionsGTK.cmake:146
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ALLINONE_BUILD PRIVATE OFF)

Why is this private in GTK, but public in EFL and Windows?

[WebKit Commit Bot]

Comment on attachment 258058 [details]
Patch

Clearing flags on attachment: 258058

Committed r187818: <http://trac.webkit.org/changeset/187818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #22)
> Comment on attachment 258058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258058&action=review
> 
> r=me. This added 2-3 seconds to my build.  Not bad, but if we got everything
> in the respective directories included in the *AllInOne.cpp, we could just
> have separate properly-named lists in WebCore/CMakeLists.txt and it would be
> quite easy to keep track of which files need to be added to the
> *AllInOne.cpp.  This could be done in a future patch, though.
> 
> > Source/cmake/OptionsGTK.cmake:146
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ALLINONE_BUILD PRIVATE OFF)
> 
> Why is this private in GTK, but public in EFL and Windows?

As far as I know GTK port exposes only a few options to the 
users, other ports don't care about it and expose everything.

That's why I think it is the most conservative thing is not to expose 
all-in-one option on GTK. But if the GTK folks would like to expose it,
feel free to do it in a follow-up patch.

[Michael Catanzaro]

(In reply to comment #25)
> As far as I know GTK port exposes only a few options to the 
> users, other ports don't care about it and expose everything.

Yeah, keeping the list of public options smaller is great; with too many options, it's impossible to keep them all supported and building, and it's harder for people who look at the list of options to think about what they want. I agree this doesn't need to be a public option for GTK, since if you're developing WebKit you will always want it off. But if you are distributing WebKit you probably always want it on, to reduce the size of the binary, right? If I'm correct in saying the right choice is this clear, then we should make it depend on DEVELOPER_MODE.