1499908 - Standalone texture cache entries for rasterized blobs persist indefinitely

Status: VERIFIED FIXED

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

Description

[Bobby Holley (:bholley)]

STR:
(1) Enable gfx.webrender.debug.texture-cache
(2) Visit https://fivethirtyeight.com/politics/
(3) Scroll down past the various SVG charts on the right
(4) You should see the various charts appear as standalone textures
(5) Close the tab, continue browsing. The textures remain indefinitely.

Glenn thinks this is because of the manual eviction policy [1] for rasterized blobs, which presumably isn't working correctly.

[1] https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/gfx/webrender/src/resource_cache.rs#1574

Comment 1

[Nicolas Silva [:nical]]

In theory all images and their texture cache entries should get evicted when you close the tab if not that's worrying, it would suggest that the image key isn't removed upon closing the tab.

Some background about the manual eviction policy: the initial problem is that blobs are rasterized asynchronously during scene building so what could happen is:

 - a blob entry is in the cache
 - an update arrives that repaints half of the blob
 - asynchronously we start painting that half-blob
 - scrolling happens and the entry gets evicted from the cache
 - rasterization completes,
 - the render backend receives half of the blob's rasterized image and doesn't have an entry to update anymore.

We could allow evicting a blob image's texture cache entry if it isn't referenced anymore by the display list (so you'd need to scroll way past it but it could still be evicted).
But first we need to figure out if/why the image key isn't deleted when we close the tab.

Comment 2

[Bobby Holley (:bholley)]

Ok, NI nical to have a look at this next week. High priority since it's a potential leak on beta. Let me know if you can't repro.

Comment 3

[Nicolas Silva [:nical]]

So far I haven't been able to reproduce the same issue. I can observe two scenarios:

# If the page isn't zoomed (not a hidpi screen)

- Blobs all go into shared entries.
- We don't collect shared entries very often (from a look of the code we seem to collect them when running out of space in the shared cache), after scrolling the provided link I scrolled wikipedia's front page for ~30 seconds and expire_old_shared_entries was never called.

# If I zoom enough for the graphs to be standalone entries (probably better matches Bobby's configuration).

- the standalone entries get evicted quite aggressively (they seem to get evicted more or less as soon as they are off-screen).


Either way, if I go to about:memory and minimize the memory usage, everything gets purged out of the cache.


It wouldn't hurt to evict shared entries a bit more aggressively. If anything, evicting entries that were marked as unused when their image key was deleted would probably help newer allocation fill space at the beginning of the storage and make it easier  to shrink back after an allocation spike.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Bug 1499908 - Don't bypass cleanup logic in clear_namespace().

Comment 5

[Bobby Holley (:bholley)]

Attachment: Bug 1499908 - Add assertions against dropping images from the ResourceCache that are set to manual eviction in the TextureCache.

Depends on D9952

Comment 6

[Bobby Holley (:bholley)]

Attachment: Bug 1499908 - Assert that depth targets are bound when depth features are used.

Dzmitry asked for this during review on another bug - submitting it as
a ridealong with this one.

Depends on D9953

Comment 7

[Bobby Holley (:bholley)]

Oh, and for the record: This can be reproduced more reliably by visiting [1], scrolling down past the ex-president graphs, and closing the tab.

[1] https://projects.fivethirtyeight.com/trump-approval-ratings/?ex_cid=rrpromo

Comment 8

[Bobby Holley (:bholley)]

Attachment: Bug 1499908 - Properly clean up cached images when clearing them on memory pressure.

Depends on D9953

Comment 9

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=315693f87d6ce9affbaa886ec6acbb97afd4249d

Comment 10

[Bobby Holley (:bholley)]

https://github.com/servo/webrender/pull/3234

Comment 11

[Bobby Holley (:bholley)]

Verified in Nightly built from https://hg.mozilla.org/mozilla-central/rev/f7a97b344fa59bd3b01ea81ebd5b150aa63bfb12

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 9020455 [details]
Bug 1499908 - Don't bypass cleanup logic in clear_namespace().

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Potential leak for qualified beta WR users. No impact in release, since WR won't ride the 64 release train.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Relatively low risk. I suggest baking for a few days on nightly and then approving for beta if we haven't seen any regressions. Please ping kats to handle the uplift mechanics.

String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9020455 [details]
Bug 1499908 - Don't bypass cleanup logic in clear_namespace().

[Triage Comment]
Fixes a possible WebRender leak for Beta users opted into the study. Approved for 64.0b6.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc7548ec328b