Add PGO targets to Chrome.

Add PGO targets to Chrome.

This allows to do a PGO optimized build of Chrome with the following commands:
python build\gyp_chromium -Dchrome_pgo_phase=1
ninja -C out\Release chrome
run with '--no-sandbox' + pgosweep
python build\gyp_chromium -Dchrome_pgo_phase=2
ninja -C out\Release chrome


----- Approach that didn't worked -----

(see comment #13 to know why this didn't work)

Instead of having the following gyp dependency chain (for a regular build), here '->' means 'depends on': 

'chrome'->|'chrome_main_dll'
----------|'chrome_child_dll'

we know have the following one (when chrome_pgo==1):
'chrome'->|'chrome_main_dll' |->'chrome_pgo_optimize'->|'chrome_main_dll_pgi'
----------|'chrome_child_dll'|-------------------------|'chrome_child_dll_pgi'


The '*_pgi' target correspond to the instrumented dll, the name of the DLLs produced by target will be terminated by '_pgi'.

During the 'chrome_pgo_optimize' step the instrumented DLLs will be renamed (the '_pgi' suffix will be removed) and Chrome will be run against a set of profile test cases, pgosweep.exe will also be invoked to collect the instrumentation data.

Then the main targets ('chrome_main_dll' and 'chrome_child_dll') will be linked with the flag /LTCG:PGOPTIMIZE, this will produce the optimized DLLs.
--------------------------------------------

R=scottmg@chromium.org

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257133

[Sébastien Marchand, 2014-03-06 15:50:34]

Hey Scott, can you please take a look at this approach ? This doesn't work but
I'm wondering if I'm at least in the right direction, being able to generate a
PGO build with the command:
ninja -C out\Release chrome_pgo_optimize
would be awesome :)

In the meantime I'll start fixing the things that block the PGO build (e.g.
ffmpeg).

[scottmg, 2014-03-06 17:16:35]

+yukawa

[scottmg, 2014-03-06 18:39:28]

I think this mostly looks as expected, but I'm not sure it'll work as well
as/the same as the test in gyp.

I think in the test it works all the code is in that static_library that's
linked into the final binary targets, which makes the linker settings affect all
that code also.

In this CL chrome_pgo_main depends on chrome_main_dll but the dll already has
linked. So, I think it'd have to be restructured to make that a static_library
instead which is might be a bit more more invasive than a nice standalone
include.

+yukawa who wrote this in gyp, and I think has used this on another binary and
might have suggestions.

