Bug 184143 - [CoordGraphics] Avoid painting backing stores for zero-opacity layers
Summary: [CoordGraphics] Avoid painting backing stores for zero-opacity layers
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Local Build
Hardware: Other Linux
: P2 Major
Assignee: Zan Dobersek
URL:
Keywords:
Depends on: 184917 187385
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-29 12:03 PDT by Wouter Vanhauwaert
Modified: 2020-06-17 09:27 PDT (History)
18 users (show)

See Also:


Attachments
Massif profile (638.16 KB, text/plain)
2018-04-13 00:25 PDT, Zan Dobersek
no flags Details
Layer tree (36.87 KB, text/plain)
2018-04-13 01:30 PDT, Zan Dobersek
no flags Details
WIP (1.52 KB, patch)
2018-04-19 00:29 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP -- Massif profile (568.68 KB, text/plain)
2018-04-19 00:30 PDT, Zan Dobersek
no flags Details
Patch (4.42 KB, patch)
2018-04-23 01:38 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2019-10-24 08:27 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Layer Tree (2020-06-17) (36.46 KB, text/plain)
2020-06-17 07:13 PDT, Miguel Gomez
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wouter Vanhauwaert 2018-03-29 12:03:18 PDT
If I try to run following webpage on my NXP imx6-platform with 1GB, whether on wayland backend or on viv_imx6-backend, WebProcess gives me a huge memory drop (till 0MB free) until it crashes. This happens on the WPEWebkit fork, as on the Webkit 2.20 WPE platform. Sometimes it works, and then the memory gets back to normal. So it looks that only during build-up, memory drops. On https://pastebin.com/jaLFZFMp, you'll find a dump of the memory behaviour of the webprocess.
Demo page where I have the behaviour: http://woutervh.html-5.me The page is normally generated by angular, which explains the huge domtree. (looks ugly, I know). 
What is happening here?
Comment 1 Zan Dobersek 2018-04-13 00:25:28 PDT
Created attachment 337871 [details]
Massif profile
Comment 2 Zan Dobersek 2018-04-13 01:30:40 PDT
Created attachment 337874 [details]
Layer tree

TL;DR: 90 layers that draw content, most of them around 1920x1080px in size. Graphics pipeline wants to update their content, but is forced to do it inefficiently enough that all the necessary memory is allocated in one go.

That's the ~510MB allocated under Nicosia::Buffer::Buffer() in the #20 snapshot in the Massif profile output.
Comment 3 Zan Dobersek 2018-04-19 00:29:27 PDT
Created attachment 338316 [details]
WIP

Avoids producing backing stores for layers with 0 opacity. Still doesn't take into account potential animations of the opacity attribute.
Comment 4 Zan Dobersek 2018-04-19 00:30:19 PDT
Created attachment 338317 [details]
WIP -- Massif profile
Comment 5 Zan Dobersek 2018-04-23 01:38:27 PDT
Created attachment 338574 [details]
Patch
Comment 6 Zan Dobersek 2018-04-24 01:15:44 PDT
Comment on attachment 338574 [details]
Patch

Clearing flags on attachment: 338574

