Flip CQ builders that are building 'all' to 'gn_all'.

Flip CQ builders that are building 'all' to 'gn_all'.

R=jam@chromium.org, thakis@chromium.org
BUG=618066, 555273

[Dirk Pranke, 2016-06-09 17:26:53]

Description was changed from

==========
Flip CQ builders that are building 'all' to 'gn_all'.

R=jam@chromium.org, thakis@chromium.org
BUG=618066
==========

to

==========
Flip CQ builders that are building 'all' to 'gn_all'.

R=jam@chromium.org, thakis@chromium.org
BUG=618066
==========

[Dirk Pranke, 2016-06-09 17:27:45]

Description was changed from

==========
Flip CQ builders that are building 'all' to 'gn_all'.

R=jam@chromium.org, thakis@chromium.org
BUG=618066
==========

to

==========
Flip CQ builders that are building 'all' to 'gn_all'.

R=jam@chromium.org, thakis@chromium.org
BUG=618066, 555273
==========

[Dirk Pranke, 2016-06-09 17:28:03]

I'll let you two decide which parts of the CL we should land :).

[brettw, 2016-06-09 17:34:02]

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

[brettw, 2016-06-09 17:34:03]

lgtm

[Nico, 2016-06-09 17:34:20]

Can you summarize why gn_all is easier to analyze than all?
On Jun 9, 2016 1:28 PM, <dpranke@chromium.org> wrote:

