Precompile more in Blink in Windows for faster compilations

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

[Daniel Bratell, 2015-06-08 14:16:44]

Some more changes are needed I guess since precommit said:

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  Source\build\win\Precompile-core.h
      Illegal include: "core/dom/Document.h"
    Because of no rule applying. \
  Source\build\win\Precompile-core.h
      Illegal include: "core/layout/LayoutObject.h"
    Because of no rule applying.

check-webkit-style failed
  Source/build/win/Precompile-core.h:61:  Alphabetical sorting problem. 
[build/include_order] [4]
Source/build/win/Precompile-core.h:70:  Header file should not contain WebCore
config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 6 files


--------------------

I think it should move to inside core, though I'm not sure about the sorting
order. git cl format didn't change anything.

[scottmg, 2015-06-08 16:48:36]

As you say, it's a bit ugly, but that's also a huge decrease in compile time so
seems worthwhile to me.

How did you choose those two particular headers to add? An explanatory comment
and a suggestion as to how someone could update/optimize in the future would be
good.

[brucedawson, 2015-06-08 17:12:34]

How is Precompile-core.h being include? Is there a force-include directive
somewhere in the .gyp files?

If so, is this force-include happening on all platforms?

What is the build-time effect on other platforms? Will they also get PCH
benefits?

[scottmg, 2015-06-08 17:23:46]

On 2015/06/08 17:12:34, brucedawson wrote:
> How is Precompile-core.h being include? Is there a force-include directive
> somewhere in the .gyp files?
> 
> If so, is this force-include happening on all platforms?
> 
> What is the build-time effect on other platforms? Will they also get PCH
> benefits?

Only Windows, the msvs_precompiled_xxx gyp directives add /FI and the /Yc/Yu
switches. It won't affect other platforms either way except as Daniel noted by
diverging required #includes.

