Ninja: separate dependencies for compile steps vs actions/rules/copies

Ninja: separate dependencies for compile steps vs actions/rules/copies

Currently, if library A depends on library B, we always wait until B has linked
before compiling objects for A. That's not necessary, and overly constricts the
build.
Instead, we keep a separate dependency tree of "action" depends and "compile"
depends. Compile steps in target A depend on actions (in target A or any of its
dependencies), but not directly on other compiles. Action steps still depend on
compile and link steps of dependent targets.
That allows for much higher parallelism.

BUG=None
TEST=Make -j 10000 chrome

Committed: http://code.google.com/p/gyp/source/detail?r=1087

[tony, 2011-10-31 20:43:33]

(moving discussion from http://codereview.chromium.org/8417017/ here)

A few comments/questions:
- How will you make sure this doesn't regress in Chromium?  It seems likely that
someone will add an action to a library that breaks this. Should this be a gyp
error?
- Does this work properly for transitive dependencies?  For example, if liba
depends on libb which depends on executable_c, will compilation of liba files
wait for executable_c to be built first?
- If you make a change that doesn't require any .cc files to be recompiled but
does require some actions to run, will building the executable cause the action
to be run?  E.g., if you change an html file sourced by a grd file for a .pak
file, does building chrome cause the pak file to be rebuilt?
- Can you write some unit tests?

[tony, 2011-11-03 23:55:30]

I would love to see this move forward.

Are we waiting for unit tests from Antoine or a review from Nico?

http://codereview.chromium.org/8400082/diff/1/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/8400082/diff/1/pylib/gyp/generator/ninja.py#ne...
pylib/gyp/generator/ninja.py:487: input, _, linkable = self.target_outputs.get(
dep, (None, [], False))
Nit: extra space before |dep|?

[piman, 2011-11-04 03:51:51]

I added a test, and fixed a case where the final target wouldn't always force
dependents if actions were not involved.

http://codereview.chromium.org/8400082/diff/1/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/8400082/diff/1/pylib/gyp/generator/ninja.py#ne...
pylib/gyp/generator/ninja.py:487: input, _, linkable = self.target_outputs.get(
dep, (None, [], False))
On 2011/11/03 23:55:30, tony wrote:
> Nit: extra space before |dep|?

Done.

[Nico, 2011-11-08 04:26:57]

I don't have anything interesting to say; this looks pretty good. I'd add "This
is purely an optimization and should have no functional effect." or something
like this to the CL description.

Two minor points below.

http://codereview.chromium.org/8400082/diff/9001/test/ninja/action_dependenci...
File test/ninja/action_dependencies/gyptest-action-dependencies.py (right):

http://codereview.chromium.org/8400082/diff/9001/test/ninja/action_dependenci...
test/ninja/action_dependencies/gyptest-action-dependencies.py:3: # Copyright (c)
2009 Google Inc. All rights reserved.
daylight saving years

(also elsewhere)

http://codereview.chromium.org/8400082/diff/9001/test/ninja/action_dependenci...
test/ninja/action_dependencies/gyptest-action-dependencies.py:14: test =
TestGyp.TestGyp(formats=['ninja'])
Will this work with other generators? Can you remove the formats= parameter
completely?

[Nico, 2011-11-08 04:28:28]

Backfill from email, since that didn't make it here for some reason:

piman: """
On Mon, Oct 31, 2011 at 1:43 PM, <tony@chromium.org> wrote:
(moving discussion from http://codereview.chromium.org/8417017/ here)

A few comments/questions:
- How will you make sure this doesn't regress in Chromium?  It seems likely that
someone will add an action to a library that breaks this. Should this be a gyp
error?

I don't think this should be a gyp error. There are perfectly legitimate reasons
to add actions to a library target. Doing so won't break anything, at worse it
makes it less efficient. I don't think this is something we should be strictly
enforcing, I think education will go a long way (If we agree on it, I'll send a
PSA to chromium-dev).
 
- Does this work properly for transitive dependencies?  For example, if liba
depends on libb which depends on executable_c, will compilation of liba files
wait for executable_c to be built first?

If libb depends on executable_c, then it does so through an action (no compile
step explicitly takes an executable or a library as a dependency - that's the
basic assumption of this patch). If so, then liba depends on the pre-compile
steps of libb, then it does wait for exe_c to be built first.

- If you make a change that doesn't require any .cc files to be recompiled but
does require some actions to run, will building the executable cause the action
to be run?  E.g., if you change an html file sourced by a grd file for a .pak
file, does building chrome cause the pak file to be rebuilt?

All this patch does is removing unnecessary compile->compile dependencies. There
should be no effect on other types of dependency (compile->actions,
actions->compile, phony target->actions, phony target->compile, etc.).
Since it doesn't add dependencies, it shouldn't cause more rebuilds.
I haven't looked at the specifics of the resources, but AFAICT, 'chrome' exe
depends on stuff that depends on 'chrome_resources'. Making chrome will make
sure that the dependencies, including chrome_resources, will be up-to-date,
which, I think, will do the right thing.
 """

(and some minor additional back and forth, which resulted in a happy tony.)

[piman, 2011-11-08 22:01:41]

http://codereview.chromium.org/8400082/diff/9001/test/ninja/action_dependenci...
File test/ninja/action_dependencies/gyptest-action-dependencies.py (right):

http://codereview.chromium.org/8400082/diff/9001/test/ninja/action_dependenci...
test/ninja/action_dependencies/gyptest-action-dependencies.py:3: # Copyright (c)
2009 Google Inc. All rights reserved.
On 2011/11/08 04:26:57, Nico wrote:
> daylight saving years
> 
> (also elsewhere)

Done.

http://codereview.chromium.org/8400082/diff/9001/test/ninja/action_dependenci...
test/ninja/action_dependencies/gyptest-action-dependencies.py:14: test =
TestGyp.TestGyp(formats=['ninja'])
On 2011/11/08 04:26:57, Nico wrote:
> Will this work with other generators? Can you remove the formats= parameter
> completely?

The test will not work with other generators because:
- it explicitly tests the optimization, which is not implemented (yet?) on other
generators
- it relies on the exact path to output object files, which is generator
dependent, and actually, relies on the ability to build only that object file,
which I don't think is available on all generators.

That being said, I think the optimization could be implemented in the make
generator, in which case, it'll be worth the effort to make that test run there.

[Nico, 2011-11-09 19:47:55]

LGTM

You're a gyp committer, right? Else, ping me, and I'll land it.

http://codereview.chromium.org/8400082/diff/9001/test/ninja/action_dependenci...
File test/ninja/action_dependencies/gyptest-action-dependencies.py (right):

http://codereview.chromium.org/8400082/diff/9001/test/ninja/action_dependenci...
test/ninja/action_dependencies/gyptest-action-dependencies.py:14: test =
TestGyp.TestGyp(formats=['ninja'])
> The test will not work with other generators because:
> - it explicitly tests the optimization, which is not implemented (yet?) on
other
> generators
> - it relies on the exact path to output object files, which is generator
> dependent, and actually, relies on the ability to build only that object file,
> which I don't think is available on all generators.

Makes sense. Maybe add this as a comment.

> That being said, I think the optimization could be implemented in the make
> generator, in which case, it'll be worth the effort to make that test run
there.

Do you have numbers on how much this helps with -j100? (But I guess we could
increase the 100 on the bots if it helps a lot in make, but only with higher
parallelism)

Are you interested in working on this? :-)