Performance regression while scrolling in long pdf documents (pdf.js)

RESOLVED FIXED in Firefox 65

Status

()

P2
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: philipp, Assigned: nical)

Tracking

(Blocks: 1 bug, {perf, regression})

65 Branch
mozilla65
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 unaffected, firefox65 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
str on current nightly (win64 with nvidia gpu, qualified for webrender):
- open a long pdf document like http://www.ignaciouriarte.com/works/18/pdfs/A100page47.pdf in the firefox pdf preview
- scroll down quickly
- the page content is starting to appear and react very choppy the further down you scroll in the document

this is a performance profile captured with the steps above: http://bit.ly/2TS9Zsy

running mozregression on the issue ends up on a changelog with bug 1494403 & bug 1509495 in it: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3b50a5b394b9467ba99fa0e956b53c49b66f3238&tochange=df30b0f614c904678b72d66616a65b989f9a87e9
Flags: needinfo?(kats)
Flags: needinfo?(kats) → needinfo?(nical.bugzilla)
(In reply to [:philipp] from comment #0)
> this is a performance profile captured with the steps above:
> http://bit.ly/2TS9Zsy

Please modify the profiler threads list to include "Wr,Render" in the list as well, and get a new profile with that. Thanks!
(Reporter)

Comment 2

4 months ago
that's a profile i've captured with the additional threads and the same steps: http://bit.ly/2TUwRaT
(Assignee)

Comment 3

4 months ago
We are spending a lot of time flushing commands before clearing textures and these clears are on newly allocated textures. The time is actually spent during allocation.
For some obscure reason one of my changes appears to put us in a situation where we never discard the texture cache entries for the blob image in that test case and things are getting slow when we run out of gpu memory and the driver starts flushing texture memory out.
Assignee: nobody → nical.bugzilla
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 4

4 months ago
Ok figured it out. WebRenderImageData's destructor calls ClearImageKey which schedules the non-blob image key for deletion if any. WebRenderFallbackData which inherits from it overloads ClearImageKey to take care of the blob image key but since it's called in a destructor we don't end up calling the overloaded version from the child class and the blob is leaked.

I wish we had a warning (treated as error) preventing us from calling overloaded virtual functions in destructors.

Fix coming soon, I'm pretty sure it will also fix bug 1510544.

Comment 6

4 months ago
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/848f4907fb07
Fix a blob image key leak. r=jrmuizel
Backed out changeset 848f4907fb07 (Bug 1510447) for failures on 136/289480.html

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.10%2Cquantumrender%2Cdebug%2Creftests%2Cwith%2Ce10s%2Ctest-macosx64-qr%2Fdebug-reftest-e10s-1%2Cr-e10s%28r1%29&tochange=11828ed488177ec28bfb343c64b466785600fab6&fromchange=1e214baf8fc1908b52fe9d19549cb4e72cfe42b4

Backout link: https://hg.mozilla.org/integration/autoland/rev/1855e3f4af3458c82f6e6b87e50fa2fc0cb05fce

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214449291&repo=autoland&lineNumber=41672

12:00:01     INFO - REFTEST TEST-LOAD | http://localhost:52813/1543435152864/1/289480-ref.html | 304 / 2052 (14%)
12:00:01     INFO - ++DOMWINDOW == 100 (0x131505000) [pid = 1007] [serial = 826] [outer = 0x11cca3000]
12:00:02     INFO - REFTEST TEST-UNEXPECTED-FAIL | http://localhost:50482/1543434643134/136/289480.html#top == http://localhost:50482/1543434643134/136/289480-ref.html | image comparison, max difference: 2, number of differing pixels: 471
12:00:02     INFO - REFTEST   IMAGE 1 (TEST): data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAyAAAAPoCAYAAAAmy5qxAAAgAElEQVR4nO3dT
12:00:02     INFO - REFTEST   IMAGE 2 (REFERENCE): data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAyAAAAPoCAYAAAAm
12:00:02     INFO - REFTEST INFO | Saved log: START http://localhost:52813/1543435152864/1/289480.html#top
12:00:02     INFO - REFTEST INFO | Saved log: [CONTENT] OnDocumentLoad triggering AfterOnLoadScripts
12:00:02     INFO - REFTEST INFO | Saved log: Initializing canvas snapshot
12:00:02     INFO - REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
12:00:02     INFO - REFTEST INFO | Saved log: [CONTENT] RecordResult fired
12:00:02     INFO - REFTEST INFO | Saved log: RecordResult fired
12:00:02     INFO - REFTEST INFO | Saved log: START http://localhost:52813/1543435152864/1/289480-ref.html
12:00:02     INFO - REFTEST INFO | Saved log: [CONTENT] OnDocumentLoad triggering AfterOnLoadScripts
12:00:02     INFO - REFTEST INFO | Saved log: Initializing canvas snapshot
12:00:02     INFO - REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
12:00:02     INFO - REFTEST INFO | Saved log: [CONTENT] RecordResult fired
12:00:02     INFO - REFTEST INFO | Saved log: RecordResult fired
12:00:02     INFO - REFTEST TEST-END | http://localhost:50482/1543434643134/136/289480.html#top == http://localhost:50482/1543434643134/136/289480-ref.html
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 8

4 months ago
It makes absolutely no sense to me that not leaking something changes the rendering of this test but that's fine, this test was fuzzy before and somehow the patch that caused the regression I'm fixing here removed the fuzziness and I'll add it back.
Flags: needinfo?(nical.bugzilla)
Blocks: 1386669
Priority: -- → P2

Comment 11

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5593c533a22d
https://hg.mozilla.org/mozilla-central/rev/6e6108966d1c
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox65: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.