ThinLTO: limit link / codegen parallelism to 16.

ThinLTO: limit link / codegen parallelism to 16.

Based on the experiments, ThinLTO does not scale beyond that,
and the default setting is the number of cores on a machine, which
is often 32 or more for Chrome devs, where ThinLTO is slower than at 16:
https://docs.google.com/spreadsheets/d/18vi9p8ffIYNVPTyxtJwr-YrP4WJRbaQr_2nZ1AKKBs4/edit?usp=sharing

BUG=645295
Committed: https://crrev.com/a769e96dbeb3c566fb34f91185095cbfb0de36b4
Cr-Commit-Position: refs/heads/master@{#418789}

[krasin1, 2016-09-15 02:05:04]

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

[krasin1, 2016-09-15 02:05:10]

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

[commit-bot: I haz the power, 2016-09-15 02:05:48]

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-09-15 02:53:14]

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

[commit-bot: I haz the power, 2016-09-15 02:53:15]

Dry run: This issue passed the CQ dry run.

[dpranke, 2016-09-15 03:35:20]

dpranke@google.com changed reviewers:
+ dpranke@google.com

[dpranke, 2016-09-15 03:35:20]

lgtm to land since this FYI, but I have a few questions ...

https://codereview.chromium.org/2341983002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2341983002/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:441: # As of now, ThinLTO does scale beyond 16
cores anyway.
I think you mean "does not scale", right?

Does this flag impose a maximum number of jobs, or does it impose a fixed
number, i.e., would we still try to use 16 jobs on a 4-core machine?

Also, is the if (...) check even needed? At this time won't this always be true?

[krasin1, 2016-09-15 03:43:42]

https://codereview.chromium.org/2341983002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2341983002/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:441: # As of now, ThinLTO does scale beyond 16
cores anyway.
On 2016/09/15 03:35:20, dpranke (slow) wrote:
> I think you mean "does not scale", right?
right. Fixed.
> 
> Does this flag impose a maximum number of jobs, or does it impose a fixed
> number, i.e., would we still try to use 16 jobs on a 4-core machine?
Jobs is not really a good name here. It's actually a number of threads that the
linker will start to generate native code from the bitcode. It will still be a
single process (ld.gold with LLVMgold.so loaded) with 16 threads.

It's perfectly fine to run 16 threads on 4-core machine. While the performance
might be 10%-15% lower, I have verified locally that -jobs=48 and one core
(limited with schedtool -a 1) works reasonably fast.
> 
> Also, is the if (...) check even needed? At this time won't this always be
true?
with lld (which we aim to support), the flag will likely be -Wl,--job=16, but
it's not clear yet, as it's only to be implemented. On Mac, the flag will be
different as well.

So, this check is to avoid a time bomb: even though we only test ThinLTO with
Linux+Gold plugin now, I would like ThinLTO to still work for other options too,
may be not as fast as it could be with the proper limit.

[dpranke, 2016-09-15 03:47:46]

On 2016/09/15 03:43:42, krasin1 wrote:
> So, this check is to avoid a time bomb: even though we only test ThinLTO with
> Linux+Gold plugin now, I would like ThinLTO to still work for other options
too,
> may be not as fast as it could be with the proper limit.

I understand what you're saying, but personally I'd rather err on the side of
not having
code that is checking for conditions that don't actually exist at this time; it
makes
the code harder to understand. I'd guess that it'll be pretty obvious a thing to
change
when we're ready for it.

[krasin1, 2016-09-15 03:55:12]

On 2016/09/15 03:47:46, dpranke (slow) wrote:
> On 2016/09/15 03:43:42, krasin1 wrote:
> > So, this check is to avoid a time bomb: even though we only test ThinLTO
with
> > Linux+Gold plugin now, I would like ThinLTO to still work for other options
> too,
> > may be not as fast as it could be with the proper limit.
> 
> I understand what you're saying, but personally I'd rather err on the side of
> not having
> code that is checking for conditions that don't actually exist at this time;
it
> makes
> the code harder to understand. I'd guess that it'll be pretty obvious a thing
to
> change
> when we're ready for it.

okay. Done.

[krasin1, 2016-09-15 03:57:37]

The CQ bit was checked by krasin@chromium.org

[krasin1, 2016-09-15 03:57:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@google.com
Link to the patchset: https://codereview.chromium.org/2341983002/#ps60001
(title: "fmt")

[commit-bot: I haz the power, 2016-09-15 03:57:51]

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-09-15 04:05:24]

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

[commit-bot: I haz the power, 2016-09-15 04:05:25]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dirk Pranke, 2016-09-15 04:09:01]

lgtm

(from the right account)

[krasin1, 2016-09-15 04:34:48]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2016-09-15 04:35:05]

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-09-15 05:21:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-15 05:23:36]

Description was changed from

==========
ThinLTO: limit link / codegen parallelism to 16.

Based on the experiments, ThinLTO does not scale beyond that,
and the default setting is the number of cores on a machine, which
is often 32 or more for Chrome devs, where ThinLTO is slower than at 16:
https://docs.google.com/spreadsheets/d/18vi9p8ffIYNVPTyxtJwr-YrP4WJRbaQr_2nZ1...

BUG=645295
==========

to

==========
ThinLTO: limit link / codegen parallelism to 16.

Based on the experiments, ThinLTO does not scale beyond that,
and the default setting is the number of cores on a machine, which
is often 32 or more for Chrome devs, where ThinLTO is slower than at 16:
https://docs.google.com/spreadsheets/d/18vi9p8ffIYNVPTyxtJwr-YrP4WJRbaQr_2nZ1...

BUG=645295

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

[commit-bot: I haz the power, 2016-09-15 05:23:37]

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