New issue
Advanced search Search tips

Issue 634231 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Generated .cpp files for bindings layer should be compiled independently

Project Member Reported by tzik@chromium.org, Aug 4 2016

Issue description

I found the compilation of V8GeneratedCoreBindings*.cpp takes long time.
They takes around 180 seconds each, and one of them takes 440 seconds on Windows.

Can we break them into pieces for faster build, or do some better handling to these generated code?
 
Cc: -peria@chromium.org
Components: Blink>Bindings
Owner: peria@chromium.org

Comment 2 by peria@chromium.org, Aug 4 2016

Labels: OS-All
Those V8GeneratedCoreBindings*.cpp files include other V8X.cpp generated from X.idl files.
I think we can specify V8X.cpp and V8X.h as source in .gn/.gni files, each compilation
can be run in parallel.

And as another concern of this issue is to run needless compilations.
Assume V8GeneratedCoreBindings0.cpp includes V8A.cpp and V8B.cpp, and we update A.idl.
Then we don't have to compile V8B.cpp, but do it through V8GeneratedCoreBindings0.cpp.
So if we can split compilations, we may skip some compilations.

Comment 3 by bashi@chromium.org, Aug 4 2016

+1 to stop aggregating generated file. Actually, when I took over IDL stuff from nbarth@ he suggested to stop aggregating. (I'll share you a doc nbarth@ wrote)

You don't need to worry about recompilation as the code generator won't touch files if there is no diff. See write_file() in utilities.py

Comment 4 by peria@chromium.org, Aug 4 2016

Status: Started (was: Assigned)
Summary: Generated .cpp files for bindings layer should be compiled independently (was: Slow V8GeneratedCoreBindings*.cpp compilation)
Hmm, we need to resolve another issue to work for this issue.

Generated files may not include enough files to be compiled.
For example, V8Location.cpp, generated from interface_base.cpp, uses toLocalWindow() function
that is defined in core/frame/LocalDOMWindow.h, but it does not include the header file.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a213fd43d3915932361089a18ffdca4390775d4

commit 2a213fd43d3915932361089a18ffdca4390775d4
Author: peria <peria@chromium.org>
Date: Thu Aug 04 14:38:19 2016

Put a prototype class of DOMException.

We have no declaration of DOMException in header files
recursively included from ErrorCallback.h.
Compiles passed well by chance.

BUG= 634231 

Review-Url: https://codereview.chromium.org/2217453002
Cr-Commit-Position: refs/heads/master@{#409782}

[modify] https://crrev.com/2a213fd43d3915932361089a18ffdca4390775d4/third_party/WebKit/Source/modules/filesystem/ErrorCallback.h

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb4c3ca5ce05cb31f699401813f0664ee70fda19

commit eb4c3ca5ce05cb31f699401813f0664ee70fda19
Author: peria <peria@chromium.org>
Date: Thu Aug 04 15:02:30 2016

Fix compile errors ignored by binding aggregations

In binding layer, we compile many .cpp file at once,
aggregated into few .cpp files.
This aggregation hides some lacks of dependency, and
it strongly dependent on current aggregation style.
(For example, crrev.com/2216563002 makes it obvious)

This CL fixes those hidden compile errors in generated code.

BUG= 634231 

Review-Url: https://codereview.chromium.org/2210973002
Cr-Commit-Position: refs/heads/master@{#409785}

[modify] https://crrev.com/eb4c3ca5ce05cb31f699401813f0664ee70fda19/third_party/WebKit/Source/bindings/scripts/v8_interface.py
[modify] https://crrev.com/eb4c3ca5ce05cb31f699401813f0664ee70fda19/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp
[modify] https://crrev.com/eb4c3ca5ce05cb31f699401813f0664ee70fda19/third_party/WebKit/Source/core/html/HTMLFrameSetElement.h

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a77ec79a00851a95760c94c0fe9daa02ce44ab6c

commit a77ec79a00851a95760c94c0fe9daa02ce44ab6c
Author: peria <peria@chromium.org>
Date: Thu Aug 04 16:44:15 2016

Increase the number of files to aggregate generated .cpp
files from .idl files.
(This change is a workaround change.)

It is expected to reduce compile time because of following reasons.
- The number of lines to be processed in one compile reduced.
- The number of compile tasks increased, so they can be run in parallel.

BUG= 634231 

Review-Url: https://codereview.chromium.org/2216563002
Cr-Commit-Position: refs/heads/master@{#409809}

[modify] https://crrev.com/a77ec79a00851a95760c94c0fe9daa02ce44ab6c/third_party/WebKit/Source/bindings/core/v8/generated.gni
[modify] https://crrev.com/a77ec79a00851a95760c94c0fe9daa02ce44ab6c/third_party/WebKit/Source/bindings/core/v8/generated.gypi
[modify] https://crrev.com/a77ec79a00851a95760c94c0fe9daa02ce44ab6c/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[modify] https://crrev.com/a77ec79a00851a95760c94c0fe9daa02ce44ab6c/third_party/WebKit/Source/bindings/modules/v8/generated.gypi

Comment 8 by peria@chromium.org, Aug 17 2016

There is another concern in aggregating .cpp files;
if we use anonymous namespace in generated .cpp files, names of variables
or functions defined in the anonymous namespace can conflict.
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df0a0c5106ff224578092178373faca20bca2b02

commit df0a0c5106ff224578092178373faca20bca2b02
Author: peria <peria@chromium.org>
Date: Thu Aug 18 06:21:59 2016

Specify if we aggregate partial interfaces by an argument.

Before this CL, file names to include were decided with
the path of original IDL files.
This behavior could mislead us as if we included generated
files for partial interface and non-partial interface
in one aggregation file, although we aggregate files for
all partial interfaces into one file.

This change make it obvious in .gn/.gyp files to aggregate
files for (non-)partial interfaces.

BUG= 634231 

Review-Url: https://codereview.chromium.org/2259463002
Cr-Commit-Position: refs/heads/master@{#412756}

[modify] https://crrev.com/df0a0c5106ff224578092178373faca20bca2b02/third_party/WebKit/Source/bindings/core/v8/BUILD.gn
[modify] https://crrev.com/df0a0c5106ff224578092178373faca20bca2b02/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/df0a0c5106ff224578092178373faca20bca2b02/third_party/WebKit/Source/bindings/modules/v8/generated.gyp
[modify] https://crrev.com/df0a0c5106ff224578092178373faca20bca2b02/third_party/WebKit/Source/bindings/scripts/aggregate_generated_bindings.py
[modify] https://crrev.com/df0a0c5106ff224578092178373faca20bca2b02/third_party/WebKit/Source/bindings/scripts/scripts.gni

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 8 2016

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9

commit d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9
Author: peria <peria@chromium.org>
Date: Thu Sep 08 08:31:15 2016

Revert of [Bindings] Remove aggregation of generated binding code (patchset #3 id:60001 of https://codereview.chromium.org/2318933002/ )

Reason for revert:
Build break
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/builds/10652

Original issue's description:
> Remove aggregation of generated binding code.
>
> After this change, we maybe able to skip needless compiles
> based on redundant dependencies. ( issue 388172 )
> Plus, it is expected to compile those files in parallel.
>
> BUG= 634231 ,  388172 
>
> Committed: https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55
> Cr-Commit-Position: refs/heads/master@{#417216}

TBR=bashi@chromium.org,yukishiino@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 634231 ,  388172 

Review-Url: https://codereview.chromium.org/2325583002
Cr-Commit-Position: refs/heads/master@{#417225}

[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/core/v8/BUILD.gn
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[add] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/scripts/aggregate_generated_bindings.py
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/scripts/generate_init_partial_interfaces.py
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/scripts/scripts.gni
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/modules/BUILD.gn

Project Member

Comment 13 by bugdroid1@chromium.org, Sep 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f82e43808017ab95d877be86eb63fe1e4057e97

commit 0f82e43808017ab95d877be86eb63fe1e4057e97
Author: peria <peria@chromium.org>
Date: Thu Sep 08 08:32:23 2016

Revert of [Bindings] Remove aggregation of generated binding code (patchset #3 id:60001 of https://codereview.chromium.org/2318933002/ )

Reason for revert:
Build break
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/builds/10652

Original issue's description:
> Remove aggregation of generated binding code.
>
> After this change, we maybe able to skip needless compiles
> based on redundant dependencies. ( issue 388172 )
> Plus, it is expected to compile those files in parallel.
>
> BUG= 634231 ,  388172 
>
> Committed: https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55
> Cr-Commit-Position: refs/heads/master@{#417216}

TBR=bashi@chromium.org,yukishiino@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 634231 ,  388172 

Review-Url: https://codereview.chromium.org/2325583002
Cr-Commit-Position: refs/heads/master@{#417226}

[modify] https://crrev.com/0f82e43808017ab95d877be86eb63fe1e4057e97/third_party/WebKit/Source/bindings/scripts/scripts.gni

Project Member

Comment 14 by bugdroid1@chromium.org, Sep 8 2016

Labels: merge-merged-2854
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/134a59bd48149d721ce06554570db1b2b5bdee55

commit 134a59bd48149d721ce06554570db1b2b5bdee55
Author: peria <peria@chromium.org>
Date: Thu Sep 08 07:14:41 2016

Remove aggregation of generated binding code.

After this change, we maybe able to skip needless compiles
based on redundant dependencies. ( issue 388172 )
Plus, it is expected to compile those files in parallel.

BUG= 634231 ,  388172 

Review-Url: https://codereview.chromium.org/2318933002
Cr-Commit-Position: refs/heads/master@{#417216}

[modify] https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55/third_party/WebKit/Source/bindings/core/v8/BUILD.gn
[modify] https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[delete] https://crrev.com/9fd8459b286577c2c4fa8becfae92101765bf2f1/third_party/WebKit/Source/bindings/scripts/aggregate_generated_bindings.py
[modify] https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55/third_party/WebKit/Source/bindings/scripts/generate_init_partial_interfaces.py
[modify] https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55/third_party/WebKit/Source/bindings/scripts/scripts.gni
[modify] https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55/third_party/WebKit/Source/modules/BUILD.gn

Project Member

Comment 15 by bugdroid1@chromium.org, Sep 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9

commit d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9
Author: peria <peria@chromium.org>
Date: Thu Sep 08 08:31:15 2016

Revert of [Bindings] Remove aggregation of generated binding code (patchset #3 id:60001 of https://codereview.chromium.org/2318933002/ )

Reason for revert:
Build break
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/builds/10652

Original issue's description:
> Remove aggregation of generated binding code.
>
> After this change, we maybe able to skip needless compiles
> based on redundant dependencies. ( issue 388172 )
> Plus, it is expected to compile those files in parallel.
>
> BUG= 634231 ,  388172 
>
> Committed: https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55
> Cr-Commit-Position: refs/heads/master@{#417216}

TBR=bashi@chromium.org,yukishiino@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 634231 ,  388172 

Review-Url: https://codereview.chromium.org/2325583002
Cr-Commit-Position: refs/heads/master@{#417225}

[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/core/v8/BUILD.gn
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[add] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/scripts/aggregate_generated_bindings.py
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/scripts/generate_init_partial_interfaces.py
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/bindings/scripts/scripts.gni
[modify] https://crrev.com/d7586a77bd18573597cc48bbd2e0e6fb0fbc0fb9/third_party/WebKit/Source/modules/BUILD.gn

Project Member

Comment 16 by bugdroid1@chromium.org, Sep 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f82e43808017ab95d877be86eb63fe1e4057e97

commit 0f82e43808017ab95d877be86eb63fe1e4057e97
Author: peria <peria@chromium.org>
Date: Thu Sep 08 08:32:23 2016

Revert of [Bindings] Remove aggregation of generated binding code (patchset #3 id:60001 of https://codereview.chromium.org/2318933002/ )

Reason for revert:
Build break
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/builds/10652

Original issue's description:
> Remove aggregation of generated binding code.
>
> After this change, we maybe able to skip needless compiles
> based on redundant dependencies. ( issue 388172 )
> Plus, it is expected to compile those files in parallel.
>
> BUG= 634231 ,  388172 
>
> Committed: https://crrev.com/134a59bd48149d721ce06554570db1b2b5bdee55
> Cr-Commit-Position: refs/heads/master@{#417216}

TBR=bashi@chromium.org,yukishiino@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 634231 ,  388172 

Review-Url: https://codereview.chromium.org/2325583002
Cr-Commit-Position: refs/heads/master@{#417226}

[modify] https://crrev.com/0f82e43808017ab95d877be86eb63fe1e4057e97/third_party/WebKit/Source/bindings/scripts/scripts.gni

Project Member

Comment 17 by bugdroid1@chromium.org, Sep 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aff7c7109dd3651121f94ceea720464844c86ec3

commit aff7c7109dd3651121f94ceea720464844c86ec3
Author: peria <peria@chromium.org>
Date: Mon Sep 12 14:35:22 2016

Remove aggregation of generated binding code.

After this change, we maybe able to skip needless compiles
based on redundant dependencies. ( issue 388172 )
Plus, it is expected to compile those files in parallel.

This CL is same with http://crrev.com/2318933002/ with a fix.
The fix is to aggregate them only on Windows Official build,
where we need to preserve symbol table.

BUG= 634231 ,  388172 

Review-Url: https://codereview.chromium.org/2325973002
Cr-Commit-Position: refs/heads/master@{#417935}

[modify] https://crrev.com/aff7c7109dd3651121f94ceea720464844c86ec3/third_party/WebKit/Source/bindings/core/v8/BUILD.gn
[modify] https://crrev.com/aff7c7109dd3651121f94ceea720464844c86ec3/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/aff7c7109dd3651121f94ceea720464844c86ec3/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[modify] https://crrev.com/aff7c7109dd3651121f94ceea720464844c86ec3/third_party/WebKit/Source/bindings/scripts/aggregate_generated_bindings.py
[modify] https://crrev.com/aff7c7109dd3651121f94ceea720464844c86ec3/third_party/WebKit/Source/bindings/scripts/generate_init_partial_interfaces.py
[modify] https://crrev.com/aff7c7109dd3651121f94ceea720464844c86ec3/third_party/WebKit/Source/bindings/scripts/scripts.gni
[modify] https://crrev.com/aff7c7109dd3651121f94ceea720464844c86ec3/third_party/WebKit/Source/modules/BUILD.gn

Comment 18 by peria@chromium.org, Sep 15 2016

Status: Fixed (was: Started)
Static build on Windows, we need an aggregation to minimize image data. [LNK1248]
We've done for other platforms, so let me close this issue as fixed.

Comment 19 by brat...@opera.com, Mar 27 2017

It is a bit ironic, considering that  bug 388172  is about reducing compilation times, that this change might have achieved the opposite.

Building in "windows" mode my test compilations of bindings need 
108 seconds real (wall clock) time.
381 seconds CPU time.

Building in "linux/mac" mode with many small files it needs
379 seconds real (wall clock) time.
2796 seconds CPU time.

I looked at it because it's the most time consuming folder during compilation and I was investigating the potential gains from unity builds.


Comment 20 by brat...@opera.com, Mar 27 2017

s/2796/2874/ CPU seconds. I forgot to add kernel time to that one.

So the split up build uses about 8 times more CPU time it would seem. For someone with *a lot* of parallel cores it might still be faster or about as fast. For people with more ordinary computers it becomes quite a lot of minutes slower.

Comment 21 by peria@chromium.org, Mar 27 2017

Re; #19,20
Yes, it can happen.

I'm not sure which one is the critical one, but I think up some reasons.
One is parallel I/O on disk. It depends on the number of parallelized process and HDD/SSD you use.
One is #include's. In Windows style, all .cpp files are compiled as a one .cpp file, so it ignores many duplicated .h files, while all .h files are included in  linux/Mac OSX style.

But anyway, we are not working to fix the issue for now, because
(1) if the number of processes can resolve it, now we don't have a way to control the number in GN,
(2) most Chormium developers are in Google and using Goma (distributed compiling system), and this is not problematic for them, and
(3) the windows style has a potential problems. I already fixed some of them, but it is impossible not to introduce new and same type problems.

Comment 22 by brat...@opera.com, Mar 27 2017

Yes, this is really tricky. Optimizing the build for scenario with a 1000 (virtual) parallel cores is clearly the right thing for people with that kind of (virtual) hardware, but on the other hand there are also quite a lot of people outside that would like to contribute or work at companies that build products based on Chromium and those are discouraged by the hardware requirements.

Right now, in this code, you basically have two "if (is_win)" statements. I guess that is where you were thinking of "if (is_win or virtual_cpu_count < 10)".
But I'm just investigating and experimenting now. It looks like it's possible to make Chrome build in a tenth of the time this way, counted in CPU cycles, which makes it a tempting to keep investigating.

Comment 23 by peria@chromium.org, Mar 27 2017

For goma users, their virtual CPUs can be < 10, because goma does not provide internal virtual CPUs, and whether goma works or not can change after GN works.

How about adding a new GN flag, if it is acceptable for you?
Of cause in that case, you need an action to specify the flag to switch to Windows style
compile. (no automated routine works to switch)

What do you think?

Comment 24 by brat...@opera.com, Mar 28 2017

I think a global flag is one option that is worth pursuing but I'm looking at more code than just this so maybe hold off until there are some kind of agreement of how to configure it?

One option is to build it into the gn binary itself, so that it will group files if you ask it to do so. May be a bit too magic when everything happens automatically. Listing it by hand is much less magic but requires changes in many more places.

Project Member

Comment 25 by bugdroid1@chromium.org, Dec 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6bdfe3b98a5c84fb9cdd1eeccccdce05f16caba8

commit 6bdfe3b98a5c84fb9cdd1eeccccdce05f16caba8
Author: Takuto Ikuta <tikuta@chromium.org>
Date: Thu Dec 21 02:27:26 2017

Use V8GeneratedCoreBindings only in jumbo build

This is retry of
https://codereview.chromium.org/2318933002/

Current chromium/src can build all with is_official_build=true in windows.
This improves build parallelism on windows.

Bug:  634231 
Change-Id: Ie16541b56d90718ab01a992b7f4cbc07caebcd22
Reviewed-on: https://chromium-review.googlesource.com/836627
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Takuto Ikuta <tikuta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525565}
[modify] https://crrev.com/6bdfe3b98a5c84fb9cdd1eeccccdce05f16caba8/third_party/WebKit/Source/bindings/core/v8/BUILD.gn

Comment 26 by brat...@opera.com, Dec 21 2017

> This improves build parallelism on windows.

Did you check if it was also faster? Last I checked it was 22 times more work compiling the files one by one so even if higher parallelism keeps many cores active, it might still be slower.

Comment 27 by brat...@opera.com, Dec 21 2017

Cc: tikuta@chromium.org
+tikuta so that they can see my question.
In my 48 thread windows 10 machine, my CL improved build time of
third_party/WebKit/Source/bindings/core/v8:bindings_core_impl
from 330 seconds to 171 seconds when I use goma to specify -j1000.

Compiling V8GeneratedCoreBindings.cpp takes more than 200 seconds in my build.
Small task with high parallelism is very important in goma build.

Comment 29 by brat...@opera.com, Dec 21 2017

Indeed. 

Maybe it should be controlled by whether you have goma or not. Does it make things faster without goma as well (if you have a 48 thread machine).
I measured the build time on the same machine, and it increased from 848 seconds to 1023 seconds.

But if you want to build faster, you can do it by enabling jumbo build yet.
And I prefer jumbo config controls such a thing.

Project Member

Comment 31 by bugdroid1@chromium.org, Dec 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42a666ceeee18625040f708ae3084f1e43a2f6e9

commit 42a666ceeee18625040f708ae3084f1e43a2f6e9
Author: Takuto Ikuta <tikuta@google.com>
Date: Fri Dec 22 10:05:16 2017

Revert "Use V8GeneratedCoreBindings only in jumbo build"

This reverts commit 6bdfe3b98a5c84fb9cdd1eeccccdce05f16caba8.

Reason for revert: Broke official win asan build crbug.com/797202
FAILED: obj/third_party/WebKit/Source/bindings/core/v8/bindings_core_impl.lib 
C:/b/depot_tools/win_tools-2_7_6_bin/python/bin/python.exe ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x86 False lib.exe /nologo /ignore:4221 /OUT:obj/third_party/WebKit/Source/bindings/core/v8/bindings_core_impl.lib @obj/third_party/WebKit/Source/bindings/core/v8/bindings_core_impl.lib.rsp
LINK : MSIL .netmodule or module compiled with /GL found; restarting link with /LTCG; add /LTCG to the link command line to improve linker performance
obj/third_party/WebKit/Source/bindings/core/v8/bindings_core_impl.lib : fatal error LNK1248: image size (1001D34E4) exceeds maximum allowable size (FFFFFFFF)

Original change's description:
> Use V8GeneratedCoreBindings only in jumbo build
> 
> This is retry of
> https://codereview.chromium.org/2318933002/
> 
> Current chromium/src can build all with is_official_build=true in windows.
> This improves build parallelism on windows.
> 
> Bug:  634231 
> Change-Id: Ie16541b56d90718ab01a992b7f4cbc07caebcd22
> Reviewed-on: https://chromium-review.googlesource.com/836627
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
> Commit-Queue: Takuto Ikuta <tikuta@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525565}

TBR=peria@chromium.org,bashi@chromium.org,haraken@chromium.org,tikuta@google.com,tikuta@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  634231 , 797202
Change-Id: I86c3a554a5121ab94c7950a97de7b9914b0a7b95
Reviewed-on: https://chromium-review.googlesource.com/842564
Commit-Queue: Shinya Kawanaka <shinyak@chromium.org>
Reviewed-by: Shinya Kawanaka <shinyak@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525966}
[modify] https://crrev.com/42a666ceeee18625040f708ae3084f1e43a2f6e9/third_party/WebKit/Source/bindings/core/v8/BUILD.gn

Project Member

Comment 32 by bugdroid1@chromium.org, Dec 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dee25c1bd32025929f350ae4cb23039bbb2c4c11

commit dee25c1bd32025929f350ae4cb23039bbb2c4c11
Author: Takuto Ikuta <tikuta@chromium.org>
Date: Mon Dec 25 11:22:03 2017

Do not use V8GeneratedCoreBindings in non-offical win build

This is retry of
https://chromium-review.googlesource.com/836627

Note that following builder switched to clang-cl and /GL is not added now.
That made me thinking the reverted CL is safe to land.
https://ci.chromium.org/buildbot/chromium.chrome/Google%20Chrome%20Win/


To improve build speed when using goma, let me stop using amalgamation on non-official win build.
This CL won't effect builder causing revert of previous CL.

In my 48 thread windows 10 machine, my CL improved build time of
third_party/WebKit/Source/bindings/core/v8:bindings_core_impl
from 330 seconds to 171 seconds when I use goma to specify -j1000 without backend cache.

Bug:  634231 
Change-Id: I212c50d4d0d5d2a6399004be877665a9c95de838
Reviewed-on: https://chromium-review.googlesource.com/843996
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Takuto Ikuta <tikuta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526172}
[modify] https://crrev.com/dee25c1bd32025929f350ae4cb23039bbb2c4c11/third_party/WebKit/Source/bindings/core/v8/BUILD.gn

Project Member

Comment 33 by bugdroid1@chromium.org, Sep 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b6fe95874503156d33bf99b8df3e2abbff23554

commit 5b6fe95874503156d33bf99b8df3e2abbff23554
Author: David Michael Barr <david.barr@samsung.com>
Date: Mon Sep 17 08:58:24 2018

Do not use v8_generated_core_bindings in jumbo build

For low values of jumbo_file_merge_limit, this removes a "long pole".

The rationale is similar to
https://chromium-review.googlesource.com/843996

When building with use_jumbo_build=true and cc_wrapper="sccache"[1],
the unit containing v8_generated_core_bindings was refused by the cache.
It also was an outlier for compilation time, 2 times the next slowest
object and 3x the next jumbo unit (jumbo_file_merge_limit=14).

For linux builds of content shell with a hot cache, this improved build
times by about a minute, ~20% of wall time, due to the expensive miss.

[1] https://github.com/mozilla/sccache

Bug:  634231 
Change-Id: I07ee42fc442ee3372e54ae757466d7af0a5845e8
Reviewed-on: https://chromium-review.googlesource.com/1164145
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Daniel Bratell <bratell@opera.com>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591622}
[modify] https://crrev.com/5b6fe95874503156d33bf99b8df3e2abbff23554/third_party/blink/renderer/bindings/core/v8/BUILD.gn

Sign in to add a comment