Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(204)

Issue 1167523007: Precompile more in Blink in Windows for faster compilations (Closed)

Created:
5 years, 6 months ago by Daniel Bratell
Modified:
5 years, 5 months ago
CC:
blink-reviews, blink-reviews-wtf_chromium.org, Mikhail, haraken, tasak (please_use_google.com)
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Precompile more in Blink in Windows for faster compilations One reason Blink is slow to compile is that there is a lot of code included in every compilation unit since 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 (4 minutes instead of 19 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. BUG=495697 R=thakis@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198859

Patch Set 1 #

Patch Set 2 : Small fixes #

Patch Set 3 : More documentation #

Patch Set 4 : Fixing boiler plats and include orders. #

Patch Set 5 : Oilpan fixes and rebased #

Patch Set 6 : Right file. #

Patch Set 7 : Silence style checkers #

Total comments: 4

Patch Set 8 : Reuse Precompile.h from Precompile-core.h #

Patch Set 9 : Rebased to newer master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -9 lines) Patch
M Source/core/DEPS View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/core.gyp View 1 2 3 4 5 6 4 chunks +4 lines, -4 lines 0 comments Download
M Source/core/frame/Frame.h View 1 2 3 4 5 6 7 8 2 chunks +1 line, -1 line 0 comments Download
A Source/core/win/Precompile-core.h View 1 2 3 4 5 6 7 1 chunk +33 lines, -0 lines 0 comments Download
A Source/core/win/Precompile-core.cpp View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download
A Source/core/win/precompile-core.gypi View 1 2 3 4 5 6 1 chunk +29 lines, -0 lines 0 comments Download
M Source/core/xml/XPathGrammar.y View 2 chunks +3 lines, -3 lines 0 comments Download
M Source/modules/modules.gyp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M Tools/Scripts/webkitpy/style/checkers/cpp.py View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 47 (4 generated)
Daniel Bratell
Some more changes are needed I guess since precommit said: ** Presubmit ERRORS ** You ...
5 years, 6 months ago (2015-06-08 14:16:44 UTC) #1
scottmg
As you say, it's a bit ugly, but that's also a huge decrease in compile ...
5 years, 6 months ago (2015-06-08 16:48:36 UTC) #2
brucedawson
How is Precompile-core.h being include? Is there a force-include directive somewhere in the .gyp files? ...
5 years, 6 months ago (2015-06-08 17:12:34 UTC) #3
scottmg
On 2015/06/08 17:12:34, brucedawson wrote: > How is Precompile-core.h being include? Is there a force-include ...
5 years, 6 months ago (2015-06-08 17:23:46 UTC) #4
jam
I'm not an owner in Blink so I'll defer to owners on the mechanics of ...
5 years, 6 months ago (2015-06-08 17:32:58 UTC) #5
brucedawson
I'm definitely not a fan of /FI (hidden includes - yuck) but that ship has ...
5 years, 6 months ago (2015-06-08 20:37:03 UTC) #6
jam
btw out of curiosity, what is the clean build time of chrome on your machine ...
5 years, 6 months ago (2015-06-08 22:14:01 UTC) #7
Daniel Bratell
On 2015/06/08 22:14:01, jam wrote: > btw out of curiosity, what is the clean build ...
5 years, 6 months ago (2015-06-09 06:12:13 UTC) #8
Daniel Bratell
On 2015/06/09 06:12:13, Daniel Bratell wrote: > On 2015/06/08 22:14:01, jam wrote: > > btw ...
5 years, 6 months ago (2015-06-09 08:29:46 UTC) #9
Daniel Bratell
On 2015/06/08 20:37:03, brucedawson wrote: > I'm definitely not a fan of /FI (hidden includes ...
5 years, 6 months ago (2015-06-09 11:28:05 UTC) #10
Daniel Bratell
On 2015/06/08 16:48:36, scottmg wrote: ... > How did you choose those two particular headers ...
5 years, 6 months ago (2015-06-09 11:39:55 UTC) #11
jam
On 2015/06/09 08:29:46, Daniel Bratell wrote: > On 2015/06/09 06:12:13, Daniel Bratell wrote: > > ...
5 years, 6 months ago (2015-06-10 02:01:11 UTC) #12
Daniel Bratell
So I guess it's time to make this ready for landing. I removed the "WIP" ...
5 years, 6 months ago (2015-06-10 15:48:22 UTC) #13
sof
FTR, if I additionally use precompile-core.gypi in Source/web.gyp, a full Blink rebuild takes 9:40 instead ...
5 years, 6 months ago (2015-06-10 20:35:41 UTC) #15
Daniel Bratell
On 2015/06/10 20:35:41, sof wrote: > FTR, if I additionally use precompile-core.gypi in Source/web.gyp, a ...
5 years, 6 months ago (2015-06-11 06:28:55 UTC) #16
jam
friendly ping: I'm wondering who you're waiting to review this? this is such a nice ...
5 years, 6 months ago (2015-06-16 01:55:56 UTC) #17
sof
On 2015/06/16 01:55:56, jam wrote: > friendly ping: I'm wondering who you're waiting to review ...
5 years, 6 months ago (2015-06-16 05:36:27 UTC) #18
brucedawson
If /Zm is needed then set it a bit higher than the required level, to ...
5 years, 6 months ago (2015-06-16 18:30:10 UTC) #19
Daniel Bratell
On 2015/06/16 01:55:56, jam wrote: > friendly ping: I'm wondering who you're waiting to review ...
5 years, 6 months ago (2015-06-22 16:27:35 UTC) #20
sof
On 2015/06/22 16:27:35, Daniel Bratell wrote: > On 2015/06/16 01:55:56, jam wrote: > > friendly ...
5 years, 6 months ago (2015-06-22 16:31:39 UTC) #21
Nico
Two questions: 1. In a components build, don't there have to be two versions of ...
5 years, 6 months ago (2015-06-23 22:35:16 UTC) #22
haraken
+tasak
5 years, 6 months ago (2015-06-24 00:06:31 UTC) #24
tasak
On 2015/06/24 00:06:31, haraken wrote: > +tasak I will check msvc2013's behavior.
5 years, 6 months ago (2015-06-24 04:28:46 UTC) #25
Daniel Bratell
On 2015/06/23 22:35:16, Nico wrote: > Two questions: > > 1. In a components build, ...
5 years, 6 months ago (2015-06-24 08:00:09 UTC) #26
sof
On 2015/06/22 16:31:39, sof wrote: > On 2015/06/22 16:27:35, Daniel Bratell wrote: > > On ...
5 years, 6 months ago (2015-06-24 09:29:39 UTC) #27
tasak
On 2015/06/24 04:28:46, tasak wrote: > On 2015/06/24 00:06:31, haraken wrote: > > +tasak > ...
5 years, 6 months ago (2015-06-24 09:59:58 UTC) #28
sof
looks fine from here, fwiw. (with https://codereview.chromium.org/1178823003 , it would look even finer, but might ...
5 years, 6 months ago (2015-06-24 14:21:41 UTC) #29
Daniel Bratell
On 2015/06/24 14:21:41, sof wrote: > looks fine from here, fwiw. > > (with https://codereview.chromium.org/1178823003 ...
5 years, 6 months ago (2015-06-25 11:23:51 UTC) #30
Nico
One more question. https://codereview.chromium.org/1167523007/diff/120001/Source/core/frame/Frame.h File Source/core/frame/Frame.h (right): https://codereview.chromium.org/1167523007/diff/120001/Source/core/frame/Frame.h#newcode32 Source/core/frame/Frame.h:32: #include "core/frame/FrameHost.h" Why this? https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precompile-core.h File ...
5 years, 6 months ago (2015-06-25 23:14:45 UTC) #31
sof
https://codereview.chromium.org/1167523007/diff/120001/Source/core/frame/Frame.h File Source/core/frame/Frame.h (right): https://codereview.chromium.org/1167523007/diff/120001/Source/core/frame/Frame.h#newcode32 Source/core/frame/Frame.h:32: #include "core/frame/FrameHost.h" On 2015/06/25 23:14:44, Nico wrote: > Why ...
5 years, 6 months ago (2015-06-26 06:23:03 UTC) #32
Daniel Bratell
On 2015/06/25 23:14:45, Nico wrote: > Source/core/frame/Frame.h:32: #include "core/frame/FrameHost.h" > Why this? I hope sof ...
5 years, 6 months ago (2015-06-26 10:54:45 UTC) #33
Nico
On 2015/06/26 10:54:45, Daniel Bratell wrote: > On 2015/06/25 23:14:45, Nico wrote: > > Source/core/frame/Frame.h:32: ...
5 years, 5 months ago (2015-06-29 14:17:06 UTC) #34
Daniel Bratell
> https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precompile-core.h#newcode51 > > > Source/core/win/Precompile-core.h:51: #include "config.h" > > > This looks wrong. > ...
5 years, 5 months ago (2015-06-29 14:55:22 UTC) #35
sof
what's holding this one up?
5 years, 5 months ago (2015-07-12 07:35:09 UTC) #36
Daniel Bratell
On 2015/07/12 07:35:09, sof wrote: > what's holding this one up? thakis asked a few ...
5 years, 5 months ago (2015-07-13 07:27:32 UTC) #37
Nico
lgtm, thanks for explaining. I have one more question I'd like to understand before you ...
5 years, 5 months ago (2015-07-13 23:27:49 UTC) #38
Daniel Bratell
On 2015/07/13 23:27:49, Nico wrote: > lgtm, thanks for explaining. > > I have one ...
5 years, 5 months ago (2015-07-14 12:51:36 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167523007/160001
5 years, 5 months ago (2015-07-14 12:51:54 UTC) #42
commit-bot: I haz the power
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198859
5 years, 5 months ago (2015-07-14 12:55:19 UTC) #43
Daniel Bratell
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1234393002/ by bratell@opera.com. ...
5 years, 5 months ago (2015-07-16 15:23:16 UTC) #44
Nico
On 2015/07/16 15:23:16, Daniel Bratell wrote: > A revert of this CL (patchset #9 id:160001) ...
5 years, 5 months ago (2015-07-16 20:53:29 UTC) #45
sof
On 2015/07/16 20:53:29, Nico wrote: > On 2015/07/16 15:23:16, Daniel Bratell wrote: > > A ...
5 years, 5 months ago (2015-07-16 21:02:40 UTC) #46
Nico
5 years, 5 months ago (2015-07-16 21:04:23 UTC) #47
Message was sent while issue was closed.
Ah, nevermind me then. Sorry.

On Thu, Jul 16, 2015 at 2:02 PM, <sigbjornf@opera.com> wrote:

> On 2015/07/16 20:53:29, Nico wrote:
>
>> On 2015/07/16 15:23:16, Daniel Bratell wrote:
>> > A revert of this CL (patchset #9 id:160001) has been created in
>> > https://codereview.chromium.org/1234393002/ by mailto:bratell@opera.com
>> .
>> >
>> > The reason for reverting is: Strange errors in
>> > http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Builder
>> indicating
>> > that old versions of headers are used by the compiler. Possibly a stale
>>
> cached
>
>> > pch file..
>>
>
>  That might be due to the issue bauerb mentioned on chromium-dev and not
>> due to
>> this change. That bot also didn't cycle green after the revert, as far as
>> I
>>
> can
>
>> see.
>>
>
> Looks like it will cycle green on
>
>
>
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Builder/builds/4...
>
> which is based on r199034. i.e., something that doesn't have the revert (@
> r199036.)
>
> https://codereview.chromium.org/1167523007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698