If that restructuring gets too messy, it might be worthwhile to look at making
it easier for chrome by doing something different in gyp? Maybe something that
creates additional targets for a given target would work? i.e. chrome_main_dll
with a flag and then gyp creates chrome_main_dll_pgo_instrument,
chrome_main_dll_pgo_update, etc. (haven't really thought that through yet..)

[yukawa, 2014-03-07 05:24:39]

On 2014/03/06 18:39:28, scottmg wrote:
> I think this mostly looks as expected, but I'm not sure it'll work as well
> as/the same as the test in gyp.
> 
> I think in the test it works all the code is in that static_library that's
> linked into the final binary targets, which makes the linker settings affect
all
> that code also.
> 
> In this CL chrome_pgo_main depends on chrome_main_dll but the dll already has
> linked. So, I think it'd have to be restructured to make that a static_library
> instead which is might be a bit more more invasive than a nice standalone
> include.
> 
> +yukawa who wrote this in gyp, and I think has used this on another binary and
> might have suggestions.
> 
> If that restructuring gets too messy, it might be worthwhile to look at making
> it easier for chrome by doing something different in gyp? Maybe something that
> creates additional targets for a given target would work? i.e. chrome_main_dll
> with a flag and then gyp creates chrome_main_dll_pgo_instrument,
> chrome_main_dll_pgo_update, etc. (haven't really thought that through yet..)

Yeah, I wrote that gyp test but I haven't yet applied PGO with GYP for my
(previous)
main project (Google Japanese Input for Windows). So I will not be surprised if
my
proof-of-concept test code isn't applicable for projects in the real world.

Anyway, I left some comments on my initial scan.  I'll follow up this weekend.

[yukawa, 2014-03-07 05:25:22]

Oops, forgot to publish my inline comments.

https://codereview.chromium.org/187813008/diff/100001/chrome/chrome_pgo.gypi
File chrome/chrome_pgo.gypi (right):

https://codereview.chromium.org/187813008/diff/100001/chrome/chrome_pgo.gypi#...
chrome/chrome_pgo.gypi:14: 'type': 'none',
Does this really work?  The 'type' is 'none' here. I'm worried about whether
linker action is kicked here or not. I guess it depends on how the target
generator is implemented in GYP.

https://codereview.chromium.org/187813008/diff/100001/chrome/chrome_pgo.gypi#...
chrome/chrome_pgo.gypi:27: # Tell ninja generator not to pass
/ManifestFile:<filename>
You might be able to drop this hack.  I added this for the VS 2008's linker
because one of gyp build bots was still using it.  I guess having some consensus
about which version of Visual C++ will be supported in Chromium regarding PGO
makes things easy.

[yukawa, 2014-03-10 04:12:39]

Ugh, I was in trouble due to http://crbug.com/350694 and couldn't test well but
I had a feeling that simply copying
https://code.google.com/p/gyp/source/browse/trunk/test/win/linker-flags/pgo.gyp
might not work well easily in the case of Chromium.

I'm wondering if we should start with more naive but simple approach in this
case.

1. In
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_dll....
put the following section into 'chrome_main_dll' target.
  'VCLinkerTool': {
    'ProfileGuidedDatabase': '$(OutDir)\\chrome_dll.pgd',
    'LinkTimeCodeGeneration': '2',
  },

2. Run gyp

3. ninja -C out\Release chrome

4. Replace the section we added in the step 1 as follows and add
'$(OutDir)\\chrome_dll.pgd' into the sources list.
  'VCLinkerTool': {
    'ProfileGuidedDatabase': '$(OutDir)\\<(pgd_basename).pgd',
    'LinkTimeCodeGeneration': '3',
  },
  'sources': [
    '$(OutDir)\\chrome_dll.pgd',
  ],

5. Run gyp

6. ninja -C out\Release chrome

If everything goes fine, we will see PGO link at step 6.  No, to be precise, it
isn't PGO because we didn't run the instrumented build between step 3 and 6.
However, we should be able to see chrome_dll.pgd in out/Release after step 4.
Therefore the step 6 shouldn't fail.

I know we don't want to follow this approach in production but theoretically it
should work.  Once we can confirm this works, then we will actually run
chrome.exe between step 4 and 6 then see what will happen in practice. (Sandbox
pitfalls?  multiprocess conflicts?)

Scott, Sébastien, what do you think?

[Sébastien Marchand, 2014-03-10 04:46:46]

Hey,

I've been able to optimize Chrome with PGO earlier this week, I've used the
approach you've suggested (in a more hacky way, I've edited the .ninja files
directly), I'll forward you the results soon ! I'll try to find a better
approach (that'd be suitable for an eventual usage in production) this week !
Thanks for the suggestions on this CR, sorry if I didn't reply faster, I was
busy benchmarking the PGO builds :)

[Sébastien Marchand, 2014-03-10 22:21:46]

Here's another approach, please note that this doesn't work as is (see the error
message below).

A potential solution would be to remove the target names from chrome_dll.gypi
and to pass them as variable in the script that import this gypi.

In the normal (non PGO) mode we'll have chrome.gyp that'll include chrome.gypi
with the 'normal' targets names (chrome_main_dll, chrome_dll,
chrome_child_dll...). In the PGO mode chrome_dll.gypi will gets included in
chrome.gyp through chrome_pgo.gypi, this way we'll be able to link chrome.dll
with different options... 

The steps will be the following (with chrome_pgo=1 in the GYP_DEFINES):
1) Ninja will build the instrumented targets (chrome_main_dll_pgi and
chrome_child_dll_pgi), which will result in chrome_pgi.dll and
chrome_child_pgi.dll.
2) Then (not present in this patchset), those dlls will be renamed (s/_pgi//)
3) Then we'll do the profile step.
4) Then Ninja will build the optimized targets (chrome_main_dll and
chrome_child_dll), which will result in chrome.dll and chrome_child.dll. The
DLLs renamed in step #2 will be overwritten.

Sounds like a good approach ? If so I'll try to find why it's not working as is
tomorrow.



Here's the error message that I get:
Generating gyp files from GN...
Updating projects from gyp files...
Using overrides found in C:\Users\sebmarchand\.gyp\include.gypi
Traceback (most recent call last):
  File "src/build/gyp_chromium", line 564, in <module>
    gyp_rc = gyp.main(args)
  File "D:\src\chrome\src\tools\gyp\pylib\gyp\__init__.py", line 527, in main
    return gyp_main(args)
  File "D:\src\chrome\src\tools\gyp\pylib\gyp\__init__.py", line 503, in
gyp_main
    options.circular_check)
  File "D:\src\chrome\src\tools\gyp\pylib\gyp\__init__.py", line 129, in Load
    params['parallel'], params['root_targets'])
  File "D:\src\chrome\src\tools\gyp\pylib\gyp\input.py", line 2726, in Load
    RemoveLinkDependenciesFromNoneTargets(targets)
  File "D:\src\chrome\src\tools\gyp\pylib\gyp\input.py", line 1455, in
