Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(342)

Issue 315343003: Add option for link concurrency. (Closed)

Created:
6 years, 6 months ago by mtklein_C
Modified:
6 years, 6 months ago
Reviewers:
iannucci, mtklein, scottmg
CC:
gyp-developer_googlegroups.com, borenet, bsalomon, iannucci
Base URL:
https://chromium.googlesource.com/external/gyp.git@master
Visibility:
Public.

Description

Add option for link concurrency. Skia's buildbots, particularly Windows builds with LTO, are held back by the link pool. We'd like to play around with this number a bit. Any problem checking an environment variable first before doing the RAM-based calculation? Testing: ran with and without GYP_LINK_CONCURRENCY=90, looked at build.ninja Patch from mtklein@chromium.org. R=scottmg@chromium.org Committed: https://code.google.com/p/gyp/source/detail?r=1930

Patch Set 1 #

Total comments: 2

Patch Set 2 : rejigger #

Total comments: 2

Patch Set 3 : zero #

Patch Set 4 : empty broken #

Total comments: 2

Patch Set 5 : ok #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -0 lines) Patch
M pylib/gyp/generator/ninja.py View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
mtklein_C
6 years, 6 months ago (2014-06-05 21:13:19 UTC) #1
scottmg
The ram thing is kind of junk anyway, so lgtm with change below. +iannucci fyi ...
6 years, 6 months ago (2014-06-05 21:44:54 UTC) #2
mtklein
yes, it seems... very conservative. https://codereview.chromium.org/315343003/diff/1/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): https://codereview.chromium.org/315343003/diff/1/pylib/gyp/generator/ninja.py#newcode1585 pylib/gyp/generator/ninja.py:1585: pool_size = os.getenv('GYP_LINK_CONCURRENCY', 0) ...
6 years, 6 months ago (2014-06-05 21:59:39 UTC) #3
mtklein
On 2014/06/05 21:59:39, mtklein wrote: > yes, it seems... very conservative. > > https://codereview.chromium.org/315343003/diff/1/pylib/gyp/generator/ninja.py > ...
6 years, 6 months ago (2014-06-05 22:02:57 UTC) #4
scottmg
A committer has to land the CL manually, I can do that. https://codereview.chromium.org/315343003/diff/20001/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py ...
6 years, 6 months ago (2014-06-05 22:08:28 UTC) #5
mtklein
https://codereview.chromium.org/315343003/diff/20001/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): https://codereview.chromium.org/315343003/diff/20001/pylib/gyp/generator/ninja.py#newcode1585 pylib/gyp/generator/ninja.py:1585: pool_size = os.getenv('GYP_LINK_CONCURRENCY') On 2014/06/05 22:08:28, scottmg wrote: > ...
6 years, 6 months ago (2014-06-05 22:12:57 UTC) #6
scottmg
sorry for all the back and forth on one silly line https://codereview.chromium.org/315343003/diff/2/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): ...
6 years, 6 months ago (2014-06-05 22:21:45 UTC) #7
mtklein
https://codereview.chromium.org/315343003/diff/2/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): https://codereview.chromium.org/315343003/diff/2/pylib/gyp/generator/ninja.py#newcode1585 pylib/gyp/generator/ninja.py:1585: pool_size = os.getenv('GYP_LINK_CONCURRENCY') On 2014/06/05 22:21:45, scottmg wrote: > ...
6 years, 6 months ago (2014-06-05 22:24:51 UTC) #8
scottmg
On 2014/06/05 22:24:51, mtklein wrote: > https://codereview.chromium.org/315343003/diff/2/pylib/gyp/generator/ninja.py > File pylib/gyp/generator/ninja.py (right): > > https://codereview.chromium.org/315343003/diff/2/pylib/gyp/generator/ninja.py#newcode1585 > ...
6 years, 6 months ago (2014-06-05 22:31:40 UTC) #9
scottmg
Committed patchset #5 manually as r1930 (presubmit successful).
6 years, 6 months ago (2014-06-05 22:32:34 UTC) #10
mtklein
On 2014/06/05 22:32:34, scottmg wrote: > Committed patchset #5 manually as r1930 (presubmit successful). Ooh, ...
6 years, 6 months ago (2014-06-06 01:35:32 UTC) #11
iannucci
6 years, 6 months ago (2014-06-06 06:53:07 UTC) #12
Message was sent while issue was closed.
yeah lgtm. the ram thing is doofy, since it assumes that the link targets are
all sizeof(chrome.dll). Which turns out to be a bad assumption if you're not,
you know, chrome.

Powered by Google App Engine
This is Rietveld 408576698