There is a separate gyp variable (I think it's GCC_PREFIX_HEADER?) but I'm not
sure how we use it, or if it needs to be in this CL.

[jam, 2015-06-08 17:32:58]

I'm not an owner in Blink so I'll defer to owners on the mechanics of doing
this, but these are great results. Thank you for working to improve this.

[brucedawson, 2015-06-08 20:37:03]

I'm definitely not a fan of /FI (hidden includes - yuck) but that
ship has sailed and the build-time benefits are clearly huge so this
is definitely the right direction to go in.

It could be worth exploring whether Linux and OSX would also benefit
from this PCH change. That would avoid the platform differences that
will otherwise occur.

[jam, 2015-06-08 22:14:01]

btw out of curiosity, what is the clean build time of chrome on your machine
(debug, component build, gyp)?
On a new Z840 (with various security software installed) I'm seeing 35 minutes.
With patching this change, I'm seeing about 32-33 minutes. I'm wondering why I'm
not seeing as big of a gain as you did?

[Daniel Bratell, 2015-06-09 06:12:13]

On 2015/06/08 22:14:01, jam wrote:
> btw out of curiosity, what is the clean build time of chrome on your machine
> (debug, component build, gyp)?
> On a new Z840 (with various security software installed) I'm seeing 35
minutes.
> With patching this change, I'm seeing about 32-33 minutes. I'm wondering why
I'm
> not seeing as big of a gain as you did?

I don't build chrome so I don't really know. Rather I build content_shell which
I believe is little short of an hour for a full rebuild (I will try to get exact
numbers later). The savings of 15 minutes would then be 25%, which is much more
than the ~8% you seem to get. Very different hardware though. I have a quadcore
4790@3.6GHz (8 logical cores) so if I were to guess your 24 core machine has a
lot of cores idling or stalled on RAM or something similar a lot of the time.
Too bad your gain was so much smaller. I will try to find someone else here to
verify my results to see that it wasn't just my machine that gets 4x faster
compiling webcore.

[Daniel Bratell, 2015-06-09 08:29:46]

On 2015/06/09 06:12:13, Daniel Bratell wrote:
> On 2015/06/08 22:14:01, jam wrote:
> > btw out of curiosity, what is the clean build time of chrome on your machine
> > (debug, component build, gyp)?
> > On a new Z840 (with various security software installed) I'm seeing 35
> minutes.
> > With patching this change, I'm seeing about 32-33 minutes. I'm wondering why
> I'm
> > not seeing as big of a gain as you did?
> 
> I don't build chrome so I don't really know. Rather I build content_shell
which
> I believe is little short of an hour for a full rebuild (I will try to get
exact
> numbers later). The savings of 15 minutes would then be 25%, which is much
more
> than the ~8% you seem to get. Very different hardware though. I have a
quadcore
> mailto:4790@3.6GHz (8 logical cores) so if I were to guess your 24 core
machine has a
> lot of cores idling or stalled on RAM or something similar a lot of the time.
> Too bad your gain was so much smaller. I will try to find someone else here to
> verify my results to see that it wasn't just my machine that gets 4x faster
> compiling webcore.

content_shell, debug, component build, no nacl, no patch: 60:23 
Same with patch: 41:55.

sof compiled blink_tests with and without the patch and got an improvement from
32:17 to 13:40. He will have hardware similar to what I have.

So my best guess is that a machine with 48 logical cores have different
bottlenecks than a machine with 8 logical core and to find really big wins it
will have to be analyzed on a Z840.

But I would be interested in what numbers you get with the patch and without the
patch when compiling only blink. Either with the target webcore (as I used
above) or the target blink_tests (as sof used).

[Daniel Bratell, 2015-06-09 11:28:05]

On 2015/06/08 20:37:03, brucedawson wrote:
> I'm definitely not a fan of /FI (hidden includes - yuck) but that
> ship has sailed and the build-time benefits are clearly huge so this
> is definitely the right direction to go in.
> 
> It could be worth exploring whether Linux and OSX would also benefit
> from this PCH change. That would avoid the platform differences that
> will otherwise occur.

It looks like gcc has something similar to force-include via the -include flag,
at least according to
https://gcc.gnu.org/onlinedocs/gcc/Precompiled-Headers.html . I have not used
precompiled headers much in Linux though. Their use has often been complicated
for some reason, while tools like ccache and icecc have solved many of the
problems anyway.

I would also prefer proper pch.h support via a normal header file but I would
not want to be the one that executed on that due to all the side tracks that
people in reviews would try to pull me onto. It would be the very definition of
a bikeshed review.

[Daniel Bratell, 2015-06-09 11:39:55]

On 2015/06/08 16:48:36, scottmg wrote:
... 
> How did you choose those two particular headers to add? An explanatory comment
> and a suggestion as to how someone could update/optimize in the future would
be
> good.

High level it's like I put it in the review message:
"everything depends on either LayoutObject.h or Document.h and those in turn
include huge portions of the rest of Blink."

The process to get there was that I know that the Document class is central to a
lot of operations so I tried that one first. That cut the time for webcore in
half. (19 to 9 minutes). I then thought of Element or Node but those are already
included via Document.h so no effect. I then asked around to see if someone knew
about another header that was huge and used everywhere and mstensho suggested
LayoutObject.h which took the time from 9 to 4 (great suggestion in other
words). I then tried to find some other header file in svg or style that were
large, but nothing I found made any difference.

I will add some kind of explanation to the Precompile-core.h file to help future
experiments.

[jam, 2015-06-10 02:01:11]

On 2015/06/09 08:29:46, Daniel Bratell wrote:
> On 2015/06/09 06:12:13, Daniel Bratell wrote:
> > On 2015/06/08 22:14:01, jam wrote:
> > > btw out of curiosity, what is the clean build time of chrome on your
machine
> > > (debug, component build, gyp)?
> > > On a new Z840 (with various security software installed) I'm seeing 35
> > minutes.
> > > With patching this change, I'm seeing about 32-33 minutes. I'm wondering
why
> > I'm
> > > not seeing as big of a gain as you did?
> > 
> > I don't build chrome so I don't really know. Rather I build content_shell
> which
> > I believe is little short of an hour for a full rebuild (I will try to get
> exact
> > numbers later). The savings of 15 minutes would then be 25%, which is much
> more
> > than the ~8% you seem to get. Very different hardware though. I have a
> quadcore
> > mailto:4790@3.6GHz (8 logical cores) so if I were to guess your 24 core
> machine has a
> > lot of cores idling or stalled on RAM or something similar a lot of the
time.
> > Too bad your gain was so much smaller. I will try to find someone else here
to
> > verify my results to see that it wasn't just my machine that gets 4x faster
> > compiling webcore.
> 
> content_shell, debug, component build, no nacl, no patch: 60:23 
> Same with patch: 41:55.
> 
> sof compiled blink_tests with and without the patch and got an improvement
from
> 32:17 to 13:40. He will have hardware similar to what I have.
> 
> So my best guess is that a machine with 48 logical cores have different
> bottlenecks than a machine with 8 logical core and to find really big wins it
> will have to be analyzed on a Z840.
> 
> But I would be interested in what numbers you get with the patch and without
the
> patch when compiling only blink. Either with the target webcore (as I used
> above) or the target blink_tests (as sof used).

Sorry for the delay, it took a while to do the tests (really slow to run this
stuff on windows!).

I built content_shell in debug component build. Without your patch, the average
of 5 runs is 23.1 minutes. With your patch, it's 18.9. So very nice speedup, and
similar percentages to what you're seeing. I guess in the overall chrome build,
the percentage is less just because this only affects blink. nevertheless, this
is still a very nice improvement.

[Daniel Bratell, 2015-06-10 15:48:22]

So I guess it's time to make this ready for landing. I removed the "WIP" label.

So who do you recommend to review this?

I do have one issue with the presubmit scripts:
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  Source\build\win\Precompile-core.h
      Illegal include: "core/dom/Document.h"
    Because of no rule applying. \
  Source\build\win\Precompile-core.h
      Illegal include: "core/layout/LayoutObject.h"
    Because of no rule applying.

check-webkit-style failed
  Source/build/win/Precompile-core.h:51:  Header file should not contain WebCore
config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 6 files

[sof, 2015-06-10 20:35:40]

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

[sof, 2015-06-10 20:35:41]

FTR, if I additionally use precompile-core.gypi in Source/web.gyp, a full Blink
rebuild takes 9:40 instead of 13:40 (w/ this CL) or ~33mins (w/ ToT.)

[Daniel Bratell, 2015-06-11 06:28:55]

On 2015/06/10 20:35:41, sof wrote:
> FTR, if I additionally use precompile-core.gypi in Source/web.gyp, a full
Blink
> rebuild takes 9:40 instead of 13:40 (w/ this CL) or ~33mins (w/ ToT.)

We should do that as well! But it required some hacks that need to be discussed
so I uploaded it separately to https://codereview.chromium.org/1178823003 . My
gain with that was slightly smaller (a further -20% rather than your -30%) but
still big.

[jam, 2015-06-16 01:55:56]

friendly ping: I'm wondering who you're waiting to review this? this is such a
nice improvement on Windows.

With some other local changes, I see ~20% improvements with this patch now to
build chrome.

[sof, 2015-06-16 05:36:27]

On 2015/06/16 01:55:56, jam wrote:
> friendly ping: I'm wondering who you're waiting to review this? this is such a
> nice improvement on Windows.
> 
> With some other local changes, I see ~20% improvements with this patch now to
> build chrome.

bratell's OOO all this week, but two things I ran into:

 - Had to compile Debug with /Zm113 to avoid running out of memory compiling
Precompile-core.cpp
 - Frame.h needed to #include "FrameHost.h" to get FrameHost into scope when
compiling Frame::trace().

Both of these was with enable_oilpan=1.

[brucedawson, 2015-06-16 18:30:10]

If /Zm is needed then set it a bit higher than the required level, to allow for
some future growth. Perhaps /Zm140 or /Zm150.

[Daniel Bratell, 2015-06-22 16:27:35]

On 2015/06/16 01:55:56, jam wrote:
> friendly ping: I'm wondering who you're waiting to review this? this is such a
> nice improvement on Windows.

As sof said, I was away last week, but I'm back now so now I am waiting for a
review. I need to see what to do about the issues sof mentioned too. I can not
reproduce them (and CQ seemed happy) but better to fix them before more than sof
are affected.

sof, did you add that flag in the gypi file?

[sof, 2015-06-22 16:31:39]

On 2015/06/22 16:27:35, Daniel Bratell wrote:
> On 2015/06/16 01:55:56, jam wrote:
> > friendly ping: I'm wondering who you're waiting to review this? this is such
a
> > nice improvement on Windows.
> 
> As sof said, I was away last week, but I'm back now so now I am waiting for a
> review. I need to see what to do about the issues sof mentioned too. I can not
> reproduce them (and CQ seemed happy) but better to fix them before more than
sof
> are affected.
> 
> sof, did you add that flag in the gypi file?

Perhaps that's possible; I merely extended my global overrides to include
win_debug_extra_cflags="-Zm113"

[Nico, 2015-06-23 22:35:16]

Two questions:

1. In a components build, don't there have to be two versions of the header, one
with dllexport, another with dllimport? (+haraken, tasak who are working on the
blink components build)

2. Since it's easy to do the same for OS X, should we do it there too for
consistency?

[haraken, 2015-06-24 00:06:30]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, tasak@google.com

[haraken, 2015-06-24 00:06:31]

+tasak

[tasak, 2015-06-24 04:28:46]

On 2015/06/24 00:06:31, haraken wrote:
> +tasak

I will check msvc2013's behavior.

[Daniel Bratell, 2015-06-24 08:00:09]

On 2015/06/23 22:35:16, Nico wrote:
> Two questions:
> 
> 1. In a components build, don't there have to be two versions of the header,
one
> with dllexport, another with dllimport? (+haraken, tasak who are working on
the
> blink components build)
> 
> 2. Since it's easy to do the same for OS X, should we do it there too for
> consistency?

thakis, I've been building component builds and the try servers are happy so
maybe all the code in these modules only use one setting anyway.

I think it would be worth experimenting with precompiled headers for all
platforms that have access to it, but right now I would like to land this so
that we have something to build on. Other platforms have access to ccache as
well, so they are not as bad off as Windows.

[sof, 2015-06-24 09:29:39]

On 2015/06/22 16:31:39, sof wrote:
> On 2015/06/22 16:27:35, Daniel Bratell wrote:
> > On 2015/06/16 01:55:56, jam wrote:
> > > friendly ping: I'm wondering who you're waiting to review this? this is
such
> a
> > > nice improvement on Windows.
> > 
> > As sof said, I was away last week, but I'm back now so now I am waiting for
a
> > review. I need to see what to do about the issues sof mentioned too. I can
not
> > reproduce them (and CQ seemed happy) but better to fix them before more than
> sof
> > are affected.
> > 
> > sof, did you add that flag in the gypi file?
> 
> Perhaps that's possible; I merely extended my global overrides to include
> win_debug_extra_cflags="-Zm113"

Re-checked -- this only reproduces if building with Oilpan enabled (reliably
so.)

Adding 

  'msvs_settings': {
    'VCCLCompilerTool': {
      'AdditionalOptions': [
        '-Zm120',
      ],
    },
  },

to target_defaults in precompile-core.gypi is what i ended up doing.

[tasak, 2015-06-24 09:59:58]

On 2015/06/24 04:28:46, tasak wrote:
> On 2015/06/24 00:06:31, haraken wrote:
> > +tasak
> 
> I will check msvc2013's behavior.

No errors occurred when enabling blink componentization flag.

[sof, 2015-06-24 14:21:41]

looks fine from here, fwiw. 

(with https://codereview.chromium.org/1178823003 , it would look even finer, but
might as well do that as a separate step.)

[Daniel Bratell, 2015-06-25 11:23:51]

On 2015/06/24 14:21:41, sof wrote:
> looks fine from here, fwiw. 
> 
> (with https://codereview.chromium.org/1178823003 , it would look even finer,
but
> might as well do that as a separate step.)

Yes, trying to do this in small steps so that it won't get stuck discussing
details.

I have now uploaded a version that has no presubmit warnings/errors. This moves
the files from build/win to core/win because files in build/win are not allowed
to include headers in core. I also added a special case to the style checker to
allow config.h includes in precompile header files. With that change I don't
know of anything that blocks this CL from landing so I will request some
reviews.

[Nico, 2015-06-25 23:14:45]

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/Fram...
Source/core/frame/Frame.h:32: #include "core/frame/FrameHost.h"
Why this?

https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precom...
File Source/core/win/Precompile-core.h (right):

https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precom...
Source/core/win/Precompile-core.h:51: #include "config.h"
This looks wrong.
https://www.webkit.org/coding/coding-style.html#include-config-h explicitly says
to not do this.

[sof, 2015-06-26 06:23:03]

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/Fram...
Source/core/frame/Frame.h:32: #include "core/frame/FrameHost.h"
On 2015/06/25 23:14:44, Nico wrote:
> Why this?

It should be FrameOwner.h

It is needed to address the following without it:

c:\src\blink\src\third_party\webkit\source\platform\heap\visitor.h(159) : error
C2338: T needs to be a garbage collected object
        c:\src\blink\src\third_party\webkit\source\platform\heap\visitor.h(169)
: see reference to function template instantiation 'void
blink::VisitorHelper<blink::Visitor>::mark<T>(T *)' being compiled
        with
        [
            T=blink::FrameOwner
        ]
        c:\src\blink\src\third_party\webkit\source\platform\heap\visitor.h(169)
: see reference to function template instantiation 'void
blink::VisitorHelper<blink::Visitor>::mark<T>(T *)' being compiled
        with
        [
            T=blink::FrameOwner
        ]
        c:\src\blink\src\third_party\webkit\source\core\frame\frame.cpp(81) :
see reference to function template instantiation 'void
blink::VisitorHelper<blink::Visitor>::trace<blink::FrameOwner>(const
blink::Member<blink::FrameOwner> &)' being compiled
        c:\src\blink\src\third_party\webkit\source\core\frame\frame.cpp(81) :
see reference to function template instantiation 'void
blink::VisitorHelper<blink::Visitor>::trace<blink::FrameOwner>(const
blink::Member<blink::FrameOwner> &)' being compiled
        c:\src\blink\src\third_party\webkit\source\core\frame\frame.cpp(77) :
see reference to function template instantiation 'void
blink::Frame::traceImpl<blink::Visitor*>(VisitorDispatcher)' being compiled
        with
        [
            VisitorDispatcher=blink::Visitor *
        ]

i.e., the full declaration of FrameOwner.h is not in scope when compiling the
trace() method for Frame (this is with MSVC2013)

Which makes little sense, as it is #include'd at the top of Frame.cpp.

We're not running into this on trunk with MSVC, so some unexplained pch "issue".
Bringing the declaration of FrameOwner into scope in Frame.h addresses this; I
don't know why.

It'd be good to leave behind a comment/TODO.

https://codereview.chromium.org/1167523007/diff/120001/Source/core/frame/Fram...
Source/core/frame/Frame.h:48: class FrameOwner;
nit: can be removed.

[Daniel Bratell, 2015-06-26 10:54:45]

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 answered this. For tracing in Oilpan and *hand waving*. I've uploaded
a new version that uses FrameOwner instead of FrameHost.

>
https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precom...
> File Source/core/win/Precompile-core.h (right):
> 
>
https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precom...
> Source/core/win/Precompile-core.h:51: #include "config.h"
> This looks wrong.
> https://www.webkit.org/coding/coding-style.html#include-config-h explicitly
says
> to not do this.

In this case Precompile.h can't be treated as a header file. It's not compiled
as a header file, it's compiled as a cpp file by the compiler, and then it needs
to have config.h because config.h declares certain base macros used by other
header files. Maybe it should be renamed so that it doesn't have the .h suffix?

[Nico, 2015-06-29 14:17:06]

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: #include "core/frame/FrameHost.h"
> > Why this?
> 
> I hope sof answered this. For tracing in Oilpan and *hand waving*. I've
uploaded
> a new version that uses FrameOwner instead of FrameHost.
> 
> >
>
https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precom...
> > File Source/core/win/Precompile-core.h (right):
> > 
> >
>
https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precom...
> > Source/core/win/Precompile-core.h:51: #include "config.h"
> > This looks wrong.
> > https://www.webkit.org/coding/coding-style.html#include-config-h explicitly
> says
> > to not do this.
> 
> In this case Precompile.h can't be treated as a header file. It's not compiled
> as a header file, it's compiled as a cpp file by the compiler, and then it
needs
> to have config.h because config.h declares certain base macros used by other
> header files. Maybe it should be renamed so that it doesn't have the .h
suffix?

What happens without this?

[Daniel Bratell, 2015-06-29 14:55:22]

>
https://codereview.chromium.org/1167523007/diff/120001/Source/core/win/Precom...
> > > Source/core/win/Precompile-core.h:51: #include "config.h"
> > > This looks wrong.
> > > https://www.webkit.org/coding/coding-style.html#include-config-h
explicitly
> > says
> > > to not do this.
> > 
> > In this case Precompile.h can't be treated as a header file. It's not
compiled
> > as a header file, it's compiled as a cpp file by the compiler, and then it
> needs
> > to have config.h because config.h declares certain base macros used by other
> > header files. Maybe it should be renamed so that it doesn't have the .h
> suffix?
> 
> What happens without this?

Without config.h you get compilation errors in header files that use the macros
defined in config.h. Those macros are the HAVE, USE, OS, ENABLE macros plus the
macros that you get from wrf/Compiler.h but those you can get anyway by
including wtf/Compiler.h so it is really HAVE, USE, OS and ENABLE that are the
problems.

This is the output if I remove config.h from Precompile-core.h:

FAILED: ninja -t msvc -e environment.x86 -- "C:\Program Files (x86)\Microsoft
Visual Studio 12.0\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC
@obj\third_party\webkit\source\core\win\webcore_svg.precompile-core.obj.rsp /c
..\..\third_party\WebKit\Source\core\win\Precompile-core.cpp
/Foobj\third_party\webkit\source\core\win\webcore_svg.precompile-core.obj
/Fdobj\third_party\WebKit\Source\core\webcore_svg.cc.pdb
l:\src\clean_chromium\src\third_party\webkit\source\wtf\assertions.h(162) :
error C2220: warning treated as error - no 'object' file generated
l:\src\clean_chromium\src\third_party\webkit\source\wtf\assertions.h(162) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
l:\src\clean_chromium\src\third_party\webkit\source\wtf\assertions.h(179) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
l:\src\clean_chromium\src\third_party\webkit\source\wtf\assertions.h(184) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
l:\src\clean_chromium\src\third_party\webkit\source\wtf\assertions.h(245) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
l:\src\clean_chromium\src\third_party\webkit\source\wtf\assertions.h(253) :
warning C4067: unexpected tokens following preprocessor directive - expected a
newline
...
l:\src\clean_chromium\src\third_party\webkit\source\wtf\addresssanitizer.h(29) :
fatal error C1012: unmatched parenthesis : missing ')'

In wtf/assertions.h:
Line 162 is a BACKTRACE_DISABLED macro defined to !ENABLE(ASSERT) so missing the
ENABLE macro.
Line 179 is OS(WIN) so missing the OS macro
Line 184 is ENABLE(ASSERT) so missing the ENABLE macro
...
In wtf/addresssanitizer.h line 29 it needs the OS macro.

Since there is currently no other way to get those macros than including
config.h, the only options I see is moving those macros to a different file
(config-macros.h?) or include config.h it in the precompiled "header".

[sof, 2015-07-12 07:35:09]

what's holding this one up?

[Daniel Bratell, 2015-07-13 07:27:32]

On 2015/07/12 07:35:09, sof wrote:
> what's holding this one up?

thakis asked a few questions I hope I've been able to answer, but not sure if
he's on vacation or something right now. If anybody else can cover for him, that
could help.

[Nico, 2015-07-13 23:27:49]

lgtm, thanks for explaining.

I have one more question I'd like to understand before you land this: Blink
files will only get this one precompiled header instead of this one and the one
from build/common.gypi, right? If so, please land.

(I'm a bit worried that this patch will break…something. But there's only one
way to find out, I suppose :-) )

