Use precompiled headers for most large projects.

Use precompiled headers for most large projects where the .gyp file
is not a third party file.

On my machine, this speeds up a full recompile of the 'chrome' target
in Debug mode by about 18%.

BUG=none
TEST=it builds, existing tests pass

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99949

[jeanluc1, 2011-08-22 22:20:24]

http://codereview.chromium.org/7706011/diff/1/build/precompile.h
File build/precompile.h (right):

http://codereview.chromium.org/7706011/diff/1/build/precompile.h#newcode3
build/precompile.h:3: // Need to figure out if/how this can be included in the
PCH in general.
This should be a TODO(joi)

http://codereview.chromium.org/7706011/diff/1/build/precompile.h#newcode9
build/precompile.h:9: #include <atlctrls.h>  // 2
What do the comments mean?

http://codereview.chromium.org/7706011/diff/1/build/precompile.h#newcode15
build/precompile.h:15: #include <Objbase.h>  // 2
This is not in alphabetical order.

http://codereview.chromium.org/7706011/diff/1/build/precompile.h#newcode56
build/precompile.h:56: //#include <shlobj.h>
Why commented out (here and below)?

http://codereview.chromium.org/7706011/diff/1/content/gpu/gpu_dx_diagnostics_...
File content/gpu/gpu_dx_diagnostics_win.cc (right):

http://codereview.chromium.org/7706011/diff/1/content/gpu/gpu_dx_diagnostics_...
content/gpu/gpu_dx_diagnostics_win.cc:16: #pragma comment(lib, "dxguid.lib")
I would comment what this line does.

http://codereview.chromium.org/7706011/diff/1/ppapi/proxy/ppapi_proxy_test.cc
File ppapi/proxy/ppapi_proxy_test.cc (right):

http://codereview.chromium.org/7706011/diff/1/ppapi/proxy/ppapi_proxy_test.cc...
ppapi/proxy/ppapi_proxy_test.cc:66: const void* iface =
observer->GetInterface(name);
I find the abbreviation 'iface' cryptic.  I would suggest
'mock_interface'.