RemoveLinkDependenciesFromNoneTargets
    if targets[t].get('variables', {}).get('link_dependency', 0):
KeyError: 'D:\\src\\chrome\\src\\chrome\\chrome.gyp:chrome_dll#target'
Error: Command D:\src\depot_tools\python276_bin\python.exe
src/build/gyp_chromium returned non-zero exit status 1 in D:\src\chrome
Hook ''D:\src\depot_tools\python276_bin\python.exe' src/build/gyp_chromium' took
11.83 secs

[scottmg, 2014-03-11 03:30:29]

On 2014/03/10 22:21:46, Sébastien Marchand wrote:
> Here's another approach, please note that this doesn't work as is (see the
error
> message below).

It seems nice enough, if you can get it to work. I'm not sure if it will though?
as the file being included defines targets, and the 'includes' is inside of
other targets in the pgo .gypi.

[Sébastien Marchand, 2014-03-12 22:40:40]

After a long fight with gyp it almost work... (almost=it seems to work but
there's some cleanup to do !).

As is this CL probably break the non official builds and the one with
chrome_multiple_dll=0 but it shouldn't be hard to fix this in another patchset,
for now I just want to be able to automagically produce a pgo-optimized build.

I've updated the CL description to explain my approach, if it sounds good to you
I'll pursue in this direction.

https://codereview.chromium.org/187813008/diff/230001/build/internal/release_...
File build/internal/release_impl_official.gypi (left):

https://codereview.chromium.org/187813008/diff/230001/build/internal/release_...
build/internal/release_impl_official.gypi:34: 'LinkTimeCodeGeneration': '1',
This line shouldn't be removed because it prevent the targets (other than
chrome.dll and chrome_child.dll) to be linked with the /LTCG flag. However if I
don't remove this it overrides the values set in chrome_dll.gypi.

Even if I use <(ltcg_value) here it doesn't work, the last value set in
chrome_pgo.gypi gets used for the instrumented and the optimized version of the
DLL...

[scottmg, 2014-03-12 22:59:25]

Great! I think this is reasonable, if you can iron out the final problems. I
don't think gyp will let this be any simpler.

I'm a little uncertain on connecting the pieces though, in particular the
necessity of the chrome_pgo_optimize step. Could we just have another script
deal with that part? So we'd have chrome_main_dll_pgi, and chrome_main_dll
(which would do PGO iff chrome_pgo==1), but they're just unrelated targets, so
you could drop the chrome_pgo_optimize target and dependency?

[yukawa, 2014-03-13 06:32:32]

