Bug 102647 - [CMake] Add an option to build AllInOne files
Summary: [CMake] Add an option to build AllInOne files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on: 146586 146580 146582 146584 146587 147557
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-18 22:44 PST by Laszlo Gombos
Modified: 2015-08-04 06:15 PDT (History)
16 users (show)

See Also:


Attachments
1st try (13.71 KB, patch)
2012-11-18 22:58 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff
patch (5.30 KB, patch)
2013-11-15 06:59 PST, Gergő Balogh
no flags Details | Formatted Diff | Diff
WIP patch (4.71 KB, patch)
2015-07-09 03:04 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (9.34 KB, patch)
2015-08-03 07:57 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch for EWS (15.40 KB, patch)
2015-08-03 08:05 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2012-11-18 22:44:28 PST
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.
Comment 1 Laszlo Gombos 2012-11-18 22:58:00 PST
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.
Comment 2 WebKit Review Bot 2012-11-19 00:02:25 PST
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
Comment 3 Patrick R. Gansterer 2012-11-19 00:58:02 PST
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
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-11-19 07:52:26 PST
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).
Comment 5 Alexis Menard (darktears) 2012-11-19 10:21:53 PST
(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.
Comment 6 Laszlo Gombos 2012-11-19 10:31:48 PST
(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).
Comment 7 Laszlo Gombos 2012-11-19 10:40:51 PST
(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).
Comment 8 Laszlo Gombos 2012-11-20 13:46:51 PST
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.
Comment 9 Gergő Balogh 2013-11-15 06:59:53 PST
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.
Comment 10 Patrick R. Gansterer 2013-11-15 13:15:02 PST
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
Comment 11 Csaba Osztrogonác 2015-07-09 03:04:05 PDT
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%).
Comment 12 WebKit Commit Bot 2015-07-09 03:05:33 PDT
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.
Comment 13 Laszlo Gombos 2015-07-10 14:00:32 PDT
I am still interested to review and land this if this is up for review. Thanks.
Comment 14 Csaba Osztrogonác 2015-07-31 06:55:38 PDT
(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.
Comment 15 Alex Christensen 2015-07-31 10:19:10 PDT
bug147484 can be landed separately, but we will need this.
Please make the default on Windows to use the all in ones.
Comment 16 Alex Christensen 2015-07-31 13:47:11 PDT
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.
Comment 17 Csaba Osztrogonác 2015-08-03 04:02:29 PDT
(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.
Comment 18 Csaba Osztrogonác 2015-08-03 07:57:20 PDT
Created attachment 258058 [details]
Patch

tested on EFL only
Comment 19 Csaba Osztrogonác 2015-08-03 07:58:37 PDT
(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.
Comment 20 Csaba Osztrogonác 2015-08-03 08:05:52 PDT
Created attachment 258059 [details]
Patch for EWS

enabled on GTK and EFL too for EWS testing, not for landing
Comment 21 Csaba Osztrogonác 2015-08-03 08:42:25 PDT
(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?
Comment 22 Alex Christensen 2015-08-03 13:19:45 PDT
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?
Comment 23 WebKit Commit Bot 2015-08-03 23:49:47 PDT
Comment on attachment 258058 [details]
Patch

Clearing flags on attachment: 258058

Committed r187818: <http://trac.webkit.org/changeset/187818>
Comment 24 WebKit Commit Bot 2015-08-03 23:49:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Csaba Osztrogonác 2015-08-04 02:42:11 PDT
(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.
Comment 26 Michael Catanzaro 2015-08-04 06:15:45 PDT
(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.