1465659 - Static initializer counts on windows opt builds are bi-modal

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Mike Hommey [:glandium]]

That's because both opt and opt static analysis builds are sending to the same bucket.

Comment 1

[Mike Hommey [:glandium]]

Example: https://treeherder.mozilla.org/perf.html#/graphs?series=autoland,1689408,1,2

Comment 2

[(Away)]

It's kind of scary that there's a >5x difference between the two.

Comment 3

[Mike Hommey [:glandium]]

And the missing piece is perfherder_extra_options from the mozharness config...

Comment 4

[Gregory Szorc [:gps]]

The check at https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/build_lints.py#17 is supposed to detect this. Obviously there's a gap in the check :/

It would be great if the check were updated to catch this scenario so it doesn't happen again.

Comment 5

[Mike Hommey [:glandium]]

(In reply to David Major [:dmajor] from comment #2)
> It's kind of scary that there's a >5x difference between the two.

Not surprising, since the recent realization is that MSVC is *really* bad at it. Like, it was > 1300 a few days ago.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #4)
> The check at
> https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/
> transforms/build_lints.py#17 is supposed to detect this. Obviously there's a
> gap in the check :/
> 
> It would be great if the check were updated to catch this scenario so it
> doesn't happen again.

There seems to be two problems:
- some perfherder extra options are set from mozharness only. For instance, no taskcluster config is setting PERFHERDER_EXTRA_OPTIONS to contain "static-analysis", yet, it's used in e.g. https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-inbound,1531342,1,2&selected=mozilla-inbound,1531342,342545,478466569
- the code added in bug 1464522 doesn't use PERFHERDER_EXTRA_OPTIONS and the lint can't test that.

Comment 7

[Mike Hommey [:glandium]]

That being said, it kind of seems silly that each PERFHERDER_DATA output in build logs need to set the extra options on their own. Why isn't there one output setting the extra options for all the perfherder data in the log?

Comment 8

[Mike Hommey [:glandium]]

I found why the build lint didn't care about PERFHERDER_EXTRA_OPTIONS not being set, and there are two reasons for this:
- not all "build" types have the build_lints transformation setup in their kind.yml
- even if they did, the build lint itself is called for each kind, but doesn't check unicity across all kinds, only the currently processed kind.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1465659 - Take PERFHERDER_EXTRA_OPTIONS into account for static initializer count report.

Review commit: https://reviewboard.mozilla.org/r/248138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248138/

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1465659 - Check perfherder options across all build kinds.

Review commit: https://reviewboard.mozilla.org/r/248140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248140/

Comment 11

[Mike Hommey [:glandium]]

Attachment: Bug 1465659 - Move perfherder extra options from mozharness to taskcluster.

While some builds have a PERFHERDER_EXTRA_OPTIONS environment set on the
taskcluster side, many others have the equivalent set at the mozharness
level. But only the former are actually linted against, which,
unsurprisingly, translates to conflicting values between some of the
mozharness configs.

So we move those configurations to taskcluster, enable the lint on all
the kinds that look like builds (based on them using the build_attrs
transform), and adjust the values to stop conflicting. Notably, for
searchfox and static-analysis-autotest.

Review commit: https://reviewboard.mozilla.org/r/248142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248142/

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8982111 [details]
Bug 1465659 - Check perfherder options across all build kinds.

https://reviewboard.mozilla.org/r/248140/#review254430

This looks good.  As a note, there's also a validation framework that could do something similar.  However, it runs on fully-developed task definitions, so it would need to do a bunch of heuristics to find things like primary_config.  So this solution is good :)

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8982110 [details]
Bug 1465659 - Take PERFHERDER_EXTRA_OPTIONS into account for static initializer count report.

https://reviewboard.mozilla.org/r/248138/#review254446

Yes please!

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8982112 [details]
Bug 1465659 - Move perfherder extra options from mozharness to taskcluster.

https://reviewboard.mozilla.org/r/248142/#review254448

I didn't fact-check this but it looks like what I expected.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2087bd3daee
Take PERFHERDER_EXTRA_OPTIONS into account for static initializer count report. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/5fc13d0a68ad
Check perfherder options across all build kinds. r=dustin
https://hg.mozilla.org/integration/autoland/rev/dfb4c974e3b0
Move perfherder extra options from mozharness to taskcluster. r=nalexander

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e2087bd3daee
https://hg.mozilla.org/mozilla-central/rev/5fc13d0a68ad
https://hg.mozilla.org/mozilla-central/rev/dfb4c974e3b0