> After a long fight with gyp it almost work...
> (almost=it seems to work but there's some cleanup to do !).

Wow! Congrats!
Can you update the CL description to note what gyp variables should be set in
GYP_DEFINES to try this patch?

[Sébastien Marchand, 2014-03-13 17:57:31]

I spoke too fast... :( Even though the ninja files looked okay I'm starting to
loose faith in gyp...

Here's the new problem:

LINK : fatal error LNK1269: inconsistent file
'obj\chrome\chrome_main_dll.test_pgo_main.obj' specified with /LTCG:PGOPTIMIZE
but not with /LTCG:PGINSTRUMENT

(test_pgo_main is the name of a temporary source file that I'm using as the
"body" of chrome.dll and chrome_child.dll, I've remove all the other
dependencies/includes. This way I can link chrome.dll in a few secs !)

This is due to the fact that in the PGInstrumented build the path of this object
file is obj\chrome\chrome_main_dll_pgi.test_pgo_main.obj... And the PGO
optimizer relies on the obj having the same paths (so here the '_pgi' part break
this expectation).

If I patch this (by editing the ninja files directly) I know get an error
because the path of the manifest for chrome_pgi.dll is
D:/src/chrome/src/out/Release/chrome_pgi.dll.manifest and the one of chrome.dll
is D:/src/chrome/src/out/Release/chrome.dll.manifest, which doesn't correspond
to an instrumented binary...

Those paths (for the obj and the manifest) are computed in
gyp/generator/ninja.py, and there's no way to control them from the gyp file.

The 'chrome_pgo_optimize' is necessary if we want to be able to automatically do
all the steps (instrumentation, profiling and optimization) via a single ninja
target. AFAIK the buildbot can only build one target, so a scenario like this
one:
- ninja -C out\Release chrome_pgi
- profiling_script.py
- ninja -C out\Release chrome (PGO optimized)
sounds not viable for an usage on the buildbot... Anyway I now know that this
won't work (because of the problem mentioned above).

The current way of doing a PGO build is hacky:
- ninja -C out\Release chrome (with /LTCG:PGINSTRUMENT, it's equivalent to
chrome_pgi but without the renaming)
- Launch Chrome (with the no-sandbox flag).
- Gather data with pgosweep.exe
- Close Chrome
- s/LTCG:PGINSTRUMENT/LTCG:PGOPTIMIZE/ in
out/Release/obj/chrome/chrome_child_dll.ninja and chrome_main_dll.ninja
- ninja -C out\Release chrome

[scottmg, 2014-03-13 18:25:49]

I'm beginning to wonder if this isn't worth the complexity... Another much
simpler way (from the gyp POV at least), we could just have a script that does
your hacky way:

python build\gyp_chromium -Dchrome_pgo_phase=1
ninja -C out\Release chrome
run + pgosweep
python build\gyp_chromium -Dchrome_pgo_phase=2
ninja -C out\Release chrome

where chrome_pgo_phase (or whatever we want to call it) controls just the
setting of the LTCG value.

I don't know exactly how we would integrate it with buildbot (we'd have to look
into that instead). But as the buildbot scripts are just a bunch of python steps
that are going to have to be customized for pgo anyway, I don't know that it'd
necessarily be any harder for that part.

[Sébastien Marchand, 2014-03-13 22:06:55]

I've abandoned the one-target approach, here's a simpler one (which require to
build chrome_dll twice, with a different gyp_defines each time).

https://codereview.chromium.org/187813008/diff/250001/build/internal/release_...
File build/internal/release_impl_official.gypi (left):

https://codereview.chromium.org/187813008/diff/250001/build/internal/release_...
build/internal/release_impl_official.gypi:34: 'LinkTimeCodeGeneration': '1',
I've moved this to common.gypi, but this add the /LTCG flag to a few targets:
- upgrade_tests
- mksnapshot.ia32
- mini_installer
- mini_installer_syzygy
- d8
- alternate_version_generator

... I'm doing an official build to see if it breaks it.

[scottmg, 2014-03-13 23:04:38]

This LGTM.

I think this is fine, and it might even be better to avoid the additional
complexity of the other approach.

https://codereview.chromium.org/187813008/diff/250001/build/internal/release_...
File build/internal/release_impl_official.gypi (left):

https://codereview.chromium.org/187813008/diff/250001/build/internal/release_...
build/internal/release_impl_official.gypi:34: 'LinkTimeCodeGeneration': '1',
I don't think this will be a problem. Without    'WholeProgramOptimization':
'true' it doesn't have any effect, other than perhaps causing a warning at link
time (and the WPO compiler setting shouldn't have changed here).

[Sébastien Marchand, 2014-03-13 23:30:57]

Thanks, I've sent this to the trybots, if everything's green (or at least if
it's not red because of this CL) I'll land it tomorrow.

[Sébastien Marchand, 2014-03-14 01:25:33]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-03-14 01:26:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/187813008/270001

[commit-bot: I haz the power, 2014-03-14 01:49:01]

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

[commit-bot: I haz the power, 2014-03-14 01:49:02]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Sébastien Marchand, 2014-03-14 01:50:45]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-03-14 01:51:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/187813008/270001

[yukawa, 2014-03-14 01:58:01]

LGTM for starting from hacky but much simpler 2 pass build. Build is hard.

[commit-bot: I haz the power, 2014-03-14 02:17:35]

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

[commit-bot: I haz the power, 2014-03-14 02:17:36]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[Sébastien Marchand, 2014-03-14 02:19:50]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-03-14 02:20:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/187813008/270001

[commit-bot: I haz the power, 2014-03-14 03:15:39]

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

[commit-bot: I haz the power, 2014-03-14 03:15:40]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[sebmarchand (DO NOT USE), 2014-03-14 03:17:49]

The CQ bit was checked by sebmarchand@google.com

[commit-bot: I haz the power, 2014-03-14 03:18:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/187813008/270001

[commit-bot: I haz the power, 2014-03-14 03:21:38]

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

[commit-bot: I haz the power, 2014-03-14 03:21:39]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[sebmarchand (DO NOT USE), 2014-03-14 03:48:22]

The CQ bit was checked by sebmarchand@google.com

[commit-bot: I haz the power, 2014-03-14 03:49:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/187813008/270001

[commit-bot: I haz the power, 2014-03-14 03:59:09]

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

[commit-bot: I haz the power, 2014-03-14 03:59:10]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[Sébastien Marchand, 2014-03-14 13:02:19]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-03-14 13:03:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/187813008/270001

[commit-bot: I haz the power, 2014-03-14 16:48:32]

Change committed as 257133