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

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[[:philipp]]

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

(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!

Comment 2

[[:philipp]]

that's a profile i've captured with the additional threads and the same steps: http://bit.ly/2TUwRaT

Comment 3

[Nicolas Silva [:nical]]

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.

Comment 4

[Nicolas Silva [:nical]]

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 5

[Nicolas Silva [:nical]]

Attachment: bug 1510447 - Fix a blob image key leak. r=jrmuizel

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/848f4907fb07
Fix a blob image key leak. r=jrmuizel

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 8

[Nicolas Silva [:nical]]

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.

Comment 9

[Nicolas Silva [:nical]]

Attachment: Bug 1510447 - Adjust reftest expectation. r=kats

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5593c533a22d
Adjust reftest expectation. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6108966d1c
Fix a blob image key leak. r=jrmuizel

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/5593c533a22d
https://hg.mozilla.org/mozilla-central/rev/6e6108966d1c