Parallelize ninja generation when GYP_PARALLEL=1. (issue 11026061)

[dmaz...@chromium.org]

Reviewers: scottmg, Nico,

Message:
This should shave a few more seconds off the total time.


Description:
Parallelize ninja generation when GYP_PARALLEL=1.

This is a really easy approach, parallelizing by configs (Debug/Release).
It's probably not worth parallelizing this step much more because there's
some overhead and the total time of this step isn't that large.


Please review this at http://codereview.chromium.org/11026061/

SVN Base: http://git.chromium.org/external/gyp.git@master

Affected files:
M pylib/gyp/__init__.py
M pylib/gyp/generator/ninja.py


Index: pylib/gyp/__init__.py
diff --git a/pylib/gyp/__init__.py b/pylib/gyp/__init__.py
index
31f02d1a3cc728e42828a4d0a134242cafad7582..ac300a903c052744211179a94e51f573fe031ea0
100755
--- a/pylib/gyp/__init__.py
+++ b/pylib/gyp/__init__.py
@@ -47,7 +47,7 @@ def FindBuildFiles():

def Load(build_files, format, default_variables={},
includes=[], depth='.', params=None, check=False,
- circular_check=True, parallel=False):
+ circular_check=True):
"""
Loads one or more specified build files.
default_variables and includes will be copied before use.
@@ -126,7 +126,7 @@ def Load(build_files, format, default_variables={},
# Process the input specific to this generator.
result = gyp.input.Load(build_files, default_variables, includes[:],
depth, generator_input_info, check,
circular_check,
- parallel)
+ params['parallel'])
return [generator] + result

def NameValueListToDict(name_value_list):
@@ -488,15 +488,15 @@ def gyp_main(args):
'cwd': os.getcwd(),
'build_files_arg': build_files_arg,
'gyp_binary': sys.argv[0],
- 'home_dot_gyp': home_dot_gyp}
+ 'home_dot_gyp': home_dot_gyp,
+ 'parallel': options.parallel}

# Start with the default variables from the command line.
[generator, flat_list, targets, data] = Load(build_files, format,
cmdline_default_variables,
includes, options.depth,
params, options.check,
- options.circular_check,
- options.parallel)
+ options.circular_check)

# TODO(mark): Pass |data| for now because the generator needs a list of
# build files that came in. In the future, maybe it should just accept
Index: pylib/gyp/generator/ninja.py
diff --git a/pylib/gyp/generator/ninja.py b/pylib/gyp/generator/ninja.py
index
c11ba9e9c44029d16a3f8f9f2821aa8759166448..8838f2f383b166c33dbab7caf8b8413a20f32219
100644
--- a/pylib/gyp/generator/ninja.py
+++ b/pylib/gyp/generator/ninja.py
@@ -4,6 +4,7 @@

import copy
import hashlib
+import multiprocessing
import os.path
import re
import subprocess
@@ -1736,6 +1737,10 @@ def PerformBuild(data, configurations, params):
subprocess.check_call(arguments)


+def CallGenerateOutputForConfig(arglist):
+ (target_list, target_dicts, data, params, config_name) = arglist
+ GenerateOutputForConfig(target_list, target_dicts, data, params,
config_name)
+
def GenerateOutput(target_list, target_dicts, data, params):
user_config = params.get('generator_flags', {}).get('config', None)
if user_config:
@@ -1743,6 +1748,13 @@ def GenerateOutput(target_list, target_dicts, data,
params):
user_config)
else:
config_names = target_dicts[target_list[0]]['configurations'].keys()
- for config_name in config_names:
- GenerateOutputForConfig(target_list, target_dicts, data, params,
- config_name)
+ if params['parallel']:
+ pool = multiprocessing.Pool(len(config_names))
+ arglists = []
+ for config_name in config_names:
+ arglists.append((target_list, target_dicts, data, params,
config_name))
+ pool.map(CallGenerateOutputForConfig, arglists)
+ else:
+ for config_name in config_names:
+ GenerateOutputForConfig(target_list, target_dicts, data, params,
+ config_name)

[tha...@chromium.org]

Wait, isn't GYP_PARALLEL implemented already? What does it currently
parallelize?

How much does this change help?

http://codereview.chromium.org/11026061/

[fisc...@chromium.org]

drive-by

FWIW, this patch doesn't make a difference on my z620/gprecise/ninja setup.

Before CL:
fischman@fischman-linux ~/src/chromium/src {bgra} $
time ./build/gyp_chromium &&
time ./build/gyp_chromium
Updating projects from gyp files...
Using parallel processing (experimental).

real 0m10.674s
user 0m18.970s
sys 0m1.750s
Updating projects from gyp files...
Using parallel processing (experimental).

real 0m10.379s
user 0m18.830s
sys 0m1.480s

After patching CL:
fischman@fischman-linux ~/src/chromium/src {bgra} $
time ./build/gyp_chromium &&
time ./build/gyp_chromium
Updating projects from gyp files...
Using parallel processing (experimental).

real 0m10.683s
user 0m18.910s
sys 0m1.800s
Updating projects from gyp files...
Using parallel processing (experimental).

real 0m11.217s
user 0m19.260s
sys 0m1.580s


http://codereview.chromium.org/11026061/

[dmaz...@chromium.org]

On 2012/10/05 06:45:25, Nico wrote:
> Wait, isn't GYP_PARALLEL implemented already? What does it currently
> parallelize?

> How much does this change help?

My first patch parallelized parsing the input files, which took the
majority of
the time of a gyp run.

On my Mac, this saves 2 seconds. On my z620, the generation time is only 2
seconds, so there's probably not any room to speed up after overhead.

Are there 4 configs on Windows? It could potentially help more there.

I was looking into more places to parallelize, but it appears to get more
difficult after this.


http://codereview.chromium.org/11026061/

[tha...@chromium.org]

Ok.

Any reason to not just always do this, instead of having it behind a flag?

http://codereview.chromium.org/11026061/

[dmaz...@chromium.org]

On 2012/10/05 07:09:34, Nico wrote:
> Ok.

> Any reason to not just always do this, instead of having it behind a flag?

I didn't want to break everyone's build if there was a bug.

Also, I think it shouldn't be the default for all configurations - like a
single-processor machine, but also it's possible that for a heavily-loaded
machine (like a build slave?) it could make things worse, not better.


http://codereview.chromium.org/11026061/

[tha...@chromium.org]

On 2012/10/05 07:14:17, Dominic Mazzoni wrote:
> On 2012/10/05 07:09:34, Nico wrote:
> > Ok.
> >
> > Any reason to not just always do this, instead of having it behind a
> flag?

> I didn't want to break everyone's build if there was a bug.

That'd justify putting every single change behind a flag. There'll probably
be
no bug, and if there is, gyp rolls are easy to revert.

> Also, I think it shouldn't be the default for all configurations - like a
> single-processor machine, but also it's possible that for a heavily-loaded
> machine (like a build slave?) it could make things worse, not better.

Can you measure if that's the case? (Disable all but one core locally, run a
python shell with an endless loop.)

Just like in chrome ui, it's better to make these decisions yourself
instead of
leaving the choice to the user who won't know how which option to pick.

(My guess is that it's fine to just disable it, but if it's not, you should
gate
this on the number of processors or the load average, not on an option)

http://codereview.chromium.org/11026061/

[sco...@chromium.org]

Super, thanks! (this was at my behest because windows spends about 4x the
time
in generation due to more configs and slower writes)

It's almost as much of a win on z620/ssd/win as the loading parallelization:

Non-parallel: 57.8s
Loading-only parallel: 46.0s
Loading and generation parallel: 36.8s

Code lgtm.

FWIW, I agree with Nico, turn it on by default and if nothing breaks on the
bots, roll on.

http://codereview.chromium.org/11026061/

[dmaz...@chromium.org]

On 2012/10/05 16:29:43, scottmg wrote:
> Non-parallel: 57.8s
> Loading-only parallel: 46.0s
> Loading and generation parallel: 36.8s

Fantastic!

> FWIW, I agree with Nico, turn it on by default and if nothing breaks on
> the
> bots, roll on.

Looks like the failure case is bugging some people - i.e. if there's a gyp
error
or if you try to cancel it, it can hang, or at least not die cleanly. Let's
fix
that before making it the default.

I agree about flag bloat. I don't think this should be a user option, it
should
"just work". But I still think it was the right decision to make it opt-in
until
we get all of the kinks worked out.


http://codereview.chromium.org/11026061/

[sco...@chromium.org]

On 2012/10/08 17:02:46, Dominic Mazzoni wrote:
> On 2012/10/05 16:29:43, scottmg wrote:
> > Non-parallel: 57.8s
> > Loading-only parallel: 46.0s
> > Loading and generation parallel: 36.8s

> Fantastic!

Only 35s to go! ;)

> > FWIW, I agree with Nico, turn it on by default and if nothing breaks on
> the
> > bots, roll on.

> Looks like the failure case is bugging some people - i.e. if there's a gyp
error
> or if you try to cancel it, it can hang, or at least not die cleanly.
> Let's
fix
> that before making it the default.

> I agree about flag bloat. I don't think this should be a user option, it
should
> "just work". But I still think it was the right decision to make it opt-in
until
> we get all of the kinks worked out.

Yup, sounds good. I hadn't noticed the failure case (Because Ctrl-Break
works
fine, Ctrl-C doesn't).

Looks like the callback never hits in that case.
Searching "keyboardinterrupt
multiprocessing" makes it look slightly-annoying-but-possible to make work
properly.

http://codereview.chromium.org/11026061/