Bug 32128 – [Chromium] webcore_bindings target builds too slowly

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[Jens Alfke]

In Chromium, WebCore has a separate build target "webcore_bindings" that contains DerivedSourcesAllInOne.cpp and a handful of other files. This setup is slowing down the build
process on my Mac Pro, because DerivedSourcesAllInOne takes much longer to compile than any of the other sources in that target (since it #includes about 100 .cpp files), so there's a period of 30-45 seconds where one of the 8 cores is compiling that one file while the others are twiddling their thumbs.

The apparent reason for this is given in a comment at the top of DSAIO.cpp: "to reduce bloat and allow us to link release builds on 32-bit Windows."

Here's a proposal for speeding this up.

First, break DerivedSourcesAllInOne.cpp up into four pieces. This is easy since it just consists of a bunch of #includes of other .cpp files. Just move 1/4 of the lines (plus the required header #includes at the top) into three new source files.
	DerivedSourcesAllInOne.cpp
	DerivedSourcesAllInTwo.cpp
	DerivedSourcesAllInThree.cpp
	DerivedSourcesAllInFour.cpp
These four files can now build in parallel, cutting down compile time.

I'm assuming that the issues with the Windows release build still hold: so in some circumstances we need to build these all as one file. So we can add some conditional compilation magic. Here "NEED_TO_BUILD_ALL_IN_ONE" represents some compile-time expression that determines whether we need to build this as one file.

Append this to DerivedSourcesAllInOne:
	#if NEED_TO_BUILD_ALL_IN_ONE
	#define ALLINONE
	#include "DerivedSourcesAllInTwo.cpp"
	#include "DerivedSourcesAllInThree.cpp"
	#include "DerivedSourcesAllInFour.cpp"
	#endif

Wrap the other three files with:
	#if !NEED_TO_BUILD_ALL_IN_ONE || defined(ALLINONE)
	....body of file...
	#endif

[Evan Martin]

Created attachment 48751 [details]
derivedsources hack

[Evan Martin]

This patch is a quick hack, so no r? since it lacks a changelog, nor the ifdef idea Jens wrote.  (I actually wrote this patch after forgetting about this bug report... I thought I was so clever with the "DerivedSourcesAllInTwo" naming but clearly I just remembered that from this bug.)

[Jeremy Orlow]

Comment on attachment 48751 [details]
derivedsources hack

> diff --git a/WebCore/WebCore.gyp/WebCore.gyp b/WebCore/WebCore.gyp/WebCore.gyp
> index e40da56..5734c12 100644
> --- a/WebCore/WebCore.gyp/WebCore.gyp
> +++ b/WebCore/WebCore.gyp/WebCore.gyp
> @@ -579,6 +579,8 @@
>          # This file includes all the .cpp files generated from the .idl files
>          # in webcore_files.
>          '../bindings/v8/DerivedSourcesAllInOne.cpp',
> +        '../bindings/v8/DerivedSourcesAllInTwo.cpp',
> +        '../bindings/v8/DerivedSourcesAllInThree.cpp',
>  
>          # Additional .cpp files from webcore_bindings_sources actions.
>          '<(SHARED_INTERMEDIATE_DIR)/webkit/HTMLElementFactory.cpp',

Hm.  Seems like maybe we should have it do this for release but not debug?  Maybe in release, AllInOne could then include the others and gyp would not compile them?

> diff --git a/WebCore/bindings/v8/DerivedSourcesAllInThree.cpp b/WebCore/bindings/v8/DerivedSourcesAllInThree.cpp
> new file mode 100644
> index 0000000..e7c10b7
> --- /dev/null
> +++ b/WebCore/bindings/v8/DerivedSourcesAllInThree.cpp
> @@ -0,0 +1,201 @@

Need copyright/license.

> diff --git a/WebCore/bindings/v8/DerivedSourcesAllInTwo.cpp b/WebCore/bindings/v8/DerivedSourcesAllInTwo.cpp
> new file mode 100644
> index 0000000..00f012d
> --- /dev/null
> +++ b/WebCore/bindings/v8/DerivedSourcesAllInTwo.cpp
> @@ -0,0 +1,126 @@

Ditto on copyright/license.  + config.


Any reason you chose to split it into 3?  Maybe a bit more would be better?


Thanks a lot for doing this!  With these changes, I think we could actually check this in as default!

[Evan Martin]

DerivedSourcesAllInOne.o takes many minutes to compile and is 19mb.

I sorted the non-conditional parts alphabetically then split it so the resulting split .o files are more balanced:

1: 3.4M
2: 4.6M
3: 9.9M
4: 6.4M

Now they individually take <10s to compile, and can be parallelized 4 ways.

The vast majority of the code weight is again my arch-nemesis SVG.  When I put the SVG bits in their own file they were a full 12mb themselves.  So now they are split across parts 3 and 4.  :(

Patch coming up in a sec.

[Evan Martin]

Created attachment 48887 [details]
patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/DerivedSourcesPartTwo.cpp:37:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesPartFour.cpp:41:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesAllInOne.cpp:35:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesAllInOne.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesAllInOne.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesPartThree.cpp:40:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesPartOne.cpp:37:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 7


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

[Evan Martin]

Created attachment 48889 [details]
patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/DerivedSourcesPartTwo.cpp:37:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesPartFour.cpp:41:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesAllInOne.cpp:35:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesAllInOne.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesAllInOne.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesPartThree.cpp:40:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/DerivedSourcesPartOne.cpp:37:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 7


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

[Evan Martin]

I believe the style queue complaints here can be ignored, since these files are abusing the preprocessor.

(E.g. it doesn't like that these aren't alphabetically sorted:
#include "DerivedSourcesPartOne.cpp"
#include "DerivedSourcesPartTwo.cpp"
#include "DerivedSourcesPartThree.cpp"
#include "DerivedSourcesPartFour.cpp"
)

[Jeremy Orlow]

Comment on attachment 48889 [details]
patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +    # If set to 1, combines all v8 derived sources into a single.cpp
> +    # file.  This takes much longer to compile but results in a faster
> +    # binary and is also used to work around a linker problem on
> +    # Windows.
> +    'combine_derived_sources%': 1,

We'll need to widely publicize this.


LGTM, but it's probably best Dimitri take a look as well.  :-)

[Dimitri Glazkov (Google)]

Comment on attachment 48889 [details]
patch

I think this will work, but it's not the right solution. The right solution is to generate DerivedSourcesAllInOne conditionally.

[Dimitri Glazkov (Google)]

*** This bug has been marked as a duplicate of bug 33048 ***

[Jeremy Orlow]

Can we please either streamline the other patch in (it's been sitting there for 2 months) or take this one, Dimitri?