Add support for user defined "pool" to GN.

Add support for user defined "pool" to GN.

Allow user to define pool of limited size to limit the number of
concurrent tasks that are executed for a given set of tools. The
pool object directly correspond to a ninja pool and can be shared
by multiple targets.

Design document:
https://docs.google.com/document/d/1b598i4WmeFOe3_iRBrjPKSXdcR5R32UPtgxqKmoY2-M/view

BUG=612786
Committed: https://crrev.com/40451c5093a4108bf65d3aae30bea2ab014f4b94
Cr-Commit-Position: refs/heads/master@{#397917}

[sdefresne, 2016-05-24 15:09:04]

Patchset #1 (id:1) has been deleted

[sdefresne, 2016-05-24 15:09:25]

sdefresne@chromium.org changed reviewers:
+ brettw@chromium.org

[sdefresne, 2016-05-24 15:09:26]

Please take a look.

[brettw, 2016-05-24 22:03:31]

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/functions.cc#n...
tools/gn/functions.cc:694: "  A pool is referenced by its label just like a
target.\n"
The file containing the pool definition could theoretically be executed in the
context of more than one toolchain. This will give you multiple pool definitions
which is probably not what's desired.

I think we should note here that it's recommended to specify an explicit
toolchain on the pool definition and references to ensure that there is exactly
one pool definition with that name.

Then make the example look like:

  if (current_toolchain == default_toolchain) {
    pool("link_pool") {
      depth = 1"
    }
  }

  ...
       pool = ":link_pool($default_toolchain)"

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/ninja_toolchai...
File tools/gn/ninja_toolchain_writer.cc (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/ninja_toolchai...
tools/gn/ninja_toolchain_writer.cc:84: out_ << "pool " << rule_prefix <<
pool->label().name() << std::endl;
How the naming works here is a bit suspicious.

In the build files, a pool is identified by a label, but then only the past part
of that label is used for the actual pool name, so it must be globally unique.
Nothing else works this way. I think we should generate a name we give to Ninja
for the pool that's based on the full label name. Probably this name should also
include the toolchain of the pool (see my other comment in the documentation).

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/tool.h
File tools/gn/tool.h (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/tool.h#newcode180
tools/gn/tool.h:180: const LabelPtrPair<Pool> pool_label_pair() const { return
pool_; }
According to style I think this should be called pool()/set_pool().

The set_pool call below is a bit weird. Since it's only set once, I think it
might be better just to omit this helper function and have the code in
builder.cc do:
  tool->set_pool(LabelPtrPair<Pool>(record->item()->AsPool()));

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/variables.h
File tools/gn/variables.h (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/variables.h#ne...
tools/gn/variables.h:170: extern const char kDepth[];
This is never used or defined. I think it's OK not to have global definitions
for this since it's only used in one place. Just revert this file.

[Dirk Pranke, 2016-05-25 02:04:51]

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

[Dirk Pranke, 2016-05-25 02:04:52]

don't forget to update //tools/gn/docs/reference.md as well.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/function_toolc...
File tools/gn/function_toolchain.cc (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:551: "        Label of the pool to use for the
tool. Pool are used to limit\n"
Nit: "Pools are used" or "A pool is used"

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:552: "        the number of task to launch
simultaneously during the build.\n"
Nit: "number of tasks". In fact, I'd probably write this as "Pools are used to
limit the number of tasks that can execute concurrently during the build."
"launch simultaneously" is maybe a bit less clear.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/functions.cc#n...
tools/gn/functions.cc:690: "  Pool objects can be applied to tool to limit the
parallelism of the\n"
Nit: "applied to a tool".

[sdefresne, 2016-05-26 15:38:37]

Please take another look. I think I've addressed all comments.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/function_toolc...
File tools/gn/function_toolchain.cc (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:551: "        Label of the pool to use for the
tool. Pool are used to limit\n"
On 2016/05/25 at 02:04:52, Dirk Pranke (slow) wrote:
> Nit: "Pools are used" or "A pool is used"

Done.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:552: "        the number of task to launch
simultaneously during the build.\n"
On 2016/05/25 at 02:04:52, Dirk Pranke (slow) wrote:
> Nit: "number of tasks". In fact, I'd probably write this as "Pools are used to
limit the number of tasks that can execute concurrently during the build."
"launch simultaneously" is maybe a bit less clear.

Done.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/functions.cc#n...
tools/gn/functions.cc:690: "  Pool objects can be applied to tool to limit the
parallelism of the\n"
On 2016/05/25 at 02:04:52, Dirk Pranke (slow) wrote:
> Nit: "applied to a tool".

Done.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/functions.cc#n...
tools/gn/functions.cc:694: "  A pool is referenced by its label just like a
target.\n"
On 2016/05/24 at 22:03:31, brettw wrote:
> The file containing the pool definition could theoretically be executed in the
context of more than one toolchain. This will give you multiple pool definitions
which is probably not what's desired.
> 
> I think we should note here that it's recommended to specify an explicit
toolchain on the pool definition and references to ensure that there is exactly
one pool definition with that name.
> 
> Then make the example look like:
> 
>   if (current_toolchain == default_toolchain) {
>     pool("link_pool") {
>       depth = 1"
>     }
>   }
> 
>   ...
>        pool = ":link_pool($default_toolchain)"

Good point. Done.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/ninja_toolchai...
File tools/gn/ninja_toolchain_writer.cc (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/ninja_toolchai...
tools/gn/ninja_toolchain_writer.cc:84: out_ << "pool " << rule_prefix <<
pool->label().name() << std::endl;
On 2016/05/24 at 22:03:31, brettw wrote:
> How the naming works here is a bit suspicious.
> 
> In the build files, a pool is identified by a label, but then only the past
part of that label is used for the actual pool name, so it must be globally
unique. Nothing else works this way. I think we should generate a name we give
to Ninja for the pool that's based on the full label name. Probably this name
should also include the toolchain of the pool (see my other comment in the
documentation).

Done.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/tool.h
File tools/gn/tool.h (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/tool.h#newcode180
tools/gn/tool.h:180: const LabelPtrPair<Pool> pool_label_pair() const { return
pool_; }
On 2016/05/24 at 22:03:31, brettw wrote:
> According to style I think this should be called pool()/set_pool().
> 
> The set_pool call below is a bit weird. Since it's only set once, I think it
might be better just to omit this helper function and have the code in
builder.cc do:
>   tool->set_pool(LabelPtrPair<Pool>(record->item()->AsPool()));

Done.

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/variables.h
File tools/gn/variables.h (right):

https://codereview.chromium.org/2006923004/diff/20001/tools/gn/variables.h#ne...
tools/gn/variables.h:170: extern const char kDepth[];
On 2016/05/24 at 22:03:31, brettw wrote:
> This is never used or defined. I think it's OK not to have global definitions
for this since it's only used in one place. Just revert this file.

Ack.

[sdefresne, 2016-05-28 12:31:10]

Ping?

[sdefresne, 2016-06-03 07:42:54]

Ping?

[brettw, 2016-06-03 21:24:00]

lgtm

[sdefresne, 2016-06-04 11:32:57]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-06-04 11:32:58]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2006923004/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-04 11:33:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006923004/60001

[commit-bot: I haz the power, 2016-06-04 12:37:43]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-06-04 12:38:46]

Description was changed from

==========
Add support for user defined "pool" to GN.

Allow user to define pool of limited size to limit the number of
concurrent tasks that are executed for a given set of tools. The
pool object directly correspond to a ninja pool and can be shared
by multiple targets.

Design document:
https://docs.google.com/document/d/1b598i4WmeFOe3_iRBrjPKSXdcR5R32UPtgxqKmoY2...

BUG=612786
==========

to

==========
Add support for user defined "pool" to GN.

Allow user to define pool of limited size to limit the number of
concurrent tasks that are executed for a given set of tools. The
pool object directly correspond to a ninja pool and can be shared
by multiple targets.

Design document:
https://docs.google.com/document/d/1b598i4WmeFOe3_iRBrjPKSXdcR5R32UPtgxqKmoY2...

BUG=612786

Committed: https://crrev.com/40451c5093a4108bf65d3aae30bea2ab014f4b94
Cr-Commit-Position: refs/heads/master@{#397917}
==========

[commit-bot: I haz the power, 2016-06-04 12:38:47]

Patchset 3 (id:??) landed as
https://crrev.com/40451c5093a4108bf65d3aae30bea2ab014f4b94
Cr-Commit-Position: refs/heads/master@{#397917}

[Dirk Pranke, 2016-06-06 23:32:18]

Maybe fix the typos and update //tools/gn/docs/reference.md in a follow-up
patch?

https://codereview.chromium.org/2006923004/diff/60001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2006923004/diff/60001/tools/gn/functions.cc#n...
tools/gn/functions.cc:695: "  of more than one toolchain it is recommended to
specify an explicit\n"
nit: "in the context of"

https://codereview.chromium.org/2006923004/diff/60001/tools/gn/functions.cc#n...
tools/gn/functions.cc:696: "  toolchain when definining and referencing a
pool.\n"
nit: "defining"