🐳 Move linux pkg_config() calls into separate BUILD.gn files

Move linux pkg_config() calls into separate BUILD.gn files

This showed up as a bottleneck for gn gen on Android. Refactoring these
out improves gn gen time (avg of 3 trails on my machine) from 2151->1984

BUG=none

Committed: https://crrev.com/95ba4446355ffd95eca68d3c3ba7fc3c2e364b6d
Cr-Commit-Position: refs/heads/master@{#389484}

[agrieve, 2016-04-22 01:20:12]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-22 01:20:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909273002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909273002/1

[commit-bot: I haz the power, 2016-04-22 01:24:38]

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

[commit-bot: I haz the power, 2016-04-22 01:24:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[agrieve, 2016-04-22 01:31:44]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-22 01:31:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909273002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909273002/20001

[agrieve, 2016-04-22 01:32:11]

Description was changed from

==========
Move linux pkg_config() calls into separate BUILD.gn files

This showed up as a bottleneck for gn gen on Android. Refactoring these
out improves gn gen time (avg of 3 trails on my machine) from 2151->1984

BUG=none
==========

to

==========
Move linux pkg_config() calls into separate BUILD.gn files

This showed up as a bottleneck for gn gen on Android. Refactoring these
out improves gn gen time (avg of 3 trails on my machine) from 2151->1984

BUG=none
==========

[commit-bot: I haz the power, 2016-04-22 01:36:26]

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

[commit-bot: I haz the power, 2016-04-22 01:36:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[agrieve, 2016-04-22 01:40:21]

agrieve@chromium.org changed reviewers:
+ brettw@chromium.org

[agrieve, 2016-04-22 13:55:44]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-22 13:56:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909273002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909273002/40001

[brettw, 2016-04-22 18:09:29]

https://codereview.chromium.org/1909273002/diff/60001/build/config/linux/BUIL...
File build/config/linux/BUILD.gn (right):

https://codereview.chromium.org/1909273002/diff/60001/build/config/linux/BUIL...
build/config/linux/BUILD.gn:7: # Exists so that this file can referenced from
//BUILD.gn.
Did you measure this? Now that all but one pkg_configs are gone, I wouldn't
expect much benefit. For the other case below, that one just has a config in it
and I don't really see how pulling that in early should make anything faster (so
definitely remove that one).

https://codereview.chromium.org/1909273002/diff/60001/build/config/posix/BUIL...
File build/config/posix/BUILD.gn (right):

https://codereview.chromium.org/1909273002/diff/60001/build/config/posix/BUIL...
build/config/posix/BUILD.gn:10: group("posix") {
I think you should remove this.

[agrieve, 2016-04-22 18:20:38]

https://codereview.chromium.org/1909273002/diff/60001/build/config/linux/BUIL...
File build/config/linux/BUILD.gn (right):

https://codereview.chromium.org/1909273002/diff/60001/build/config/linux/BUIL...
build/config/linux/BUILD.gn:7: # Exists so that this file can referenced from
//BUILD.gn.
On 2016/04/22 18:09:29, brettw wrote:
> Did you measure this? Now that all but one pkg_configs are gone, I wouldn't
> expect much benefit. For the other case below, that one just has a config in
it
> and I don't really see how pulling that in early should make anything faster
(so
> definitely remove that one).

Yep, this was a result of using gn gen -tracelog. For some reason these
pkg_config calls cause all other threads to be idle (there's a big white column
where these are the only things running). Even having just one call to
pkg_config causes the all-other-threads-idle thing to happen. Both this and the
posix one save 50ms by moving the one call earlier.

Is there a way to include a this file in the root BUILD.gn without having this
group? Maybe by having a dummy config() in //BUILD.gn?

[brettw, 2016-04-22 19:30:21]

LGTM with some tweaks.

This has a nice side effect that if a particular sub-project (say V8, which uses
the build directory) doesn't depend on one of these pkgconfig packages, the
build will not only be faster, but can run without the package installed locally
at all.

https://codereview.chromium.org/1909273002/diff/60001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1909273002/diff/60001/BUILD.gn#newcode741
BUILD.gn:741: "//build/config/linux(//build/toolchain/linux:clang_x64)",
We should be more careful about bringing things in. Here you're bringing in x86
versions of these files. Unless you're targeting x86, these will otherwise never
be needed (the local system will be x64) and you'll actually run pkg_config an
extra and unnecessary time.

Use "...($host_toolchain)" for both targets here, and that should be sufficient.

Can you also add to the comment (I realize why this case is particularly bad
now):

"These run expensive scripts in the host toolchain. Generally host toolchain
targets are loaded in the later part of the run, and the result is they push out
the end of generation. By preloading these, the scripts can be parallelized with
the rest of the load."

https://codereview.chromium.org/1909273002/diff/60001/build/config/linux/BUIL...
File build/config/linux/BUILD.gn (right):

https://codereview.chromium.org/1909273002/diff/60001/build/config/linux/BUIL...
build/config/linux/BUILD.gn:7: # Exists so that this file can referenced from
//BUILD.gn.
On 2016/04/22 18:20:37, agrieve wrote:
> On 2016/04/22 18:09:29, brettw wrote:
> > Did you measure this? Now that all but one pkg_configs are gone, I wouldn't
> > expect much benefit. For the other case below, that one just has a config in
> it
> > and I don't really see how pulling that in early should make anything faster
> (so
> > definitely remove that one).
> 
> Yep, this was a result of using gn gen -tracelog. For some reason these
> pkg_config calls cause all other threads to be idle (there's a big white
column
> where these are the only things running). Even having just one call to
> pkg_config causes the all-other-threads-idle thing to happen. Both this and
the
> posix one save 50ms by moving the one call earlier.
> 
> Is there a way to include a this file in the root BUILD.gn without having this
> group? Maybe by having a dummy config() in //BUILD.gn?

Can you call this something like "optimize_gen_time" or something, and set
visibility to the one target that depends on this so people don't get confused?

https://codereview.chromium.org/1909273002/diff/60001/build/config/posix/BUIL...
File build/config/posix/BUILD.gn (right):

https://codereview.chromium.org/1909273002/diff/60001/build/config/posix/BUIL...
build/config/posix/BUILD.gn:26: [
Oh I see, preloading this one optimizes this sysroot_ld_path call. I was looking
for pkgconfig.

[agrieve, 2016-04-25 14:45:35]

https://codereview.chromium.org/1909273002/diff/60001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1909273002/diff/60001/BUILD.gn#newcode741
BUILD.gn:741: "//build/config/linux(//build/toolchain/linux:clang_x64)",
On 2016/04/22 19:30:21, brettw wrote:
> We should be more careful about bringing things in. Here you're bringing in
x86
> versions of these files. Unless you're targeting x86, these will otherwise
never
> be needed (the local system will be x64) and you'll actually run pkg_config an
> extra and unnecessary time.
> 
> Use "...($host_toolchain)" for both targets here, and that should be
sufficient.
> 
> Can you also add to the comment (I realize why this case is particularly bad
> now):
> 
> "These run expensive scripts in the host toolchain. Generally host toolchain
> targets are loaded in the later part of the run, and the result is they push
out
> the end of generation. By preloading these, the scripts can be parallelized
with
> the rest of the load."

Added a comment. Left in x86 and added a subsequent comment. V8 uses it for
snapshot generation.

https://codereview.chromium.org/1909273002/diff/60001/build/config/linux/BUIL...
File build/config/linux/BUILD.gn (right):

https://codereview.chromium.org/1909273002/diff/60001/build/config/linux/BUIL...
build/config/linux/BUILD.gn:7: # Exists so that this file can referenced from
//BUILD.gn.
On 2016/04/22 19:30:21, brettw wrote:
> On 2016/04/22 18:20:37, agrieve wrote:
> > On 2016/04/22 18:09:29, brettw wrote:
> > > Did you measure this? Now that all but one pkg_configs are gone, I
wouldn't
> > > expect much benefit. For the other case below, that one just has a config
in
> > it
> > > and I don't really see how pulling that in early should make anything
faster
> > (so
> > > definitely remove that one).
> > 
> > Yep, this was a result of using gn gen -tracelog. For some reason these
> > pkg_config calls cause all other threads to be idle (there's a big white
> column
> > where these are the only things running). Even having just one call to
> > pkg_config causes the all-other-threads-idle thing to happen. Both this and
> the
> > posix one save 50ms by moving the one call earlier.
> > 
> > Is there a way to include a this file in the root BUILD.gn without having
this
> > group? Maybe by having a dummy config() in //BUILD.gn?
> 
> Can you call this something like "optimize_gen_time" or something, and set
> visibility to the one target that depends on this so people don't get
confused?

Like it. Done.

[agrieve, 2016-04-25 14:46:01]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-04-25 14:46:01]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1909273002/#ps80001
(title: "comments & visibility")

[commit-bot: I haz the power, 2016-04-25 14:46:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909273002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909273002/80001

[commit-bot: I haz the power, 2016-04-25 14:58:25]

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

[commit-bot: I haz the power, 2016-04-25 14:58:27]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[agrieve, 2016-04-25 15:39:02]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-04-25 15:39:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909273002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909273002/80001

[commit-bot: I haz the power, 2016-04-25 15:47:28]

Description was changed from

==========
Move linux pkg_config() calls into separate BUILD.gn files

This showed up as a bottleneck for gn gen on Android. Refactoring these
out improves gn gen time (avg of 3 trails on my machine) from 2151->1984

BUG=none
==========

to

==========
Move linux pkg_config() calls into separate BUILD.gn files

This showed up as a bottleneck for gn gen on Android. Refactoring these
out improves gn gen time (avg of 3 trails on my machine) from 2151->1984

BUG=none
==========

[commit-bot: I haz the power, 2016-04-25 15:47:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-25 15:48:53]

Description was changed from

==========
Move linux pkg_config() calls into separate BUILD.gn files

This showed up as a bottleneck for gn gen on Android. Refactoring these
out improves gn gen time (avg of 3 trails on my machine) from 2151->1984

BUG=none
==========

to

==========
Move linux pkg_config() calls into separate BUILD.gn files

This showed up as a bottleneck for gn gen on Android. Refactoring these
out improves gn gen time (avg of 3 trails on my machine) from 2151->1984

BUG=none

Committed: https://crrev.com/95ba4446355ffd95eca68d3c3ba7fc3c2e364b6d
Cr-Commit-Position: refs/heads/master@{#389484}
==========

[commit-bot: I haz the power, 2016-04-25 15:48:55]

Patchset 5 (id:??) landed as
https://crrev.com/95ba4446355ffd95eca68d3c3ba7fc3c2e364b6d
Cr-Commit-Position: refs/heads/master@{#389484}