Rework how MB and GN handle concurrent links.

Rework how MB and GN handle concurrent links.

The Mac and iOS GN bots don't currently limit the number of concurrent
links. The official continuous win bots aren't on MB because they can't
limit the number of concurrent links through MB.

This CL adds a 'concurrent_links' arg to GN, which calls the
get_concurrent_links script to get the appropriate default value
(making this consistent across platforms), and also adds a hack
to MB to translate a gyp_link_concurrent "GYP_DEFINE" to the
GYP_LINK_CONCURRENCY env var.

The CL also adds a test for this MB hack and the similar, already existing
LLVM_FORCE_HEAD_REVISION hack.

R=scottmg@chromium.org, brettw@chromium.org
BUG=602480, 611491, 616390
Committed: https://crrev.com/ec079268641d3d284884a5acf55d75af6065f549
Cr-Commit-Position: refs/heads/master@{#398200}

[Dirk Pranke, 2016-06-03 05:44:43]

I'm attempting to kill two birds with one stone here. Let me know what you
think?

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.gni
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:47: concurrent_links =
exec_script("get_concurrent_links.py", args, "value")
@brettw - am I right in thinking/remembering that this is only evaluated once
now, or is it once per toolchain or even once per import? 

If one of the latter two, do you have suggestions for how to rework this so that
I don't have to duplicate it in three toolchain BUILD files, or should I just
duplicate it?

https://codereview.chromium.org/2031233002/diff/1/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2031233002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:2221: # to be more conservative so that we don't need
this.
@scottmg / @brucedawson - thoughts on what we should do w/ the windows link
defaults?

[Dirk Pranke, 2016-06-03 05:44:55]

Description was changed from

==========
Rework how MB and GN handles concurrent links.

The Mac and iOS GN bots don't currently limit the number of concurrent
links. The official continuous win bots aren't on MB because they can't
limit the number of concurrent links through MB.

This CL adds a 'concurrent_links' arg to GN, which calls the
get_concurrent_links script to get the appropriate default value
(making this consistent across platforms), and also adds a hack
to MB to translate a gyp_link_concurrent "GYP_DEFINE" to the
GYP_LINK_CONCURRENCY env var.

The CL also adds a test for this MB hack and the similar, already existing
LLVM_FORCE_HEAD_REVISION hack.

R=scottmg@chromium.org, brettw@chromium.org
BUG=602480, 611491, 616390
==========

to

==========
Rework how MB and GN handle concurrent links.

The Mac and iOS GN bots don't currently limit the number of concurrent
links. The official continuous win bots aren't on MB because they can't
limit the number of concurrent links through MB.

This CL adds a 'concurrent_links' arg to GN, which calls the
get_concurrent_links script to get the appropriate default value
(making this consistent across platforms), and also adds a hack
to MB to translate a gyp_link_concurrent "GYP_DEFINE" to the
GYP_LINK_CONCURRENCY env var.

The CL also adds a test for this MB hack and the similar, already existing
LLVM_FORCE_HEAD_REVISION hack.

R=scottmg@chromium.org, brettw@chromium.org
BUG=602480, 611491, 616390
==========

[brucedawson, 2016-06-03 19:42:16]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2016-06-03 19:42:17]

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.gni
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:14: # In particular, when using distributed
compilation (like w/ goma),
This is true even without goma. On a Z840 we default to 48 concurrent compiles
but only 12 concurrent links, and even then we can end up consuming twice as
much RAM as the machine contains.

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:42: if (allow_posix_link_time_opt || is_cfi) {
Should we pass along more arguments to control the calculation? For instance, on
Windows Debug builds use significantly more memory than release builds.
Component builds and win_fastlink builds both use significantly less (and
component win_fastlink builds use very little).

LTCG on Windows also increases memory usage - about as much as debug
non-component builds.

I have scripts that can run a matrix of build tests while monitoring linker
memory if we want more inputs, or we could just pass in the information and
ignore it for now.

https://codereview.chromium.org/2031233002/diff/1/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2031233002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:2221: # to be more conservative so that we don't need
this.
On 2016/06/03 05:44:43, Dirk Pranke wrote:
> @scottmg / @brucedawson - thoughts on what we should do w/ the windows link
> defaults?

How much memory do the official builders have? The worst-case six links for
official builds would be six of the 64-bit test binaries which have a combined
linker working set of 82 GB. For smaller binaries the memory footprint can be an
order of magnitude less. Dynamic adjustment would be great but that is
definitely a separate bug (crbug.com/610459).

LTCG linking is multi-threaded in some stages which means that crazy parallelism
is less important on official builds when trying to maximize the hardware.

[brettw, 2016-06-03 21:31:29]

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.gni
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:42: if (allow_posix_link_time_opt || is_cfi) {
I'm OK writing a TODO with the information you wrote here.

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:47: concurrent_links =
exec_script("get_concurrent_links.py", args, "value")
This will be executed once for each toolchain that this file is imported in.

I believe this is only necessary in the default toolchain, which is the
toolchain where toolchains are defined.

We could keep the definition in this header and conditionally define it so that
it only exists in the default toolchain.

Theoretically, the build/toolchain/*/BUILD.gn files should only ever be opened
in the default toolchain. It might be cleaner to make a new .gni file and assert
that the current toolchain is the default toolchain in that file. Then it can
only be used from the toolchain definition BUILD files.

[brucedawson, 2016-06-03 22:04:51]

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.gni
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:42: if (allow_posix_link_time_opt || is_cfi) {
On 2016/06/03 21:31:29, brettw wrote:
> I'm OK writing a TODO with the information you wrote here.

Sounds good.

[Dirk Pranke, 2016-06-05 01:19:49]

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.gni
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:14: # In particular, when using distributed
compilation (like w/ goma),
On 2016/06/03 19:42:17, brucedawson wrote:
> This is true even without goma. On a Z840 we default to 48 concurrent compiles
> but only 12 concurrent links, and even then we can end up consuming twice as
> much RAM as the machine contains.

Acknowledged.

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:42: if (allow_posix_link_time_opt || is_cfi) {
On 2016/06/03 22:04:50, brucedawson wrote:
> On 2016/06/03 21:31:29, brettw wrote:
> > I'm OK writing a TODO with the information you wrote here.
> 
> Sounds good.

Will add.

https://codereview.chromium.org/2031233002/diff/1/build/toolchain/toolchain.g...
build/toolchain/toolchain.gni:47: concurrent_links =
exec_script("get_concurrent_links.py", args, "value")
On 2016/06/03 21:31:29, brettw wrote:
> This will be executed once for each toolchain that this file is imported in.
> 
> I believe this is only necessary in the default toolchain, which is the
> toolchain where toolchains are defined.
> 
> We could keep the definition in this header and conditionally define it so
that
> it only exists in the default toolchain.
> 
> Theoretically, the build/toolchain/*/BUILD.gn files should only ever be opened
> in the default toolchain. It might be cleaner to make a new .gni file and
assert
> that the current toolchain is the default toolchain in that file. Then it can
> only be used from the toolchain definition BUILD files.

Acknowledged.

https://codereview.chromium.org/2031233002/diff/1/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2031233002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:2221: # to be more conservative so that we don't need
this.
On 2016/06/03 19:42:17, brucedawson wrote:
> On 2016/06/03 05:44:43, Dirk Pranke wrote:
> > @scottmg / @brucedawson - thoughts on what we should do w/ the windows link
> > defaults?
> 
> How much memory do the official builders have? The worst-case six links for
> official builds would be six of the 64-bit test binaries which have a combined
> linker working set of 82 GB. For smaller binaries the memory footprint can be
an
> order of magnitude less. Dynamic adjustment would be great but that is
> definitely a separate bug (crbug.com/610459).
> 
> LTCG linking is multi-threaded in some stages which means that crazy
parallelism
> is less important on official builds when trying to maximize the hardware.

looks like 256GB on linux, 128 on win, and 64 on mac.

[Dirk Pranke, 2016-06-05 01:22:35]

Patchset #2 (id:20001) has been deleted

[Dirk Pranke, 2016-06-05 01:24:04]

Patchset #2 (id:40001) has been deleted

[Dirk Pranke, 2016-06-05 01:26:14]

Patchset #2 (id:60001) has been deleted

[Dirk Pranke, 2016-06-05 01:27:19]

Updated. Please take another look?

[brettw, 2016-06-06 17:33:39]

lgtm

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/concurr...
File build/toolchain/concurrent_links.gni (right):

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/concurr...
build/toolchain/concurrent_links.gni:8: 
Can you delete this blank line so that it's clear the comment goes with the
assertion.

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/concurr...
build/toolchain/concurrent_links.gni:24: args = [ "--lto" ]
This should be preceeded by an underscore to avoid polluting the namespace of
files that include it with an "args" variable.

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/gcc_too...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/gcc_too...
build/toolchain/gcc_toolchain.gni:101: template("gcc_toolchain") {
Can you add a comment in here that "concurrent_links" is inherited from the .gni
file? Otherwise it will be confusing searching this file for that and not
finding it.

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:44: # concurrent_links is picked up from the
declare_arg().
Can you mention the .gni file here?

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/win/BUI...
File build/toolchain/win/BUILD.gn (left):

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/win/BUI...
build/toolchain/win/BUILD.gn:51: template("msvc_toolchain") {
Same comment in this file.

[Dirk Pranke, 2016-06-06 22:16:13]

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/concurr...
File build/toolchain/concurrent_links.gni (right):

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/concurr...
build/toolchain/concurrent_links.gni:8: 
On 2016/06/06 17:33:38, brettw wrote:
> Can you delete this blank line so that it's clear the comment goes with the
> assertion.

Acknowledged.

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/concurr...
build/toolchain/concurrent_links.gni:24: args = [ "--lto" ]
On 2016/06/06 17:33:38, brettw wrote:
> This should be preceeded by an underscore to avoid polluting the namespace of
> files that include it with an "args" variable.

Acknowledged.

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/gcc_too...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/2031233002/diff/80001/build/toolchain/gcc_too...
build/toolchain/gcc_toolchain.gni:101: template("gcc_toolchain") {
On 2016/06/06 17:33:38, brettw wrote:
> Can you add a comment in here that "concurrent_links" is inherited from the
.gni
> file? Otherwise it will be confusing searching this file for that and not
> finding it.

Yeah, I waffled on whether to add that to all of the toolchains. Will do.

[Dirk Pranke, 2016-06-06 22:23:14]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-06-06 22:23:15]

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/2031233002/#ps100001
(title: "update w/ review feedback")

[commit-bot: I haz the power, 2016-06-06 22:23:53]

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

[commit-bot: I haz the power, 2016-06-06 23:11:40]

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

[commit-bot: I haz the power, 2016-06-06 23:11:41]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Dirk Pranke, 2016-06-06 23:13:39]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-06-06 23:13:49]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-06-06 23:14:10]

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

[commit-bot: I haz the power, 2016-06-07 02:21:34]

Description was changed from

==========
Rework how MB and GN handle concurrent links.

The Mac and iOS GN bots don't currently limit the number of concurrent
links. The official continuous win bots aren't on MB because they can't
limit the number of concurrent links through MB.

This CL adds a 'concurrent_links' arg to GN, which calls the
get_concurrent_links script to get the appropriate default value
(making this consistent across platforms), and also adds a hack
to MB to translate a gyp_link_concurrent "GYP_DEFINE" to the
GYP_LINK_CONCURRENCY env var.

The CL also adds a test for this MB hack and the similar, already existing
LLVM_FORCE_HEAD_REVISION hack.

R=scottmg@chromium.org, brettw@chromium.org
BUG=602480, 611491, 616390
==========

to

==========
Rework how MB and GN handle concurrent links.

The Mac and iOS GN bots don't currently limit the number of concurrent
links. The official continuous win bots aren't on MB because they can't
limit the number of concurrent links through MB.

This CL adds a 'concurrent_links' arg to GN, which calls the
get_concurrent_links script to get the appropriate default value
(making this consistent across platforms), and also adds a hack
to MB to translate a gyp_link_concurrent "GYP_DEFINE" to the
GYP_LINK_CONCURRENCY env var.

The CL also adds a test for this MB hack and the similar, already existing
LLVM_FORCE_HEAD_REVISION hack.

R=scottmg@chromium.org, brettw@chromium.org
BUG=602480, 611491, 616390
==========

[commit-bot: I haz the power, 2016-06-07 02:21:35]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-06-07 02:24:54]

