Closed Bug 1510548 Opened Last year Closed 2 months ago

0.42 - 205.1% compiler warnings / installer size (linux32, linux64, osx-cross, windows2012-64, windows2012-aarch64) regression on push 04d915d32eea9361457c6e7b77ebe7341c6e505f (Tue Nov 27 2018)

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 - wontfix
firefox66 - wontfix
firefox67 --- verified

People

(Reporter: igoldan, Assigned: achronop)

References

(Blocks 1 open bug)

Details

(Keywords: backlog-deferred, perf-alert, regression, Whiteboard: [qf-])

Attachments

(1 file)

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=04d915d32eea9361457c6e7b77ebe7341c6e505f

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

Regressions:

205%  compiler warnings windows2012-aarch64 opt msvc-aarch64     98.00 -> 299.00
168%  compiler warnings windows2012-64 opt msvc                  119.00 -> 319.00
  0%  installer size osx-cross opt                               74,785,150.33 -> 75,139,806.92
  0%  installer size linux64 pgo                                 67,765,547.25 -> 68,054,040.75
  0%  installer size windows2012-64 pgo                          68,400,063.25 -> 68,684,832.25
  0%  installer size linux32 pgo                                 67,563,476.50 -> 67,846,578.33


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

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

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***
Component: General → Audio/Video: Playback
Product: Testing → Core
Flags: needinfo?(achronop)
The warnings are produced from third party code imported by this patch. Follow up imports from the upstream will clear some. I will try to find and report them all in upstream repo. In any case please do not backout.
Flags: needinfo?(achronop)
See Also: → 1509327
Rank: 15
Priority: -- → P2
I am expecting that this has been improved after landing of Bug 1509327. Can you please confirm or not ...
Flags: needinfo?(igoldan)
(In reply to Alex Chronopoulos [:achronop] from comment #2)
> I am expecting that this has been improved after landing of Bug 1509327. Can
> you please confirm or not ...

I'm afraid not. I haven't seen any kind of improvement to any of the regressions from comment 0.
Flags: needinfo?(igoldan)
Ok, thank you very much, I'll keep looking
Not a user-facing issue, so no need to track.

Any updates here?

Flags: needinfo?(achronop)

The warnings are coming from 3rd party code. I reported some of them and pulled from upstream. I have also disabled the warning in moz.build. It's strange you haven't seen any improvement. Probably there are more warnings that I haven't found.

Flags: needinfo?(achronop)

(In reply to Alex Chronopoulos [:achronop] from comment #7)

The warnings are coming from 3rd party code. I reported some of them and pulled from upstream. I have also disabled the warning in moz.build. It's strange you haven't seen any improvement. Probably there are more warnings that I haven't found.

We'd like to know whether to resume investigation on this matter or conclude it. If you require assistance with looking over the compiler warnings, just contact Chris Peterson.

Flags: needinfo?(achronop)

Thanks for pinging me on this. I'll take a look again today. I could use some help on finding the building logs and the corresponding warnings. The strategy I follow is to search for them in a random try run. However, I am not sure if we are looking at the same thing. I have cleaned up some warnings but I am not sure how to check your numbers. Chris, any advice is welcome.

Flags: needinfo?(achronop) → needinfo?(cpeterson)

I see you added DisableCompilerWarnings() to media/libdav1d/moz.build. Do you see warnings being logged for dav1d code if you compile locally with MSVC on Windows?

I see DisableCompilerWarnings() [1] clears the WARNINGS_CFLAGS variable. Perhaps MSVC is compiling the dav1d code as C++ so DisableCompilerWarnings() should also clear WARNINGS_CFXXLAGS? I'm just guessing.

[1] https://searchfox.org/mozilla-central/source/build/templates.mozbuild#61-62

Flags: needinfo?(cpeterson)

Here is the list of MSVC warnings in dav1d code from from a "Windows 2012 x64 opt" Bmsvc build job for a random push to mozilla-central:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&selectedJob=225502630

https://taskcluster-artifacts.net/I0lGQwi1QHyJvlstrtr7Fw/0/public/logs/live_backing.log

warning: obj-firefox/media/libdav1d/16bd_cdef_apply_tmpl.c:86 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_cdef_apply_tmpl.c:86 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/16bd_cdef_apply_tmpl.c:86 [C4028] formal parameter 4 different from declaration
warning: obj-firefox/media/libdav1d/16bd_cdef_apply_tmpl.c:86 [C4028] formal parameter 5 different from declaration
warning: obj-firefox/media/libdav1d/16bd_cdef_tmpl.c:255 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 8 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 6 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 12 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 15 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 14 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 4 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 13 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 2 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 9 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 5 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/16bd_ipred_tmpl.c:729 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_itx_tmpl.c:180 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_lf_apply_tmpl.c:178 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/16bd_lf_apply_tmpl.c:178 [C4028] formal parameter 5 different from declaration
warning: obj-firefox/media/libdav1d/16bd_lf_apply_tmpl.c:178 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_loopfilter_tmpl.c:248 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_looprestoration_tmpl.c:577 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_lr_apply_tmpl.c:110 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/16bd_lr_apply_tmpl.c:280 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/16bd_mc_tmpl.c:885 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:366 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:366 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:747 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:747 [C4028] formal parameter 4 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:1145 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:1145 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:1589 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:1589 [C4028] formal parameter 2 different from declaration
warning: obj-firefox/media/libdav1d/16bd_recon_tmpl.c:1658 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_cdef_apply_tmpl.c:86 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_cdef_apply_tmpl.c:86 [C4028] formal parameter 4 different from declaration
warning: obj-firefox/media/libdav1d/8bd_cdef_apply_tmpl.c:86 [C4028] formal parameter 5 different from declaration
warning: obj-firefox/media/libdav1d/8bd_cdef_apply_tmpl.c:86 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/8bd_cdef_tmpl.c:255 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 2 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 4 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 14 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 5 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 9 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 12 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 13 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 6 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 8 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_prepare_tmpl.c:88 [C4028] formal parameter 15 different from declaration
warning: obj-firefox/media/libdav1d/8bd_ipred_tmpl.c:729 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_itx_tmpl.c:180 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_lf_apply_tmpl.c:178 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_lf_apply_tmpl.c:178 [C4028] formal parameter 5 different from declaration
warning: obj-firefox/media/libdav1d/8bd_lf_apply_tmpl.c:178 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/8bd_loopfilter_tmpl.c:248 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_looprestoration_tmpl.c:577 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_lr_apply_tmpl.c:110 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/8bd_lr_apply_tmpl.c:280 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/8bd_mc_tmpl.c:885 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:366 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:366 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:747 [C4028] formal parameter 4 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:747 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:1145 [C4028] formal parameter 3 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:1145 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:1589 [C4028] formal parameter 1 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:1589 [C4028] formal parameter 2 different from declaration
warning: obj-firefox/media/libdav1d/8bd_recon_tmpl.c:1658 [C4028] formal parameter 1 different from declaration

They're all MSVC C4028 warnings:

https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4028

If clearing WARNINGS_CXXFLAGS in DisableCompilerWarnings() doesn't work, then you can probably suppress just the C4024 warnings like in dav1d's moz.build:

CFLAGS += ['-wd4024']

Looking at the first warning, I see that the problem is that the function definition specifies const int parameters (const int by_start) but the function declaration specifies non-const int parameters (int by_start). This is an uncommon coding style but not a bug.

The function's declaration in the header file:
https://searchfox.org/mozilla-central/rev/e62b311b2e0e02633afda2004ba4056b09fbcaa4/third_party/dav1d/src/cdef_apply.h#35-36

void bytefn(dav1d_cdef_brow)(Dav1dFrameContext *f, pixel *const p[3],
                             const Av1Filter *lflvl, int by_start, int by_end);

The function's definition in the .c file:
https://searchfox.org/mozilla-central/source/__GENERATED__/media/libdav1d/16bd_cdef_apply_tmpl.c#82-86

void bytefn(dav1d_cdef_brow)(Dav1dFrameContext *const f,
                             pixel *const p[3],
                             const Av1Filter *const lflvl,
                             const int by_start, const int by_end)

Thank you Chris for the detailed analysis! I reported the warnings upstream in [1]. I'll try to provide a patch (upstream) later today.

https://code.videolan.org/videolan/dav1d/issues/242

I suppressed warning C4028 and I got a try run [1] without those reports.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=345770c5dbd7a6a4a4b9013cd9577ebffde35f22

Phabricator did not post the review:
https://phabricator.services.mozilla.com/D18678

(In reply to Alex Chronopoulos [:achronop] from comment #13)

I suppressed warning C4028 and I got a try run [1] without those reports.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=345770c5dbd7a6a4a4b9013cd9577ebffde35f22

Looks like your fix worked! In the random build I linked to from comment 11, Treeherder reports:

compiler warnings opt msvc: 346

In your Try run, Treeherder reports:

compiler warnings opt msvc: 123

And I don't see any [C4028] formal parameter different from declaration warnings in the build logs.

Pushed by achronopoulos@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce3045659365
Suppress MSVC warning C4024 for libdav1d. r=cpeterson
Assignee: nobody → achronop
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

compiler warnings windows2012-64 opt msvc got fixed.

I'm not yet sure about the AARCH64 warnings.
Chris, have we renamed the platform from msvc-aarch64 to simply aarch64? If that's correct, according to this graph we only did a partial fix.

As for the installer size increases, I didn't see any kind of fix. Alex, should we simply accept the binary increase?

Flags: needinfo?(cpeterson)
Flags: needinfo?(achronop)

Yeah, sorry I haven't mentioned that before, binary increase is expected, we added a whole new library, we need to accept that part.

Flags: needinfo?(achronop)

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #19)

Chris, have we renamed the platform from msvc-aarch64 to simply aarch64? If that's correct, according to this graph we only did a partial fix.

I don't know.

David, was the "msvc-aarch64" platform renamed to just "aarch64" on Treeherder on January 24?

https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=autoland,1823333,1,2&series=autoland,1844705,1,2&highlightedRevisions=ce3045659365

Flags: needinfo?(cpeterson) → needinfo?(dmajor)

Yes, the rename happened as part of bug 1512822 as those builds no longer use msvc.

Flags: needinfo?(dmajor)

Note that clang-cl warns about a lot more things than msvc in general (we saw this last summer when we switched Intel builds over) so looking at the graph alone doesn't consider all of the circumstances.

If the x86_64 warnings are back to where they used to be, then we can consider the regression resolved.

However I did chat with Alex a while back about my opposition to the use of DisableCompilerWarnings() (as opposed to AllowCompilerWarnings()) in dav1d. I don't think we should be wholesale hiding warnings from third-party libraries: our users are exposed to that code just as much as if it were our code, and if that code has problems, or gains problems on a future update, we need to know about it, not sweep it under the rug.

Just a quick note that I am happy to remove DisableCompilerWarnings() if we feel like it. If we decide that we want it please file a bug against me and I'll get it done.

I opened bug 1526435 for that.

Status: RESOLVED → REOPENED
Depends on: 1526435
Resolution: FIXED → ---

Is any of this something you want to fix on 66 beta? Or can it ride with 67?

Flags: needinfo?(achronop)

it can ride on. Those are just compilation warnings.

Flags: needinfo?(achronop)

I don't think we need to do anything for beta, it's not a user-facing issue. However, if the performance team wants it in beta I am more than happy to uplift.

Flags: needinfo?(igoldan)

(In reply to Alex Chronopoulos [:achronop] from comment #28)

I don't think we need to do anything for beta, it's not a user-facing issue. However, if the performance team wants it in beta I am more than happy to uplift.

This kind of metric is not really related to performance as it is to app stability. From my point of view, I would stick with Jean's comment 27, as he has more authority here than I have.
Unless Dave has other thoughts.

Flags: needinfo?(igoldan) → needinfo?(dave.hunt)

Agreed, thanks!

Flags: needinfo?(dave.hunt)
Whiteboard: [qf]

Why did we add perf-related keyword/whiteboard-status here (perf-alert and qf)?

As far as I can tell, this report had nothing to do with Firefox performance. See comment 27-29.

Also, why is this open / is there still a real issue that this is tracking? It looks like we:

  • landed a patch for the compiler warnings in comment 17.
  • acknowledged that the binary-size increase is expected in comment 20.
  • did some additional compiler warning tweaks in followup bug 1526435.

Is there anything here that still needs addressing, or can this be closed?

Whiteboard: [qf] → [qf-]

Also, looks like the bug was closed in comment 18 (when the compiler-warning commit hit central) but was later reopened by :igoldan between comment 25 and comment 26.

:igoldan, do you know why this was reopened & is there still something to be done here?

Flags: needinfo?(igoldan)

(In reply to Daniel Holbert [:dholbert] from comment #32)

Also, looks like the bug was closed in comment 18 (when the compiler-warning commit hit central) but was later reopened by :igoldan between comment 25 and comment 26.

:igoldan, do you know why this was reopened & is there still something to be done here?

I reopened this 7 months ago because of the unresolved bug 1526435 (at that time).

Flags: needinfo?(igoldan)

(In reply to Daniel Holbert [:dholbert] from comment #31)

Why did we add perf-related keyword/whiteboard-status here (perf-alert and qf)?

As far as I can tell, this report had nothing to do with Firefox performance. See comment 27-29.

Also, why is this open / is there still a real issue that this is tracking? It looks like we:

  • landed a patch for the compiler warnings in comment 17.
  • acknowledged that the binary-size increase is expected in comment 20.
  • did some additional compiler warning tweaks in followup bug 1526435.

Is there anything here that still needs addressing, or can this be closed?

You are right. Seems like I've misread things here. I agree this needs to be closed, given resolution of bug 1526435.

Status: NEW → RESOLVED
Closed: 9 months ago2 months ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.