Enable precompiled headers for Blink on Windows

Enable precompiled headers for Blink on Windows

One reason Blink is slow to compile is that there is a lot of code
included in every compilation unit. This is partly because everything
depends on either LayoutObject.h or Document.h and those in turn
include huge portions of the rest of Blink.

By precompiling LayoutObject.h and Document.h the compilation of core and
modules in Blink can be 4 times faster (5 minutes instead of
20 minutes on my computer).

The downside is that it will introduce Document.h and LayoutObject.h
also in compilation units that didn't expect it, for instance
XPathGrammer.y that suddenly will have both blink::Path and
blink::XPath::Path in scope (and blink::Filter / blink::XPath::Filter)

Note that distributed compilation system disables precompiled headers
globally so this will *not* make trybots faster.

This already landed[1] in the gyp/VS2013 world a year ago but some random
bot failures made us decide to revert it. Now with gn and VS2015 the world
should be a better place.

[1] https://codereview.chromium.org/1167523007/

BUG=495697

[Daniel Bratell, 2016-07-14 15:07:58]

The CQ bit was checked by bratell@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 15:08:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 16:47:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 16:47:55]

Dry run: This issue passed the CQ dry run.

[Daniel Bratell, 2016-07-14 18:20:07]

Description was changed from

==========
Enable precompiled headers for Blink on Windows

One reason Blink is slow to compile is that there is a lot of code
included in every compilation unit. This is partly because everything
depends on either LayoutObject.h or Document.h and those in turn
include huge portions of the rest of Blink.

By precompiling LayoutObject.h and Document.h the compilation of core and
modules in Blink can be 4 times faster (5 minutes instead of
20 minutes on my computer).

The downside is that it will introduce Document.h and LayoutObject.h
also in compilation units that didn't expect it, for instance
XPathGrammer.y that suddenly will have both blink::Path and
blink::XPath::Path in scope (and blink::Filter / blink::XPath::Filter)

Note that distributed compilation system disables precompiled headers
globally so this will *not* make trybots faster.

This already landed[1] in the gyp/VS2013 world a year ago but some random
bot failures made us decide to revert it. Now with gn and VS2015 the world
should be a better place.

[1] https://codereview.chromium.org/1167523007/

BUG=495697


And gn support.


Precompiled Headers for Blink updated to Feb 2016.

BUG=
==========

to

==========
Enable precompiled headers for Blink on Windows

One reason Blink is slow to compile is that there is a lot of code
included in every compilation unit. This is partly because everything
depends on either LayoutObject.h or Document.h and those in turn
include huge portions of the rest of Blink.

By precompiling LayoutObject.h and Document.h the compilation of core and
modules in Blink can be 4 times faster (5 minutes instead of
20 minutes on my computer).

The downside is that it will introduce Document.h and LayoutObject.h
also in compilation units that didn't expect it, for instance
XPathGrammer.y that suddenly will have both blink::Path and
blink::XPath::Path in scope (and blink::Filter / blink::XPath::Filter)

Note that distributed compilation system disables precompiled headers
globally so this will *not* make trybots faster.

This already landed[1] in the gyp/VS2013 world a year ago but some random
bot failures made us decide to revert it. Now with gn and VS2015 the world
should be a better place.

[1] https://codereview.chromium.org/1167523007/

BUG=495697
==========

[brucedawson, 2016-07-14 18:44:51]

PCH is good. Increasing dependencies is not. The tradeoff is that incremental
builds may now get slower because touching the force included header files (are
you using forced includes?) will force more files to recompile. I don't know how
significant this is but you should definitely measure and document the
incremental build times as well as the full build times.

Is this using forced include files, or explicitly included files?

If the former then presumably that means that the set of files included will
differ between PCH on and off, which is worrisome.
If the latter then that means that the try bots will build more slowly with this
change (when one of the header files is changed) because of the increased
dependencies.

I'm not saying we shouldn't do this - a four-times increase in build speeds is
worthwhile - but we do need to be aware of the tradeoffs. If there is any
possible way to get the benefits without the costs (only use PCH files on the
CPP files that already include those headers?) then that would be great.

But yeah, compile times for Blink are horrible.

[Daniel Bratell, 2016-07-14 19:53:53]

On 2016/07/14 18:44:51, brucedawson wrote:
> PCH is good. Increasing dependencies is not. The tradeoff is that incremental
> builds may now get slower because touching the force included header files
(are
> you using forced includes?) will force more files to recompile. I don't know
how
> significant this is but you should definitely measure and document the
> incremental build times as well as the full build times.
> 
> Is this using forced include files, or explicitly included files?
> 
> If the former then presumably that means that the set of files included will
> differ between PCH on and off, which is worrisome.
> If the latter then that means that the try bots will build more slowly with
this
> change (when one of the header files is changed) because of the increased
> dependencies.
> 
> I'm not saying we shouldn't do this - a four-times increase in build speeds is
> worthwhile - but we do need to be aware of the tradeoffs. If there is any
> possible way to get the benefits without the costs (only use PCH files on the
> CPP files that already include those headers?) then that would be great.
> 
> But yeah, compile times for Blink are horrible.