Description was changed from

==========
Rework how MB and GN handle concurrent links.

The Mac and iOS GN bots don't currently limit the number of concurrent
links. The official continuous win bots aren't on MB because they can't
limit the number of concurrent links through MB.

This CL adds a 'concurrent_links' arg to GN, which calls the
get_concurrent_links script to get the appropriate default value
(making this consistent across platforms), and also adds a hack
to MB to translate a gyp_link_concurrent "GYP_DEFINE" to the
GYP_LINK_CONCURRENCY env var.

The CL also adds a test for this MB hack and the similar, already existing
LLVM_FORCE_HEAD_REVISION hack.

R=scottmg@chromium.org, brettw@chromium.org
BUG=602480, 611491, 616390
==========

to

==========
Rework how MB and GN handle concurrent links.

The Mac and iOS GN bots don't currently limit the number of concurrent
links. The official continuous win bots aren't on MB because they can't
limit the number of concurrent links through MB.

This CL adds a 'concurrent_links' arg to GN, which calls the
get_concurrent_links script to get the appropriate default value
(making this consistent across platforms), and also adds a hack
to MB to translate a gyp_link_concurrent "GYP_DEFINE" to the
GYP_LINK_CONCURRENCY env var.

The CL also adds a test for this MB hack and the similar, already existing
LLVM_FORCE_HEAD_REVISION hack.

R=scottmg@chromium.org, brettw@chromium.org
BUG=602480, 611491, 616390

Committed: https://crrev.com/ec079268641d3d284884a5acf55d75af6065f549
Cr-Commit-Position: refs/heads/master@{#398200}
==========

[commit-bot: I haz the power, 2016-06-07 02:24:55]

Patchset 3 (id:??) landed as
https://crrev.com/ec079268641d3d284884a5acf55d75af6065f549
Cr-Commit-Position: refs/heads/master@{#398200}