> Reviewers: jam, Nico
> CL: https://codereview.chromium.org/2055853002/
>
> Message:
> I'll let you two decide which parts of the CL we should land :).
>
> Description:
> Flip CQ builders that are building 'all' to 'gn_all'.
>
> R=jam@chromium.org, thakis@chromium.org
> BUG=618066, 555273
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+21, -27 lines):
> M testing/buildbot/chromium.json
> M testing/buildbot/chromium.chromiumos.json
> M testing/buildbot/chromium.linux.json
> M testing/buildbot/chromium.win.json
>
>
> Index: testing/buildbot/chromium.chromiumos.json
> diff --git a/testing/buildbot/chromium.chromiumos.json
> b/testing/buildbot/chromium.chromiumos.json
> index
>
4aaf391efbe887f42ceb7064a2e16be08cbcc101..99b20c74ee0cdc497a7411e50c0548ec2a96bf6f
> 100644
> --- a/testing/buildbot/chromium.chromiumos.json
> +++ b/testing/buildbot/chromium.chromiumos.json
> @@ -1,7 +1,7 @@
> {
> "Linux ChromiumOS Builder": {
> "additional_compile_targets": [
> - "All"
> + "gn_all"
> ]
> },
> "Linux ChromiumOS GN": {
> Index: testing/buildbot/chromium.json
> diff --git a/testing/buildbot/chromium.json
> b/testing/buildbot/chromium.json
> index
>
f47bedd47a7d268d5743df3d0b6227ad3041e744..28ee1703e1fa6e65ab3d54de7de499cae4c4e786
> 100644
> --- a/testing/buildbot/chromium.json
> +++ b/testing/buildbot/chromium.json
> @@ -1,57 +1,46 @@
> {
> - "Linux": {
> + "Linux x64": {
> "additional_compile_targets": [
> - "all"
> + "gn_all"
> ],
> "scripts": [
> {
> - "name": "checklicenses",
> - "script": "checklicenses.py"
> - },
> - {
> - "name": "checkperms",
> - "script": "checkperms.py"
> - },
> - {
> "args": [
> - "linux-release/sizes"
> + "linux-release-64/sizes"
> ],
> "name": "sizes",
> "script": "sizes.py"
> }
> ]
> },
> - "Linux x64": {
> + "Mac": {
> "additional_compile_targets": [
> "all"
> ],
> "scripts": [
> {
> "args": [
> - "linux-release-64/sizes"
> + "mac-release/sizes"
> ],
> "name": "sizes",
> "script": "sizes.py"
> }
> ]
> },
> - "Mac": {
> + "Win": {
> "additional_compile_targets": [
> - "all"
> + "gn_all"
> ],
> "scripts": [
> {
> - "args": [
> - "mac-release/sizes"
> - ],
> - "name": "sizes",
> - "script": "sizes.py"
> + "name": "checkbins",
> + "script": "checkbins.py"
> }
> ]
> },
> - "Win": {
> + "Win x64": {
> "additional_compile_targets": [
> - "all"
> + "gn_all"
> ],
> "scripts": [
> {
> Index: testing/buildbot/chromium.linux.json
> diff --git a/testing/buildbot/chromium.linux.json
> b/testing/buildbot/chromium.linux.json
> index
>
571905b83d9f5eabf8c7cfcae50d94cf36d2d46e..fc21822612648e233484c99136256c527def2a71
> 100644
> --- a/testing/buildbot/chromium.linux.json
> +++ b/testing/buildbot/chromium.linux.json
> @@ -1,7 +1,7 @@
> {
> "Android Arm64 Builder (dbg)": {
> "additional_compile_targets": [
> - "all"
> + "gn_all"
> ]
> },
> "Android Builder": {
> @@ -17,7 +17,7 @@
> },
> "Android Clang Builder (dbg)": {
> "additional_compile_targets": [
> - "all"
> + "gn_all"
> ]
> },
> "Android Tests": {
> @@ -989,12 +989,12 @@
> },
> "Linux Builder": {
> "additional_compile_targets": [
> - "all"
> + "gn_all"
> ]
> },
> "Linux Builder (dbg)": {
> "additional_compile_targets": [
> - "all"
> + "gn_all"
> ]
> },
> "Linux Tests": {
> Index: testing/buildbot/chromium.win.json
> diff --git a/testing/buildbot/chromium.win.json
> b/testing/buildbot/chromium.win.json
> index
>
cd8b382b9d88a37bcb6fd92acda82bb8dc5853c0..98e65234a261e3cd62636dded029e138bcb758e3
> 100644
> --- a/testing/buildbot/chromium.win.json
> +++ b/testing/buildbot/chromium.win.json
> @@ -2328,5 +2328,10 @@
> "test": "events_unittests"
> }
> ]
> + },
> + "WinClang64 (dbg)": {
> + "additional_compile_targets": [
> + "gn_all"
> + ]
> }
> }
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2016-06-09 17:50:49]

On 2016/06/09 17:34:20, Nico wrote:
> Can you summarize why gn_all is easier to analyze than all?

Yes, that's a good question. There are two reasons, and this patch really only
addresses the first one, but that's enough to make things better than they are
today.

The first is that 'gn_all' is a target that actually exists in GN's build graph,
but 'all' doesn't, it's
really a pseudo-target that gets tacked on when generating the ninja files. 

The way analyze works in GN currently is we run `gn refs out/Default
@list_of_files_patch.rsp --all`
and take the intersection of the targets that are returned (i.e., the targets
that are affected), and
the targets that we care about.

And, the list that gets returned is *every* target that is affected, not just
the list of executables other nodes near
the root of the graph that you'd ideally want. So, we'd have to pass a list of
thousands of targets
to compile.py, or query GN repeatedly to try and reduce that list. 

Since we can't do this efficiently, we just give up and pass 'all' to ninja.
Changing to 'gn_all' will improve
this.

The second problem is that even if you switch to `gn_all`, we will still end up
over-building, because gn_all is a group()
target, and we'll build both the members of the group that are affected by the
patch, and the members of
the group that aren't (i.e., if the patch touches a file in net, but
base_unittests is out-of-date for other reasons,
we'll still build it). This is why we treat compile_targets and test_targets
differently in analyze, and we need even
more information from the build graph to do this correctly, but we can't get the
information we need efficiently
by calling GN on the command line.

`analyze` works differently in GYP because it actually gets the whole build
graph and is able
to determine the ideal list of things to return in that situation.

The way to fix analyze in GN is well understood, it just hasn't been a priority
and so we haven't made the time to fix it.

[Dirk Pranke, 2016-06-09 17:53:59]

On 2016/06/09 17:50:49, Dirk Pranke wrote:
> The way to fix analyze in GN is well understood, it just hasn't been a
priority
> and so we haven't made the time to fix it.

And I would guess it's probably less than a days' worth of work to do so for
someone familiar w/ the GN code.

[Nico, 2016-06-09 18:43:47]

Thanks for explaining! One follow-up question:

On 2016/06/09 17:50:49, Dirk Pranke wrote:
> On 2016/06/09 17:34:20, Nico wrote:
> > Can you summarize why gn_all is easier to analyze than all?
> 
> Yes, that's a good question. There are two reasons, and this patch really only
> addresses the first one, but that's enough to make things better than they are
> today.
> 
> The first is that 'gn_all' is a target that actually exists in GN's build
graph,
> but 'all' doesn't, it's
> really a pseudo-target that gets tacked on when generating the ninja files. 
> 
> The way analyze works in GN currently is we run `gn refs out/Default
> @list_of_files_patch.rsp --all`
> and take the intersection of the targets that are returned (i.e., the targets
> that are affected), and
> the targets that we care about.
> 
> And, the list that gets returned is *every* target that is affected, not just
> the list of executables other nodes near
> the root of the graph that you'd ideally want. So, we'd have to pass a list of
> thousands of targets
> to compile.py, or query GN repeatedly to try and reduce that list. 
> 
> Since we can't do this efficiently, we just give up and pass 'all' to ninja.
> Changing to 'gn_all' will improve
> this.

Why? Does analyze now pass "gn_all" in the "i give up" case, or does it not use
`gn refs out/Default @list_of_files_patch.rsp --all` now? If the latter, what
does it do instead? Pass gn_all instead of --all go `gn refs`? In addition to
--all?

Or do you mean that taking the intersection now yields :gn_all?

I read `gn help refs` for a while and played with it, but I couldn't come up
with a mental model that makes sense to me, sorry :-(

[Dirk Pranke, 2016-06-09 18:51:52]

On 2016/06/09 18:43:47, Nico wrote:
> Thanks for explaining! One follow-up question:
> 
> On 2016/06/09 17:50:49, Dirk Pranke wrote:
> > On 2016/06/09 17:34:20, Nico wrote:
> > > Can you summarize why gn_all is easier to analyze than all?
> > 
> > Yes, that's a good question. There are two reasons, and this patch really
only
> > addresses the first one, but that's enough to make things better than they
are
> > today.
> > 
> > The first is that 'gn_all' is a target that actually exists in GN's build
> graph,
> > but 'all' doesn't, it's
> > really a pseudo-target that gets tacked on when generating the ninja files. 
> > 
> > The way analyze works in GN currently is we run `gn refs out/Default
> > @list_of_files_patch.rsp --all`
> > and take the intersection of the targets that are returned (i.e., the
targets
> > that are affected), and
> > the targets that we care about.
> > 
> > And, the list that gets returned is *every* target that is affected, not
just
> > the list of executables other nodes near
> > the root of the graph that you'd ideally want. So, we'd have to pass a list
of
> > thousands of targets
> > to compile.py, or query GN repeatedly to try and reduce that list. 
> > 
> > Since we can't do this efficiently, we just give up and pass 'all' to ninja.
> > Changing to 'gn_all' will improve
> > this.
> 
> Why? Does analyze now pass "gn_all" in the "i give up" case, or does it not
use
> `gn refs out/Default @list_of_files_patch.rsp --all` now? 

In the "I give up" case, we don't call `gn refs` at all, we exit early and pass
back `all` unchanged.

If we get handed "gn_all" instead, we can at least detect whether the files in
the patch
affect *anything*, and if they don't, we'll return that no compile is necessary.
As per
sky's suggestion, we can probably catch this in the "all" case as well, though.
This is
probably a ~4 line change in MB and doesn't need any GN changes, I think.

> If the latter, what does it do instead? Pass gn_all instead of --all go `gn
refs`? In addition to
> --all?

It looks to see if "gn_all" is in the output of `gn refs --all`.

> Or do you mean that taking the intersection now yields :gn_all?

Right.

[jam, 2016-06-09 19:36:17]

I don't know enough about this change, i.e. why am I added? :)

I read the above comments, but not clear to me whether this change is just
preparatory for better analyze, or if it'll also improve things now.

[Dirk Pranke, 2016-06-09 20:09:58]

On 2016/06/09 19:36:17, jam wrote:
> I don't know enough about this change, i.e. why am I added? :)
> 
> I read the above comments, but not clear to me whether this change is just
> preparatory for better analyze, or if it'll also improve things now.

Well, you're the one who made this a P0 bug by complaining that we shouldn't be
building 'all'.

With this change, as explained above, changing things to 'gn_all' will mean that
a CL that doesn't
affect any build targets will be treated (correctly) as "no compile necessary",
fixing the problem
that you are complaining about.

On that bug, however, thakis argued that it's better that we keep building
'all', at least in some cases,
because we don't currently guarantee that everything affected by a patch is in
fact accounted for
in 'gn_all', and so this change might let regressions slip through. He also
argued that since we're
within the SLO and don't currently have capacity issues, there's no real need to
make this change.

So, you two need to decide who wins, or propose that we fix something else
instead.

[Nico, 2016-06-09 20:12:40]

I agree this isn't p0. I can try to make mb's analyze a bit smarter tomorrow.

[jam, 2016-06-09 23:14:39]

On 2016/06/09 20:09:58, Dirk Pranke wrote:
> On 2016/06/09 19:36:17, jam wrote:
> > I don't know enough about this change, i.e. why am I added? :)
> > 
> > I read the above comments, but not clear to me whether this change is just
> > preparatory for better analyze, or if it'll also improve things now.
> 
> Well, you're the one who made this a P0 bug by complaining that we shouldn't
be
> building 'all'.
> 
> With this change, as explained above, changing things to 'gn_all' will mean
that
> a CL that doesn't
> affect any build targets will be treated (correctly) as "no compile
necessary",
> fixing the problem
> that you are complaining about.

what about a change that only affects say extensions_unittests. will that be the
only target that's built and tested?

> 
> On that bug, however, thakis argued that it's better that we keep building
> 'all', at least in some cases,
> because we don't currently guarantee that everything affected by a patch is in
> fact accounted for
> in 'gn_all', and so this change might let regressions slip through. He also
> argued that since we're
> within the SLO and don't currently have capacity issues, there's no real need
to
> make this change.
> 
> So, you two need to decide who wins, or propose that we fix something else
> instead.

[jam, 2016-06-09 23:16:59]

On 2016/06/09 20:09:58, Dirk Pranke wrote:
> On 2016/06/09 19:36:17, jam wrote:
> > I don't know enough about this change, i.e. why am I added? :)
> > 
> > I read the above comments, but not clear to me whether this change is just
> > preparatory for better analyze, or if it'll also improve things now.
> 
> Well, you're the one who made this a P0 bug by complaining that we shouldn't
be
> building 'all'.
> 
> With this change, as explained above, changing things to 'gn_all' will mean
that
> a CL that doesn't
> affect any build targets will be treated (correctly) as "no compile
necessary",
> fixing the problem
> that you are complaining about.
> 
> On that bug, however, thakis argued that it's better that we keep building
> 'all', at least in some cases,
> because we don't currently guarantee that everything affected by a patch is in
> fact accounted for
> in 'gn_all', and so this change might let regressions slip through. He also
> argued that since we're
> within the SLO and don't currently have capacity issues, there's no real need
to
> make this change.

not sure where this comment is? I looked in both bugs. I had seen your arguments
to this point, but i had tried to address them in
https://bugs.chromium.org/p/chromium/issues/detail?id=618066#c12 (i.e. that SLO
isn't the only thing that matters).

> 
> So, you two need to decide who wins, or propose that we fix something else
> instead.

[Dirk Pranke, 2016-06-09 23:20:31]

On 2016/06/09 23:14:39, jam wrote:
> On 2016/06/09 20:09:58, Dirk Pranke wrote:
> > On 2016/06/09 19:36:17, jam wrote:
> > > I don't know enough about this change, i.e. why am I added? :)
> > > 
> > > I read the above comments, but not clear to me whether this change is just
> > > preparatory for better analyze, or if it'll also improve things now.
> > 
> > Well, you're the one who made this a P0 bug by complaining that we shouldn't
> be
> > building 'all'.
> > 
> > With this change, as explained above, changing things to 'gn_all' will mean
> that
> > a CL that doesn't
> > affect any build targets will be treated (correctly) as "no compile
> necessary",
> > fixing the problem
> > that you are complaining about.
> 
> what about a change that only affects say extensions_unittests. will that be
the
> only target that's built and tested?

No. 'gn_all' will be built. That's the second part of the problem I explained in
comment #7,
but in a little more detail ...

MB (running outside of GN and hence not having access to the full build graph) 
doesn't know anything about 'gn_all' and so can't tell that we should only build
the
affected dependencies of gn_all, and not gn_all itself. This is the difference
between
gn_all being listed as an additional_compile_target (which should be "pruned" as
we call it)
and being listed as a test_target (which would need to be completely built).

[Dirk Pranke, 2016-06-09 23:33:33]

On 2016/06/09 23:16:59, jam wrote:
> On 2016/06/09 20:09:58, Dirk Pranke wrote:
> > On 2016/06/09 19:36:17, jam wrote:
> > > I don't know enough about this change, i.e. why am I added? :)
> > > 
> > > I read the above comments, but not clear to me whether this change is just
> > > preparatory for better analyze, or if it'll also improve things now.
> > 
> > Well, you're the one who made this a P0 bug by complaining that we shouldn't
> be
> > building 'all'.
> > 
> > With this change, as explained above, changing things to 'gn_all' will mean
> that
> > a CL that doesn't
> > affect any build targets will be treated (correctly) as "no compile
> necessary",
> > fixing the problem
> > that you are complaining about.
> > 
> > On that bug, however, thakis argued that it's better that we keep building
> > 'all', at least in some cases,
> > because we don't currently guarantee that everything affected by a patch is
in
> > fact accounted for
> > in 'gn_all', and so this change might let regressions slip through. He also
> > argued that since we're
> > within the SLO and don't currently have capacity issues, there's no real
need
> to
> > make this change.
> 
> not sure where this comment is? I looked in both bugs.

https://bugs.chromium.org/p/chromium/issues/detail?id=618066#c2

and also comment 4.

>I had seen your arguments
> to this point, but i had tried to address them in
> https://bugs.chromium.org/p/chromium/issues/detail?id=618066#c12 (i.e. that
SLO
> isn't the only thing that matters).

It's possible (even likely) that we see increased flakiness, but it's unclear
what the impact
of that actually is. The "CQ Stats for the week of" emails that phajdan.jr@
sends out has 
a very high number for linux_chromium_rel_ng on May 30, but I think that's
because the 
tree was actually busted for one day. The previous few weeks all have flakiness
reports of 
~8% or lower, which is I think about historically average.

Which, again, doesn't mean that we shouldn't keep trying to improve things, but
it suggests
that this isn't a P0 regression.

It would probably be good to formalize the acceptable level of flakiness as a
SLO as well.

[jam, 2016-06-10 01:37:53]

On 2016/06/09 23:20:31, Dirk Pranke wrote:
> On 2016/06/09 23:14:39, jam wrote:
> > On 2016/06/09 20:09:58, Dirk Pranke wrote:
> > > On 2016/06/09 19:36:17, jam wrote:
> > > > I don't know enough about this change, i.e. why am I added? :)
> > > > 
> > > > I read the above comments, but not clear to me whether this change is
just
> > > > preparatory for better analyze, or if it'll also improve things now.
> > > 
> > > Well, you're the one who made this a P0 bug by complaining that we
shouldn't
> > be
> > > building 'all'.
> > > 
> > > With this change, as explained above, changing things to 'gn_all' will
mean
> > that
> > > a CL that doesn't
> > > affect any build targets will be treated (correctly) as "no compile
> > necessary",
> > > fixing the problem
> > > that you are complaining about.
> > 
> > what about a change that only affects say extensions_unittests. will that be
> the
> > only target that's built and tested?
> 
> No. 'gn_all' will be built. That's the second part of the problem I explained
in
> comment #7,
> but in a little more detail ...
> 
> MB (running outside of GN and hence not having access to the full build graph)

> doesn't know anything about 'gn_all' and so can't tell that we should only
build
> the
> affected dependencies of gn_all, and not gn_all itself. This is the difference
> between
> gn_all being listed as an additional_compile_target (which should be "pruned"
as
> we call it)
> and being listed as a test_target (which would need to be completely built).

I see, thanks for the explanation. If this change only covers the no-code at all
case, I personally don't think it's worth it to land. We should focus the effort
on getting analyze working fully as before (instead of this one corner case).

[Dirk Pranke, 2016-06-16 00:42:12]

closing this ...