build: Refactor get_concurrent_links.py

build: Refactor get_concurrent_links.py

Instead of passing build config arguments to the python
script, allow caller to provide the memory needed per linker
and the amount of memory to reserve. Then move the logic
specific to build config to gn file, which is a better place
in case the logic gets more complicated in the future.

Then clean up the script. Factor out a function to get the
total ram of system, and unify the logic for calculating the
number of concurrent links.

Note this intends to be a no-op change, but refactored code
is not identical, and there may be corner case differences.

BUG=617429
Committed: https://crrev.com/da2a0136724678276ff3d00307edb0d9c9baf4c2
Cr-Commit-Position: refs/heads/master@{#414084}

[boliu, 2016-08-17 18:41:19]

boliu@chromium.org changed reviewers:
+ dpranke@chromium.org

[boliu, 2016-08-17 18:41:19]

ptal

feels like a blunt fix, but in theory should be correct, but fingers crossed
doesn't make other builders 10 time slower or something..

see bug for details

[boliu, 2016-08-17 21:20:39]

On 2016/08/17 18:41:19, boliu wrote:
> ptal
> 
> feels like a blunt fix, but in theory should be correct, but fingers crossed
> doesn't make other builders 10 time slower or something..
> 
> see bug for details

turns out this is not the root problem of the bug, but good to do regardless

[Dirk Pranke, 2016-08-17 22:18:51]

This is probably too blunt, as you guessed. At the very least, this might mean
that any linux build would do half the concurrent links its doing currently,
which might significantly impact cycle time.

We might want to make this calculation limited to some combination of being an
android target, a static build, or an official build, at least (just like it's
based on LTO or not right now); see crbug.com/617429.

[boliu, 2016-08-20 23:43:29]

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

[commit-bot: I haz the power, 2016-08-20 23:43:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-21 00:34:54]

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

[commit-bot: I haz the power, 2016-08-21 00:34:54]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-08-22 16:41:30]

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

[commit-bot: I haz the power, 2016-08-22 16:42:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2016-08-22 16:51:37]

Description was changed from

==========
Update concurrent build estimate based on android static build

Static android build linker can use up to 20GB of ram, so update script
to reflect this worst case scenario.

BUG=638485
==========

to

==========
build: Refactor get_concurrent_links.py

Instead of passing build config arguments to the python
script, allow caller to provide the memory needed per linker
and the amount of memory to reserve. Then move the logic
specific to build config to gn file, which is a better place
in case the logic gets more complicated in the future.

Then clean up the script. Factor out a function to get the
total ram of system, and unify the logic for calculating the
number of concurrent links.

Note this intends to be a no-op change, but refactored code
is not identical, and there may be corner case differences.

BUG=617429
==========

[commit-bot: I haz the power, 2016-08-22 17:49:28]

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

[commit-bot: I haz the power, 2016-08-22 17:49:29]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-08-22 17:50:41]

ptal, repurposed this CL to refactor this instead

[Dirk Pranke, 2016-08-22 17:54:33]

dpranke@chromium.org changed reviewers:
+ brettw@chromium.org, brucedawson@chromium.org

[Dirk Pranke, 2016-08-22 17:54:33]

I like this approach. brettw@, brucedawson@, what do you think?

cc'ed folks, feel free to chime in as well ...

[brucedawson, 2016-08-22 19:31:32]

A few comments. Overall it seems like an improvement over the current behavior.

I know there have also been discussions about making the dynamic adjustments
dependent on tagging different links as to how intensive they are - can this
work with that concept?

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

https://codereview.chromium.org/2259503002/diff/40001/build/toolchain/concurr...
build/toolchain/concurrent_links.gni:28: _args = [ "--mem_per_link_gb=5" ]
The Windows memory used per linker is highly dependent on the binary being
linked and the build type. The worst-case memory consumption amounts (recorded a
few months ago) are:

64-bit official: 27 GB
32-bit official: 14 GB
64-bit/32-bit static release: 11 GB
64-bit/32-bit component release: 6 GB

These are worst-case numbers, but towards the end of the build process there are
often quite a few worst-cast (test) binaries trying to link simultaneously.

I don't have component debug numbers handy, but I think they are similar to the
component release numbers.

Starting with 5 for compatibility with the existing values is fine, but it might
be worth tweaking after that.

https://codereview.chromium.org/2259503002/diff/40001/build/toolchain/get_con...
File build/toolchain/get_concurrent_links.py (right):

https://codereview.chromium.org/2259503002/diff/40001/build/toolchain/get_con...
build/toolchain/get_concurrent_links.py:59: mem_total_bytes -= reserve_mem_gb *
2**30
You probably need to check for this going negative. I sometimes build at least
parts of Chrome on my 8 GB laptop. Checking for negative here is probably easier
than reasoning about how the remaining calculations behave with negatives.

[Dirk Pranke, 2016-08-22 19:49:26]

