Implement tracking of BUILD.gn files used to define target, toolchain or

Implement tracking of BUILD.gn files used to define target, toolchain or
pool.

Gn analyze will prune targets that could not be affected by changens in
specified BUILD.gn or .gni file.

Now each Scope knows set of source files, used to load that scope. And
each Item inherits that set from scope.

R=brettw
BUG=555273
Review-Url: https://codereview.chromium.org/2940873002
Cr-Commit-Position: refs/heads/master@{#483337}
Committed: https://chromium.googlesource.com/chromium/src/+/44fd682dac1080143af656bb828036b837aa2521

[alex-ac, 2017-06-14 14:34:40]

brettw, please take a look.

[brettw, 2017-06-23 21:31:37]

Sorry to have not done this yet! Dirk has been poking me about it. I've been
heads done on some issues but should have more time starting Monday,

[brettw, 2017-06-27 00:50:40]

Did you consider using "const InputFile*" instead of uint32_t hashes of file
names?

The InputFile pointers are guaranteed stable (they're referenced by all Location
objects. There are two main places you call AddSourceFile: ImportManager and
Loader. I believe we could get the InputFile pointers to both of those places
without too much effort.

There are two other places where we set this: for the dotfile and the
build_config file. But we can always assume those files are dependencies when
running analyze so we don't necessarily need to copy them all over the place
when running.

https://codereview.chromium.org/2940873002/diff/1/tools/gn/scope.h
File tools/gn/scope.h (right):

https://codereview.chromium.org/2940873002/diff/1/tools/gn/scope.h#newcode103
tools/gn/scope.h:103: const std::set<uint32_t>& source_files_hashes);
I believe a base::flat_set<uint32_t> will be more efficient in this use. set
will allocate for every insertion which will slow it down, and I suspect moving
ints for insertion will be much faster than a heap allocation. This will also be
much faster for making copies of the set (one allocation rather than N) which it
looks like the patch does a lot.

Maybe we want a typedef for this.

[alex-ac, 2017-06-27 13:46:32]

I've replaces uint32_t hashes of file names with const InputFile*. I've also
replaced std::set with base::flat_set and added typedef for it.

I think that FlatContainerDupes argument for flat_set constructor is a bit weird
(for example, it looks really strange with pod-types). I've added default value
for that argument to make it easier to use flat_set in place of std::set.

https://codereview.chromium.org/2940873002/diff/1/tools/gn/scope.h
File tools/gn/scope.h (right):

https://codereview.chromium.org/2940873002/diff/1/tools/gn/scope.h#newcode103
tools/gn/scope.h:103: const std::set<uint32_t>& source_files_hashes);
On 2017/06/27 00:50:40, brettw wrote:
> I believe a base::flat_set<uint32_t> will be more efficient in this use. set
> will allocate for every insertion which will slow it down, and I suspect
moving
> ints for insertion will be much faster than a heap allocation. This will also
be
> much faster for making copies of the set (one allocation rather than N) which
it
> looks like the patch does a lot.
> 
> Maybe we want a typedef for this.

Done.

[alex-ac, 2017-06-27 13:47:11]

Description was changed from

==========
Implement tracking of BUILD.gn files used to define target, toolchain or
pool.

Gn analyze will prune targets that could not be affected by changens in
specified BUILD.gn or .gni file.

Now each Scope knows set of source files, used to load that scope. And
eath Item inherits that set from scope.

I used hashes of source file names instead of names itselves, because of
memory and cpu overhead for copying/comparasion of strings (which can
add approximately 300ms for gn gen run).

R=brettw
BUG=555273
==========

to

==========
Implement tracking of BUILD.gn files used to define target, toolchain or
pool.

Gn analyze will prune targets that could not be affected by changens in
specified BUILD.gn or .gni file.

Now each Scope knows set of source files, used to load that scope. And
each Item inherits that set from scope.

R=brettw
BUG=555273
==========

[dyaroshev, 2017-06-27 13:51:18]

On 2017/06/27 at 13:46:32, alex-ac wrote:
> I've replaces uint32_t hashes of file names with const InputFile*. I've also
replaced std::set with base::flat_set and added typedef for it.
> 
> I think that FlatContainerDupes argument for flat_set constructor is a bit
weird (for example, it looks really strange with pod-types). I've added default
value for that argument to make it easier to use flat_set in place of std::set.
> 

