Bug 184143 – [CoordGraphics] Avoid painting backing stores for zero-opacity layers

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Wouter Vanhauwaert]

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?

[Zan Dobersek]

Created attachment 337871 [details]
Massif profile

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 338317 [details]
WIP -- Massif profile

[Zan Dobersek]

Created attachment 338574 [details]
Patch

[Zan Dobersek]

Comment on attachment 338574 [details]
Patch

Clearing flags on attachment: 338574

Committed r230950: <https://trac.webkit.org/changeset/230950>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Reopening due to rollout in bug #186206

[Wouter Vanhauwaert]

Any idea when this issue could be closed again?

[Zan Dobersek]

I'll be readdressing this after majority of work in bug #187385 is finished.

[Wouter Vanhauwaert]

Ok, in the meantime, is there another way to avoid this behaviour?
I mean hiding animated layers without (mis)-using the opacity property?

[Zan Dobersek]

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

[Wouter Vanhauwaert]

Any update?

[Jonas]

Any update on this issue ?

[Miguel Gomez]

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.

[Miguel Gomez]

Created attachment 381812 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 381812 [details]
Patch

Clearing flags on attachment: 381812

Committed r251837: <https://trac.webkit.org/changeset/251837>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Wouter Vanhauwaert]

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.

[Miguel Gomez]

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.

[Wouter Vanhauwaert]

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...

[Wouter Vanhauwaert]

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?

[Noam Rosenthal]

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.

[Noam Rosenthal]

(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

[Miguel Gomez]

Created attachment 418635 [details]
Patch

[Miguel Gomez]

(In reply to Miguel Gomez from comment #25)
> Created attachment 418635 [details]
> Patch

The layers that are invisible have a filter with opacity 0. When this was fixed before, those layers would have 0 opacity and would be skipped, but at some point this changed and the layers stopped having opacity 0. What they have instead is a filter with opacity 0. This caused the previous patch to stop working (they were checking for opacity 0 and not for filters with opacity 0).

I've created a patch that covers both cases: checks whether the layer has 0 opacity and whether there's a filter that will apply an opacity 0 operation. In those cases, the backingStores are not created. I've measured the memory consumption peak on 28MB with this patch.

Then, the curious thing: the minion, which I was only able to see in chromium, is there with this patch! I investigated a bit and it's visible if we disable AC mode as well (without this patch). This makes me think that we have a problem when applying filters on AC mode, and for some reason the layers that should be transparent are being painted anyway, covering the minion image.

[Carlos Garcia Campos]

Comment on attachment 418635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418635&action=review

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1353
> +    // Optimize memory usage by not creating BackingStores for layers that are invisible.
> +    bool isInvisibleBecauseOpacityZero = false;
> +
> +    // If the CSS opacity value is 0 and there's no animation over the opacity property, the layer is invisible.
> +    isInvisibleBecauseOpacityZero = !opacity() && !m_animations.hasActiveAnimationsOfType(AnimatedPropertyOpacity);

I guess this could be merged.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1357
> +    for (auto& operation : filters().operations()) {

You could use Vector::findMatching().

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1359
> +            auto& o = downcast<BasicComponentTransferFilterOperation>(*operation);

Don't use o for the name, use something like filterOperation, for example.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1360
> +            if (o.amount() == 0.0) {

Don't compare to 0 use ! instead

[Miguel Gomez]

Created attachment 418842 [details]
Patch

[EWS]

Committed r272141: <https://trac.webkit.org/changeset/272141>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418842 [details].