On 2016/08/22 19:31:32, brucedawson wrote:
> A few comments. Overall it seems like an improvement over the current
behavior.
> 
> I know there have also been discussions about making the dynamic adjustments
> dependent on tagging different links as to how intensive they are - can this
> work with that concept?

I don't think it's directly applicable, no.

[brucedawson, 2016-08-22 19:58:02]

One other nit - the CL subject and the first line of the commit message
currently don't match - they should.

[boliu, 2016-08-22 20:49:03]

> One other nit - the CL subject and the first line of the commit message
currently don't match - they should.

This CL was originally supposed to raise the linker memory for android, and
became the refactor later. I generally don't change the subject for these to
avoid creating a new email threads. But I guess this one got a bit too far from
the original CL.

Going to update the subject after sending out this comment.

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

https://codereview.chromium.org/2259503002/diff/40001/build/toolchain/concurr...
build/toolchain/concurrent_links.gni:28: _args = [ "--mem_per_link_gb=5" ]
On 2016/08/22 19:31:32, brucedawson wrote:
> The Windows memory used per linker is highly dependent on the binary being
> linked and the build type. The worst-case memory consumption amounts (recorded
a
> few months ago) are:
> 
> 64-bit official: 27 GB
> 32-bit official: 14 GB
> 64-bit/32-bit static release: 11 GB
> 64-bit/32-bit component release: 6 GB
> 
> These are worst-case numbers, but towards the end of the build process there
are
> often quite a few worst-cast (test) binaries trying to link simultaneously.
> 
> I don't have component debug numbers handy, but I think they are similar to
the
> component release numbers.
> 
> Starting with 5 for compatibility with the existing values is fine, but it
might
> be worth tweaking after that.

This was just intended to be a refactor, without changing behavior for any
platform.

I don't personally build for windows, but hopefully your proposed changes will
get easier after this lands.

https://codereview.chromium.org/2259503002/diff/40001/build/toolchain/get_con...
File build/toolchain/get_concurrent_links.py (right):

https://codereview.chromium.org/2259503002/diff/40001/build/toolchain/get_con...
build/toolchain/get_concurrent_links.py:59: mem_total_bytes -= reserve_mem_gb *
2**30
On 2016/08/22 19:31:32, brucedawson wrote:
> You probably need to check for this going negative. I sometimes build at least
> parts of Chrome on my 8 GB laptop. Checking for negative here is probably
easier
> than reasoning about how the remaining calculations behave with negatives.

Done.

[boliu, 2016-08-23 20:38:35]

ping?

[brucedawson, 2016-08-23 20:43:55]

lgtm

[boliu, 2016-08-23 21:07:45]

Dirk, do you want to stamp too?

[Dirk Pranke, 2016-08-24 01:45:56]

ah, yes. lgtm.

[boliu, 2016-08-24 13:49:01]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-08-24 13:49:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 15:58:30]

Description was changed from

==========
build: Refactor get_concurrent_links.py

Instead of passing build config arguments to the python
script, allow caller to provide the memory needed per linker
and the amount of memory to reserve. Then move the logic
specific to build config to gn file, which is a better place
in case the logic gets more complicated in the future.

Then clean up the script. Factor out a function to get the
total ram of system, and unify the logic for calculating the
number of concurrent links.

Note this intends to be a no-op change, but refactored code
is not identical, and there may be corner case differences.

BUG=617429
==========

to

==========
build: Refactor get_concurrent_links.py

Instead of passing build config arguments to the python
script, allow caller to provide the memory needed per linker
and the amount of memory to reserve. Then move the logic
specific to build config to gn file, which is a better place
in case the logic gets more complicated in the future.

Then clean up the script. Factor out a function to get the
total ram of system, and unify the logic for calculating the
number of concurrent links.

Note this intends to be a no-op change, but refactored code
is not identical, and there may be corner case differences.

BUG=617429
==========

[commit-bot: I haz the power, 2016-08-24 15:58:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-24 16:01:06]

Description was changed from

==========
build: Refactor get_concurrent_links.py

Instead of passing build config arguments to the python
script, allow caller to provide the memory needed per linker
and the amount of memory to reserve. Then move the logic
specific to build config to gn file, which is a better place
in case the logic gets more complicated in the future.

Then clean up the script. Factor out a function to get the
total ram of system, and unify the logic for calculating the
number of concurrent links.

Note this intends to be a no-op change, but refactored code
is not identical, and there may be corner case differences.

BUG=617429
==========

to

==========
build: Refactor get_concurrent_links.py

Instead of passing build config arguments to the python
script, allow caller to provide the memory needed per linker
and the amount of memory to reserve. Then move the logic
specific to build config to gn file, which is a better place
in case the logic gets more complicated in the future.

Then clean up the script. Factor out a function to get the
total ram of system, and unify the logic for calculating the
number of concurrent links.

Note this intends to be a no-op change, but refactored code
is not identical, and there may be corner case differences.

BUG=617429

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

[commit-bot: I haz the power, 2016-08-24 16:01:10]

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