Bug 184143

Created attachment 337871 [details] Massif profile

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.

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.

Created attachment 338317 [details] WIP -- Massif profile

Created attachment 338574 [details] Patch

Comment on attachment 338574 [details] Patch Clearing flags on attachment: 338574 Committed r230950: <https://trac.webkit.org/changeset/230950>

All reviewed patches have been landed. Closing bug.

Reopening due to rollout in bug #186206

Any idea when this issue could be closed again?

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

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

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

Any update?

Any update on this issue ?

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.

Created attachment 381812 [details] Patch

Comment on attachment 381812 [details] Patch Clearing flags on attachment: 381812 Committed r251837: <https://trac.webkit.org/changeset/251837>

All reviewed patches have been landed. Closing bug.

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.

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.

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

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?

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.

(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

Created attachment 418635 [details] Patch

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

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

Created attachment 418842 [details] Patch

Committed r272141: <https://trac.webkit.org/changeset/272141> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418842 [details].