[Daniel Bratell, 2015-07-14 12:51:36]

On 2015/07/13 23:27:49, Nico wrote:
> lgtm, thanks for explaining.
> 
> I have one more question I'd like to understand before you land this: Blink
> files will only get this one precompiled header instead of this one and the
one
> from build/common.gypi, right? If so, please land.
> 
> (I'm a bit worried that this patch will break…something. But there's only one
> way to find out, I suppose :-) )

It might cause surprises, and as several people have mentioned here, it's the
results that make it worthwhile. I will write a mail to blink-dev warning about
the unexpected inclusion of Document.h and LayoutObject.h.

If some *.gyp were to include more than one *precompile*.gypi, then the last one
would be the only one to be active and there would be no warnings. I'm putting
together a CL to trigger errors if that happens. Right now it won't. The
src/build/win_precompile.gypi one is used in content, net and a few other places
but not in blink.

So here we go...

[Daniel Bratell, 2015-07-14 12:51:43]

The CQ bit was checked by bratell@opera.com

[Daniel Bratell, 2015-07-14 12:51:43]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1167523007/#ps160001
(title: "Rebased to newer master")

[commit-bot: I haz the power, 2015-07-14 12:51:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1167523007/160001

[commit-bot: I haz the power, 2015-07-14 12:55:19]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198859

[Daniel Bratell, 2015-07-16 15:23:16]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1234393002/ by 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..

[Nico, 2015-07-16 20:53:29]

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.

[sof, 2015-07-16 21:02:40]

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.)

[Nico, 2015-07-16 21:04:23]

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.