@brettw, even though I originally was against this, I now think that it's a 99%
case and if one cares he/she can look at the documentation. The compilation
errors are way more annoying in my opinion. What do you think?

[brettw, 2017-06-28 20:49:41]

Looks great. LGTM with a few tweaks.

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/analyzer.cc
File tools/gn/analyzer.cc (right):

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:406: if (file->value() == "//.gn" || *file ==
build_config_file_)
Somewhat annoyingly, the dotfile can be overridden on the command line. This was
used by nacl (not sure if it still is) and some other projects that wanted to be
DEPSed in in weird ways. I think we will still want to support this.

Setup has a dotfile_name_ member that we can expose a getter for and pass to the
analyzer. The annoying thing will be that this is a base::FilePath. I think we
can call SourceDir.ResolveRelativeFile with the UTF-8 version of the
base::FilePath to convert here. You can use "//" for the SourceDir to resolve
against (the input should be absolute).

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/loader.cc
File tools/gn/loader.cc (right):

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/loader.cc#newc...
tools/gn/loader.cc:254: const auto location = root->GetRange().begin();
Good idea, I thought you would have to add a new parameter here.

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/scope_unittest.cc
File tools/gn/scope_unittest.cc (right):

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/scope_unittest...
tools/gn/scope_unittest.cc:196: // Source file hashes are merged.
remove "hashes"

[alex-ac, 2017-06-29 10:26:57]

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/analyzer.cc
File tools/gn/analyzer.cc (right):

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/analyzer.cc#ne...
tools/gn/analyzer.cc:406: if (file->value() == "//.gn" || *file ==
build_config_file_)
On 2017/06/28 20:49:40, brettw wrote:
> Somewhat annoyingly, the dotfile can be overridden on the command line. This
was
> used by nacl (not sure if it still is) and some other projects that wanted to
be
> DEPSed in in weird ways. I think we will still want to support this.
> 
> Setup has a dotfile_name_ member that we can expose a getter for and pass to
the
> analyzer. The annoying thing will be that this is a base::FilePath. I think we
> can call SourceDir.ResolveRelativeFile with the UTF-8 version of the
> base::FilePath to convert here. You can use "//" for the SourceDir to resolve
> against (the input should be absolute).

Done.

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/scope_unittest.cc
File tools/gn/scope_unittest.cc (right):

https://codereview.chromium.org/2940873002/diff/40001/tools/gn/scope_unittest...
tools/gn/scope_unittest.cc:196: // Source file hashes are merged.
On 2017/06/28 20:49:40, brettw wrote:
> remove "hashes"

Done.

[alex-ac, 2017-06-29 10:27:06]

The CQ bit was checked by alex-ac@yandex-team.ru

[alex-ac, 2017-06-29 10:27:07]

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/2940873002/#ps60001
(title: "Use .gn file path from setup.")

[commit-bot: I haz the power, 2017-06-29 10:27:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 10:39:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 10:39:25]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex-ac, 2017-06-29 10:59:54]

The CQ bit was checked by alex-ac@yandex-team.ru

[alex-ac, 2017-06-29 10:59:55]

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/2940873002/#ps80001
(title: "Fix compilation after rebase.")

[commit-bot: I haz the power, 2017-06-29 11:00:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 12:41:32]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498733994866490,
"parent_rev": "11bf2c457dacb04bcfbd93569eb3972a6dfc868e", "commit_rev":
"44fd682dac1080143af656bb828036b837aa2521"}

[commit-bot: I haz the power, 2017-06-29 12:41:48]

Description was changed from

==========
Implement tracking of BUILD.gn files used to define target, toolchain or
pool.

Gn analyze will prune targets that could not be affected by changens in
specified BUILD.gn or .gni file.

Now each Scope knows set of source files, used to load that scope. And
each Item inherits that set from scope.

R=brettw
BUG=555273
==========

to

==========
Implement tracking of BUILD.gn files used to define target, toolchain or
pool.

Gn analyze will prune targets that could not be affected by changens in
specified BUILD.gn or .gni file.

Now each Scope knows set of source files, used to load that scope. And
each Item inherits that set from scope.

R=brettw
BUG=555273

Review-Url: https://codereview.chromium.org/2940873002
Cr-Commit-Position: refs/heads/master@{#483337}
Committed:
https://chromium.googlesource.com/chromium/src/+/44fd682dac1080143af656bb8280...
==========

[commit-bot: I haz the power, 2017-06-29 12:41:49]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/44fd682dac1080143af656bb8280...