Chromium Code Reviews
Help | Chromium Project | Sign in
(39)

Issue 10911082: Load target build files in parallel using Python multiprocessing. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 7 months ago by dmazzoni
Modified:
1 year, 6 months ago
Reviewers:
M-A Ruel, scottmg
CC:
gyp-developer_googlegroups.com
Base URL:
http://git.chromium.org/external/gyp.git@master
Visibility:
Public.

Description

Load target build files in parallel using Python multiprocessing.

This parallelizes the portion of the processing that took the largest fraction of runtime previously. There's more opportunity for parallelization elsewhere, but this seems to have the biggest impact.

I did some testing to verify the output is identical, but mostly with ninja. Someone familiar with all of the generators should make sure I'm not making assumptions I shouldn't be. Also, it might make sense to keep this behind an optional flag or env var for now.

Raw numbers:

GYP_GENERATORS=ninja
time build/gyp_chromium

1. Linux, z600, chromium src + src-internal + full WebKit: 23.14 s ->
16.04 s (30% speedup)
2. MacBook Pro, chromium src only: 36.83 s -> 27.87 (25% speedup)

Patch Set 1 #

Total comments: 12

Patch Set 2 : Avoid double copy #

Patch Set 3 : Put behind flag with env var #

Total comments: 9

Patch Set 4 : Add lock and explicit class for global state #

Total comments: 8

Patch Set 5 : Switch to docstrings, move LoadTargetBuildFileCallback inside ParallelState. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+171 lines, -18 lines) Lint Patch
M pylib/gyp/__init__.py View 1 2 3 4 5 chunks +12 lines, -3 lines 0 comments ? errors Download
M pylib/gyp/input.py View 1 2 3 4 6 chunks +159 lines, -15 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 13
scottmg
This is great! Could you add some timing info to the description? I don't feel ...
1 year, 7 months ago #1
M-A Ruel
https://chromiumcodereview.appspot.com/10911082/diff/1/pylib/gyp/input.py File pylib/gyp/input.py (right): https://chromiumcodereview.appspot.com/10911082/diff/1/pylib/gyp/input.py#newcode356 pylib/gyp/input.py:356: data_keys = set(data.keys()) Why double copy? data_keys = set(data) ...
1 year, 7 months ago #2
dmazzoni
http://codereview.chromium.org/10911082/diff/1/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/10911082/diff/1/pylib/gyp/input.py#newcode356 pylib/gyp/input.py:356: data_keys = set(data.keys()) On 2012/09/05 19:45:39, Marc-Antoine Ruel wrote: ...
1 year, 7 months ago #3
M-A Ruel
http://codereview.chromium.org/10911082/diff/1/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/10911082/diff/1/pylib/gyp/input.py#newcode461 pylib/gyp/input.py:461: def LoadTargetBuildFileCallback(result): A bit of docstrings explaining the dataflows ...
1 year, 7 months ago #4
dmazzoni
I cleaned it up a bit and put the parallel code behind a --parallel flag ...
1 year, 7 months ago #5
dmazzoni
Any other comments from anyone? Now that I've made it optional/opt-in, perhaps someone could just ...
1 year, 7 months ago #6
M-A Ruel
http://codereview.chromium.org/10911082/diff/1/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/10911082/diff/1/pylib/gyp/input.py#newcode507 pylib/gyp/input.py:507: time.sleep(0.003) On 2012/09/06 18:02:36, Marc-Antoine Ruel wrote: > You ...
1 year, 7 months ago #7
dmazzoni
Thanks for the great feedback. I made the global state explicit and documented, and added ...
1 year, 7 months ago #8
dmazzoni
Ping?
1 year, 6 months ago #9
scottmg
On 2012/09/26 16:20:10, Dominic Mazzoni wrote: > Ping? I just tried the patch on Windows. ...
1 year, 6 months ago #10
M-A Ruel
lgtm with some fixes. http://codereview.chromium.org/10911082/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/10911082/diff/12001/pylib/gyp/input.py#newcode447 pylib/gyp/input.py:447: # Wrapper around LoadTargetBuildFile used ...
1 year, 6 months ago #11
dmazzoni
http://codereview.chromium.org/10911082/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/10911082/diff/12001/pylib/gyp/input.py#newcode447 pylib/gyp/input.py:447: # Wrapper around LoadTargetBuildFile used when LoadTargetBuildFile On 2012/09/27 ...
1 year, 6 months ago #12
dmazzoni
1 year, 6 months ago #13
Committed r1508
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6