http://codereview.chromium.org/7706011/diff/1/ppapi/proxy/ppapi_proxy_test.cc...
ppapi/proxy/ppapi_proxy_test.cc:109: const void* iface) {
I find the abbreviation 'iface' cryptic.  I would suggest
'test_interface'.

You may want to check 'iface' in header files too.

http://codereview.chromium.org/7706011/diff/1/webkit/plugins/ppapi/url_reques...
File webkit/plugins/ppapi/url_request_info_unittest.cc (right):

http://codereview.chromium.org/7706011/diff/1/webkit/plugins/ppapi/url_reques...
webkit/plugins/ppapi/url_request_info_unittest.cc:105: const PPB_URLRequestInfo*
iface =
We're supposed to avoid abbeviation and 'iface' is cryptic.  I would suggest
'request_info'.

[jam, 2011-08-22 22:57:09]

can't wait for this to land :) thanks for looking into this

On Mon, Aug 22, 2011 at 10:45 AM, <joi@chromium.org> wrote:

> Reviewers: jeanluc1,
>
> Description:
> Enable use of precompiled headers by default for all projects. For now
> this is via an off-by-default include.gypi flag, and a handful of
> projects disable use of precompiled headers to avoid build
> issues. Later on, given a bit of time, most such projects can probably
> be switched to use precompiled headers, and we can turn them on by
> default.
>
> On my machine, this speeds up a full recompile of the 'chrome' target
> in Debug mode by about 27%.
>
> This depends on pending GYP change
>
http://codereview.chromium.**org/7710008/<http://codereview.chromium.org/7710...
has so far only been
> tested to work for all targets in Debug mode, 'component' build.
>
> BUG=none
> TEST=it builds, existing tests pass
>
>
> Please review this at
http://codereview.chromium.**org/7706011/<http://codereview.chromium.org/7706...
>
> SVN Base:
ssh://joi@192.168.1.201/home/**joi/c/chrome/src@master<http://joi@192.168.1.201/home/joi/c/chrome/src@master>
>
> Affected files:
>  M build/common.gypi
>  A build/precompile.h
>  A build/precompile.cc
>  M chrome/chrome.gyp
>  M chrome/chrome_dll.gypi
>  M content/content_ppapi_plugin.**gypi
>  M content/gpu/gpu_dx_**diagnostics_win.cc
>  M media/media.gyp
>  M ppapi/ppapi_proxy.gypi
>  M ppapi/proxy/host_dispatcher.h
>  M ppapi/proxy/plugin_dispatcher.**h
>  M ppapi/proxy/ppapi_proxy_test.h
>  M ppapi/proxy/ppapi_proxy_test.**cc
>  M ppapi/tests/test_post_message.**cc
>  M third_party/cacheinvalidation/**cacheinvalidation.gyp
>  M third_party/mesa/mesa.gyp
>  M third_party/protobuf/protobuf.**gyp
>  M third_party/sfntly/sfntly.gyp
>  M ui/ui.gyp
>  M views/views.gyp
>  M webkit/glue/webkit_glue.gypi
>  M webkit/plugins/ppapi/url_**request_info_unittest.cc
>
>
>

[jeanluc1, 2011-08-23 14:47:27]

LGTM

http://codereview.chromium.org/7706011/diff/4002/build/precompile.h
File build/precompile.h (right):

http://codereview.chromium.org/7706011/diff/4002/build/precompile.h#newcode11
build/precompile.h:11: // included under src/chrome/browser, which was used as a
baseline for
I would specify either a time or a version number, so that later reader can
guess how fresh these counts are.

[Jói, 2011-08-24 13:56:32]

I think this is ready for commit once the trybots pass.  It won't work for
anybody unless they patch in the GYP change as well, but it does nothing unless
the include.gypi flag is specified.

Cheers,
Jói

http://codereview.chromium.org/7706011/diff/1/build/precompile.h
File build/precompile.h (right):

http://codereview.chromium.org/7706011/diff/1/build/precompile.h#newcode3
build/precompile.h:3: // Need to figure out if/how this can be included in the
PCH in general.
On 2011/08/22 22:20:24, jeanluc1 wrote:
> This should be a TODO(joi)

Sorry, forgot to clean this up before uploading.  Have cleaned up the file now,
added copyright header, sorted things and so forth.

http://codereview.chromium.org/7706011/diff/1/build/precompile.h#newcode9
build/precompile.h:9: #include <atlctrls.h>  // 2
On 2011/08/22 22:20:24, jeanluc1 wrote:
> What do the comments mean?

Added a comment to explain:

// Numeric comments beside includes are the number of times they are
// included under src/chrome/browser, which was used as a baseline for
// deciding what to include in the PCH. It may be possible to tweak the
// speed of the build by commenting out or removing some of the less
// frequently used headers.

http://codereview.chromium.org/7706011/diff/1/build/precompile.h#newcode15
build/precompile.h:15: #include <Objbase.h>  // 2
On 2011/08/22 22:20:24, jeanluc1 wrote:
> This is not in alphabetical order.

I've sorted into three sections to the extent possible, first Windows-specific,
then standard library, then C++ standard library.

http://codereview.chromium.org/7706011/diff/1/build/precompile.h#newcode56
build/precompile.h:56: //#include <shlobj.h>
On 2011/08/22 22:20:24, jeanluc1 wrote:
> Why commented out (here and below)?

Added explanation; it caused conflicts that possibly could be resolved, and if
so would be good to bring back.

http://codereview.chromium.org/7706011/diff/1/content/gpu/gpu_dx_diagnostics_...
File content/gpu/gpu_dx_diagnostics_win.cc (right):

http://codereview.chromium.org/7706011/diff/1/content/gpu/gpu_dx_diagnostics_...
content/gpu/gpu_dx_diagnostics_win.cc:16: #pragma comment(lib, "dxguid.lib")
On 2011/08/22 22:20:24, jeanluc1 wrote:
> I would comment what this line does.

Done.

http://codereview.chromium.org/7706011/diff/1/ppapi/proxy/ppapi_proxy_test.cc
File ppapi/proxy/ppapi_proxy_test.cc (right):

http://codereview.chromium.org/7706011/diff/1/ppapi/proxy/ppapi_proxy_test.cc...
ppapi/proxy/ppapi_proxy_test.cc:66: const void* iface =
observer->GetInterface(name);
On 2011/08/22 22:20:24, jeanluc1 wrote:
> I find the abbreviation 'iface' cryptic.  I would suggest
> 'mock_interface'.

Done.

http://codereview.chromium.org/7706011/diff/1/ppapi/proxy/ppapi_proxy_test.cc...
ppapi/proxy/ppapi_proxy_test.cc:109: const void* iface) {
On 2011/08/22 22:20:24, jeanluc1 wrote:
> I find the abbreviation 'iface' cryptic.  I would suggest
> 'test_interface'.
> 
> You may want to check 'iface' in header files too.

Done, changed header file too.

http://codereview.chromium.org/7706011/diff/1/webkit/plugins/ppapi/url_reques...
File webkit/plugins/ppapi/url_request_info_unittest.cc (right):

http://codereview.chromium.org/7706011/diff/1/webkit/plugins/ppapi/url_reques...
webkit/plugins/ppapi/url_request_info_unittest.cc:105: const PPB_URLRequestInfo*
iface =
On 2011/08/22 22:20:24, jeanluc1 wrote:
> We're supposed to avoid abbeviation and 'iface' is cryptic.  I would suggest
> 'request_info'.

Done.

http://codereview.chromium.org/7706011/diff/4002/build/precompile.h
File build/precompile.h (right):

http://codereview.chromium.org/7706011/diff/4002/build/precompile.h#newcode11
build/precompile.h:11: // included under src/chrome/browser, which was used as a
baseline for
On 2011/08/23 14:47:27, jeanluc1 wrote:
> I would specify either a time or a version number, so that later reader can
> guess how fresh these counts are.

Done.

[jeanluc1, 2011-08-24 17:47:41]

LGTM

[Jói, 2011-09-05 15:15:43]

Update on this change, which I didn't manage to check in before I went
on vacation for a week:  After updating a couple of times, I'm
starting to think that the approach of using precompiled headers for
all projects except those that opt out is a bit unstable (I've seen a
couple of different projects break on different updates).

I'm going to try the approach of just explicitly using precompiled
headers for some of the biggest projects, and see if that helps bring
down the build times by a similar amount.  If so, I will do that
instead and abandon this change.  Will post an update here.

Cheers,
Jói



On Wed, Aug 24, 2011 at 5:47 PM,  <jeanluc@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/7706011/
>

[Jói, 2011-09-06 13:29:08]

Please take another look, I've switched to explicitly enabling precompiled
headers.

[jeanluc1, 2011-09-06 19:32:33]

http://codereview.chromium.org/7706011/diff/27001/content/gpu/gpu_dx_diagnost...
File content/gpu/gpu_dx_diagnostics_win.cc (right):

http://codereview.chromium.org/7706011/diff/27001/content/gpu/gpu_dx_diagnost...
content/gpu/gpu_dx_diagnostics_win.cc:17: #pragma comment(lib, "dxguid.lib")
Sorry to revisit this, but why a pragma rather than specifying the lib at link
time?  That's more where people would look for it.

[Jói, 2011-09-06 21:45:55]

Actually I think the #pragma is the way we generally link to DLLs, see
http://codesearch.google.com/#search/&exact_package=chromium&q=%23pragma.comm...

Cheers,
Jói


On Tue, Sep 6, 2011 at 7:32 PM,  <jeanluc@chromium.org> wrote:
>
>
http://codereview.chromium.org/7706011/diff/27001/content/gpu/gpu_dx_diagnost...
> File content/gpu/gpu_dx_diagnostics_win.cc (right):
>
>
http://codereview.chromium.org/7706011/diff/27001/content/gpu/gpu_dx_diagnost...
> content/gpu/gpu_dx_diagnostics_win.cc:17: #pragma comment(lib,
> "dxguid.lib")
> Sorry to revisit this, but why a pragma rather than specifying the lib
> at link time?  That's more where people would look for it.
>
> http://codereview.chromium.org/7706011/
>

[jeanluc1, 2011-09-06 22:01:44]

LGTM!

[Jói, 2011-09-07 09:22:33]

I added the DEPS roll of GYP to r1033 (the GYP-side change this
depends on) to this change, and am running try jobs again.

Jean-Luc and the rest of the folks on gyp-developer, before I commit I
wanted to ask, are you aware of any reason that try jobs won't surface
for why GYP should not be rolled from r1025 to r1033?

Cheers,
Jói


On Tue, Sep 6, 2011 at 10:01 PM,  <jeanluc@chromium.org> wrote:
> LGTM!
>
> http://codereview.chromium.org/7706011/
>

[Jói, 2011-09-07 11:46:38]

Looks like the delta is only from r1031 to r1033; r1032 is a bug fix
that seems very safe, r1033 is my change.  Therefore I will commit
once trybots are green, better to do it while the tree is super quiet.

Cheers,
Jói


2011/9/7 Jói Sigurðsson <joi@chromium.org>:
> I added the DEPS roll of GYP to r1033 (the GYP-side change this
> depends on) to this change, and am running try jobs again.
>
> Jean-Luc and the rest of the folks on gyp-developer, before I commit I
> wanted to ask, are you aware of any reason that try jobs won't surface
> for why GYP should not be rolled from r1025 to r1033?
>
> Cheers,
> Jói
>
>
> On Tue, Sep 6, 2011 at 10:01 PM,  <jeanluc@chromium.org> wrote:
>> LGTM!
>>
>> http://codereview.chromium.org/7706011/
>>
>

[jeanluc, 2011-09-07 15:33:29]

Don't know of any reasons.

It's nice that the tree is super quiet; an advantage of working in a
different time zone I'm sure.

Jean-Luc

On Wed, Sep 7, 2011 at 4:46 AM, Jói Sigurðsson <joi@chromium.org> wrote:
> Looks like the delta is only from r1031 to r1033; r1032 is a bug fix
> that seems very safe, r1033 is my change.  Therefore I will commit
> once trybots are green, better to do it while the tree is super quiet.
>
> Cheers,
> Jói
>
>
> 2011/9/7 Jói Sigurðsson <joi@chromium.org>:
>> I added the DEPS roll of GYP to r1033 (the GYP-side change this
>> depends on) to this change, and am running try jobs again.
>>
>> Jean-Luc and the rest of the folks on gyp-developer, before I commit I
>> wanted to ask, are you aware of any reason that try jobs won't surface
>> for why GYP should not be rolled from r1025 to r1033?
>>
>> Cheers,
>> Jói
>>
>>
>> On Tue, Sep 6, 2011 at 10:01 PM,  <jeanluc@chromium.org> wrote:
>>> LGTM!
>>>
>>> http://codereview.chromium.org/7706011/
>>>
>>
>

[brettw, 2011-09-09 23:01:56]

Why did you rename interface in a bunch of places?

[brettw, 2011-09-09 23:18:19]

On 2011/09/09 23:01:56, brettw wrote:
> Why did you rename interface in a bunch of places?

And it looks like you ignored OWNERS for the PPAPI directory? Why?

[Jói, 2011-09-09 23:29:32]

One of the Windows headers defines it, unfortunately.

(sent from phone, pardon brevity)
On Sep 9, 2011 11:01 PM, <brettw@chromium.org> wrote:
> Why did you rename interface in a bunch of places?
>
> http://codereview.chromium.org/7706011/

[brettw, 2011-09-09 23:36:03]

I'm reverting this, you did not get proper reviews for any of this code.

[Jói, 2011-09-09 23:51:10]

Brett, instead of reverting, any chance you could review and LGTM for
ppapi OWNERS?

I thought the change was trivial enough to land without OWNERS
approval, just a couple of variable renames in the ppapi code that you
take issue with, and it was hard to land, so I wanted to land it
before it got outdated again.  I'm sorry if you feel I overstepped the
bounds around OWNERS, but my understanding was that these kind of
changes that touch a lot of code but are trivial apart from the main
part of the change (which was reviewed by jeanluc@) were OK to land
like this.

Cheers,
Jói


On Fri, Sep 9, 2011 at 11:36 PM,  <brettw@chromium.org> wrote:
> I'm reverting this, you did not get proper reviews for any of this code.
>
> http://codereview.chromium.org/7706011/
>