Committed r230950: <https://trac.webkit.org/changeset/230950>
Comment 7 Zan Dobersek 2018-04-24 01:15:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Michael Catanzaro 2018-06-22 08:44:33 PDT
Reopening due to rollout in bug #186206
Comment 9 Wouter Vanhauwaert 2018-08-16 00:30:36 PDT
Any idea when this issue could be closed again?
Comment 10 Zan Dobersek 2018-08-17 03:44:23 PDT
I'll be readdressing this after majority of work in bug #187385 is finished.
Comment 11 Wouter Vanhauwaert 2018-09-03 05:50:03 PDT
Ok, in the meantime, is there another way to avoid this behaviour?
I mean hiding animated layers without (mis)-using the opacity property?
Comment 12 Zan Dobersek 2018-09-06 03:02:25 PDT
(In reply to Wouter Vanhauwaert from comment #11)
> Ok, in the meantime, is there another way to avoid this behaviour?
> I mean hiding animated layers without (mis)-using the opacity property?

I think `display: none;` would achieve the hiding effect.
Comment 13 Wouter Vanhauwaert 2019-03-27 05:18:43 PDT
Any update?
Comment 14 Jonas 2019-07-05 06:26:24 PDT
Any update on this issue ?
Comment 15 Miguel Gomez 2019-10-24 08:14:40 PDT
The original patch was causing a crash in certain conditions:

- A layer with 0 opacity and an opacity animation would be set. CoordinatedGraphicsLayer::shouldHaveBackingStore() would return true because of the animation. The Nicosia::BackingStore is created and passed to the CoordinatedGraphicsScene
- The CoordinatedGraphicsScene creates the appropriate CoordinatedBackingStore and the animation goes on
- A layerFlush happens almost at the end of the animation, the CoordinatedGraphicsLayer still sees the animation active so it keeps the backingStore, but when the same check is performed by the CoordinatedGraphicsScene, the animation is detected as over, which causes the scene to destroy the layer's CoordinatedBackingStore.
- A new layer flush changes the motive to keep the layer (maybe opacity is not 0 not), so the backingStore is kept. When propagating this to the CoordinatedGraphicsScene, a new CoordinatedBackingStore is created, but its tiles are not in sync with the tiles of the CoordinatedGraphicsLayer's backing store. Due to this, a request to update an existing tile causes a crash, as it doesn't exist in the new CoordinatedBackingStore.

What I think is that the check added by the patch in CoordinatedGraphicsLayer::shouldHaveBackingStore() is ok, but we should not do the same check inside the CoordinatedGraphicsScene to delete a CoordinatedBackingStore. This check is not required. The creation and destruction of backingStores is handled by the CoordinatedGraphicsLayer, and whatever is decided is propagated to the CoordinatedGraphicsScene. In the case mentioned before, the scene wouldn't destroy any backingStore because the CoordinatedGraphicsLayer didn't do it either. When the CoordinatedGraphicsLayer decides that the backingStore is not needed anymore, it will destroy its Nicosia::BackingStore, which when taken through the state to the CoordinatedGraphicsScene, will destroy the related assets and clear the CoordinatedBackingStore that was assigned to the TextureMapperLayer.

PD: as I'm testing this, there seems to be some rendering issue with the test page http://woutervh.html-5.me cause I can't see the minion on wpe that I see on chrome, and also the memory usage without the patch is much smaller. But this happens already without the patch to this issue, so it has to be previous change.
Comment 16 Miguel Gomez 2019-10-24 08:27:10 PDT
Created attachment 381812 [details]
Patch
Comment 17 WebKit Commit Bot 2019-10-31 02:31:21 PDT
Comment on attachment 381812 [details]
Patch

Clearing flags on attachment: 381812

Committed r251837: <https://trac.webkit.org/changeset/251837>
Comment 18 WebKit Commit Bot 2019-10-31 02:31:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Wouter Vanhauwaert 2020-06-17 03:22:03 PDT
This one is not fixed at all..
I just tested the minion on cog, using wpewebkit 2.28.2 (which should have said patch), and it's just crashing as before (10.0.0.254 is the minion page on my pc, as I don't have direct internet access):

Cog-Message: 13:40:02.488: <http://10.0.0.254/> Load started.
Cog-Message: 13:40:02.545: <http://10.0.0.254/> Loading...
Cog-Message: 13:40:03.542: <http://10.0.0.254/> Loaded successfully.
WARNING: trying to load platform resource 'missingImage'
Memory pressure relief: Total: res = 691519488/692174848/655360, res+swap = 729772032/730554368/782336
Memory pressure relief: Total: res = 5689344/5718016/28672, res+swap = 14016512/7032832/-6983680
Memory pressure relief: Total: res = 607207424/607440896/233472, res+swap = 776130560/805498880/29368320
(tft:646): Cog-WARNING **: 13:40:16.902: <http://10.0.0.254/> Crash!: The renderer process crashed. Reloading the page may fix intermittent failures.
Comment 20 Miguel Gomez 2020-06-17 07:13:32 PDT
Created attachment 402104 [details]
Layer Tree (2020-06-17)

This is the current layer tree of the page. It's not the same as the one Zan added in comment 2. There are still a ton of 1920x1080 layers as before, but now their opacity is not zero. This means that the optimization we added so those layers won't consume memory is useless now. All of them will get a backingStore, requiring 8MB of memory each.
Comment 21 Wouter Vanhauwaert 2020-06-17 09:13:54 PDT
The page hasn't been touched since I've put it there. (To be honest, I don't remember the credentials even). So it would be very odd that the layer tree is any different...
Comment 22 Wouter Vanhauwaert 2020-06-17 09:19:24 PDT
Also you mention the page is not showing up on your side either, where it does show up in chrome and previous wpewebkits. Wouldn't that mean that something else is broken, leading to this missing opacity 0?
Comment 23 Noam Rosenthal 2020-06-17 09:23:46 PDT
When we started coordinated graphics (~2011) for the N9 phone, we had many cases in the wild where backing stores would cumulatively take too much memory. The thing that ended up working was to simply turn off accelerated compositing for the page if the required total backing store space was too large - at some point it makes more sense to draw everything in every frame than to spend tons of memory on backing stores.

My nostalgic 2cents, hope it helps.
Comment 24 Noam Rosenthal 2020-06-17 09:27:08 PDT
(In reply to Noam Rosenthal from comment #23)
> When we started coordinated graphics (~2011) for the N9 phone, we had many
> cases in the wild where backing stores would cumulatively take too much
> memory. The thing that ended up working was to simply turn off accelerated
> compositing for the page if the required total backing store space was too
> large - at some point it makes more sense to draw everything in every frame
> than to spend tons of memory on backing stores.
> 
> My nostalgic 2cents, hope it helps.

... oh and my plan for it long-term was to make more and more stuff "directly composited" so that it wouldn't need a backing store in the first place, kind of like what we did here: https://trac.webkit.org/changeset/148172/webkit