In gn it is explicitly using the /FI header. I would have liked an explicit file
better but that would have been very intrusive and considering config.h was just
eliminated I don't think anyone would be very keen.

For compilation performance this is a win for all scenarios. The two files
included in pch, Document.h and LayoutObject.h typically trigger 15-20 minute
compilations and now, despite more targets being compiled, it's 5 minutes.

Note that goma explicitly disables pch. I don't know why but it's how it has
always been. Maybe because it breaks goma or maybe goma is faster without pch.

The only downside is that you sometimes (most files include Document.h and
LayoutObject.h anyway) will have  more symbols in your files than you had
expected. This may cause collisions or may cause you to initially forget to add
an include that is needed for other platforms, but that is not really a new
problem for a Chromium developer.

[Daniel Bratell, 2016-07-14 19:55:14]

bratell@opera.com changed reviewers:
+ thakis@chromium.org

[Daniel Bratell, 2016-07-14 19:55:14]

thakis, can you please take a look? This is an identical change to the one you
reviewed last summer, but with gn support.

[Nico, 2016-07-14 19:56:13]

Given that this didn't work last summer, and the change is identical, why would
it work now? Because vs2015?

[Nico, 2016-07-14 19:58:52]

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/B...
File third_party/WebKit/Source/BUILD.gn (right):

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/B...
third_party/WebKit/Source/BUILD.gn:53: if (!is_official_build && !use_goma &&
!disable_precompiled_headers) {
Rather than repeat all these here, maybe they should influence the default for
disable_precompiled_headers?

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/win/precompile-core.gypi (right):

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/win/precompile-core.gypi:18: # compilation in
VS2013. Here we increase the limits
do we still need this?

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebKit.cpp (right):

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebKit.cpp:58: namespace helpers {
why is this needed?

[Daniel Bratell, 2016-07-14 20:18:57]

On 2016/07/14 19:56:13, Nico wrote:
> Given that this didn't work last summer, and the change is identical, why
would
> it work now? Because vs2015?

Since we never figured out exactly why that single bot broke it is hard to be
sure, but enough have changed to at least give it a shot. Top of my list of
suspects was broken output in the the dependency list from VS2013, but it could
have been anything, including a local hardware or software problem on the
failing machine. All we know was that it lost track of dependencies a couple of
days after pch was landed (twice).

We have been running with this at Opera since last year and I have not seen or
heard of any problems, but it is possible that if someone encountered a problem,
they would just force a bigger recompile and the problem would be gone and never
telling anyone about it. Our build machines have been fine though.

I will address/respond to your code comments tomorrow. They seem to be good
questions, and thanks for the very fast review!

[Daniel Bratell, 2016-07-15 06:43:56]

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/B...
File third_party/WebKit/Source/BUILD.gn (right):

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/B...
third_party/WebKit/Source/BUILD.gn:53: if (!is_official_build && !use_goma &&
!disable_precompiled_headers) {
On 2016/07/14 19:58:52, Nico wrote:
> Rather than repeat all these here, maybe they should influence the default for
> disable_precompiled_headers?

I've done that change in the next version but there are pros and cons of both
approaches. For instance, moving it to declare_args makes it possible to
override the value also when it should be true. But then again, if someone wants
to combine pch and official builds, let them.

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/win/precompile-core.gypi (right):

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/win/precompile-core.gypi:18: # compilation in
VS2013. Here we increase the limits
On 2016/07/14 19:58:52, Nico wrote:
> do we still need this?

I don't think so. This survived in the CL while it was being removed in the tree
so I'm removing it in the CL as well.

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebKit.cpp (right):

https://codereview.chromium.org/2152783002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebKit.cpp:58: namespace helpers {
On 2016/07/14 19:58:52, Nico wrote:
> why is this needed?

Doesn't seem to be needed. It used to be needed because there was a
MainThreadTaskRunner class here that collided (namewise) with other
MainThreadTaskRunner classes but that is gone. Also, this patch doesn't enable
pch for web.

[Daniel Bratell, 2016-07-15 07:39:42]

The CQ bit was checked by bratell@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 07:39:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 09:16:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-15 09:16:01]

Dry run: This issue passed the CQ dry run.

[Daniel Bratell, 2016-07-15 11:09:10]

The CQ bit was checked by bratell@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 11:09:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Bratell, 2016-07-15 14:35:30]

thakis, new version uploaded. Do you think this is better?

[sof, 2016-11-21 10:04:52]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-11-21 10:04:52]

At Daniel's request, taking over this one at
https://codereview.chromium.org/2520863002/

[Daniel Bratell, 2016-11-21 10:49:19]

On 2016/11/21 10:04:52, sof wrote:
> At Daniel's request, taking over this one at
> https://codereview.chromium.org/2520863002/

Yay! 

I'm closing this.