1379880 - 2.46 - 8.27% compiler warnings (android-4-0-armv7-api15, android-4-2-x86, android-5-0-aarch64, android-api-15-gradle, linux32, linux64) regression on push 64ce651bf43d15e711de6e752d80702cd991b336 (Tue Jul 11 2017)

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[Ionuț Goldan [:igoldan]]

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=64ce651bf43d15e711de6e752d80702cd991b336

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  8%  compiler warnings summary linux64 pgo      1,338.58 -> 1,449.25
  4%  compiler warnings summary linux32 pgo      1,352.17 -> 1,408.75
  3%  compiler warnings summary android-4-2-x86 opt 1,231.00 -> 1,262.00
  3%  compiler warnings summary android-api-15-gradle opt 1,240.83 -> 1,271.83
  3%  compiler warnings summary android-4-0-armv7-api15 opt 1,240.92 -> 1,272.00
  2%  compiler warnings summary android-5-0-aarch64 opt 1,261.83 -> 1,292.83


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=7807

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics

Comment 1

[Ionuț Goldan [:igoldan]]

Boris, could you take a look over this build regression? So far, looks like this happens on the Linux and Android platforms.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm happy to look, but the comparison view linked above is pretty useless: it says things like "number of warnings went from 1,240.92 to 1272" (what does "1,240.92" even mean here?) but doesn't say what the new warnings are, or link to the logs it's comparing, even.

The link at the end of comment 0 has absolutely no information about the test involved.

Comment 3

[Boris Zbarsky [:bzbarsky]]

But assuming the "linux64 pgo" bit is comparing https://public-artifacts.taskcluster.net/GQwWt-_qS1eYPrCTGrD8ww/0/public/logs/live_backing.log to https://public-artifacts.taskcluster.net/ZNENK_jITyGgVC3O2AI14A/0/public/logs/live_backing.log it looks like a number of instances of:

  [-Wmaybe-uninitialized] 'tempValue' may be used uninitialized in this function

for the tempValue variable.  This warning is wrong.  tempValue can't be used uninitialized in this function: the only use is guarded by NS_SUCCEDED(rv) and the callee that returns rv always assigns to tempValue on codepaths where it returns NS_OK.  Given the inlining that's supposed to be happening here the compiler should even be succeeding in proving this statically.  Looks like gcc just doesn't manage to, while our other compilers do.

I could add an unnecessary initialization just to shut the warning up, but this is rather performance-sensitive (in the "every instruction counts" sense) code, and if the compiler can't prove the value is in fact initialized it's unlikely to be able to prove that it can remove the initialization.  Given that we have quite a number of these bogus maybe-uninitialized warnings in the tree already (in fact, they're a large majority of all our warnings), my gut feeling is that we should just leave things be here.

Comment 4

[Chris Peterson [:cpeterson]]

gcc's -Wmaybe-uninitialized warnings are false positives about 99% of the time. We should probably just suppress all -Wmaybe-uninitialized warnings.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

given that these are just compiler warnings, I am going to remove this from our tracking bug for firefox 56 perf regressions.