1692140 - 50.12% pdfpaint (windows10-64-shippable-qr) regression on push 7fe4363fd980fc54a1ae73033f26b61708c3eaea (Mon February 8 2021)

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Perfherder has detected a talos performance regression from push 7fe4363fd980fc54a1ae73033f26b61708c3eaea. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
65%	pdfpaint		linux64-shippable	e10s stylo	321.56 -> 530.55
58%	pdfpaint		windows10-64-shippable	e10s stylo	347.76 -> 549.34
57%	pdfpaint		windows10-64-shippable-qr	e10s stylo webrender	342.06 -> 536.18
54%	pdfpaint		linux64-shippable-qr	e10s stylo webrender-sw	330.77 -> 509.27
53%	pdfpaint		macosx1014-64-shippable-qr	e10s stylo webrender	440.50 -> 674.64
52%	pdfpaint		windows10-64-shippable-qr	e10s stylo webrender-sw	350.70 -> 532.90
50%	pdfpaint		windows10-64-shippable-qr	e10s stylo webrender-sw	350.69 -> 526.46
49%	pdfpaint		linux64-shippable-qr	e10s stylo webrender	342.41 -> 509.27
45%	pdfpaint		macosx1014-64-shippable-qr	e10s stylo webrender-sw	453.38 -> 655.49

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1618553

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Brendan are you familiar with this test? It seems it wasn't really testing all that much before my patch since it wasn't waiting for the pages to load?

If so, maybe this is wontfix?

Comment 3

[Brendan Dahl [:bdahl]]

I added pdfpaint. It measures from performance.timing.navigationStart to pagerendered (first page of a pdf rendered to canvas). I wouldn't expect your patch to slow it down. Last I checked pdf.js didn't have anything that waited for the load event, but maybe it's causing something else to be delayed in Firefox (fonts?).

Comment 4

[Jonas Jenwald [:Snuffleupagus]]

Looking at https://searchfox.org/mozilla-central/source/testing/talos/talos/pageloader/chrome/lh_pdfpaint.js, which I assume is the relevant test, we're waiting for the "load" before running the test. Given that we're now purposely delaying that event until all PDF pages have loaded, the regression thus seem somewhat expected as far as I can tell.

The question is if we actually need to wait for the "load" event at all, or if we could use e.g. DOMContentLoaded or some other event instead?

Comment 5

[Jonas Jenwald [:Snuffleupagus]]

Actually, is it even possible that waiting for the "load" event (which now fires later) means that we're actually missing the first PDF.js "pagerendered" event in this test and instead get the one for e.g. the second page?

Comment 6

[Brendan Dahl [:bdahl]]

(In reply to Jonas Jenwald [:Snuffleupagus] from comment #5)

Actually, is it even possible that waiting for the "load" event (which now fires later) means that we're actually missing the first PDF.js "pagerendered" event in this test and instead get the one for e.g. the second page?

That could definitely be it. I'll try it out locally. IIRC I had to wait for load because the window wasn't the right one when first running that file.

Comment 7

[Brendan Dahl [:bdahl]]

Yeah, that's what's happening we get pagerendered for page 2 now.

Comment 8

[Brendan Dahl [:bdahl]]

Attachment: Bug 1692140 - Fix talos test pdfpaint measurement. r=emilio

Now that we delay the load event for pdf.js, the pagerendered event listener
was being added too late and we were catching the second page rendered event.
Also, add a check to ensure the page rendered is the first page.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df4d5615fdb9
Fix talos test pdfpaint measurement. r=emilio,perftest-reviewers

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/df4d5615fdb9

Comment 11

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #28802 (as of Mon, 15 Feb 2021 07:29:24 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
30%	pdfpaint		windows10-64-shippable-qr	e10s stylo webrender-sw	549.88 -> 385.49
25%	pdfpaint		linux64-shippable-qr	e10s stylo webrender	515.34 -> 384.96
25%	pdfpaint		windows10-64-shippable	e10s stylo	556.48 -> 418.64
25%	pdfpaint		linux64-shippable-qr	e10s stylo webrender-sw	507.03 -> 381.63
23%	pdfpaint		windows10-64-shippable-qr	e10s stylo webrender	531.56 -> 410.32
22%	pdfpaint		linux64-shippable	e10s stylo	539.30 -> 419.15

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=28802