Bug 70623 – Ensure CCLayerTreeHost actually frees textures when hidden

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Iain Merrick]

Ensure CCLayerTreeHost actually frees textures when hidden

[Iain Merrick]

Created attachment 111988 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 111988 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:129
> +        contentsTextureManager()->reduceMemoryToLimit(TextureManager::lowLimitBytes());

Out of curiosity, what is it that would cause more textures to be created after setVisible(false) is called.  Do we just need to make sure that no drawing action happens while we're not visible?

[James Robinson]

Comment on attachment 111988 [details]
Patch

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

This seems iffy - we shouldn't be doing any more draws after going non-visible.  When are you seeing problems?

> Source/WebCore/ChangeLog:17
> +        No new tests. (OOPS!)

What's the test case where this fails?

[James Robinson]

Comment on attachment 111988 [details]
Patch

R- for the lack of tests (OOPS!) line in the changelog - the svn presubmit check will fail on that.  We also really should have tests.

[Iain Merrick]

(In reply to comment #2)
> (From update of attachment 111988 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111988&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:129
> > +        contentsTextureManager()->reduceMemoryToLimit(TextureManager::lowLimitBytes());
> 
> Out of curiosity, what is it that would cause more textures to be created after setVisible(false) is called.  Do we just need to make sure that no drawing action happens while we're not visible?

That's a good question. I just added some logging to TextureManager, and it wasn't releasing textures down to the 3MB limit as expected.

I'll investigate a bit further and add a test.

[Iain Merrick]

After some further experiments:

- We don't reduce down to 3MB unless the window is fairly small, because the root layer tiles are protected.

- The total size of the protected tiles varies, but not due to additional drawing as I thought, it's just because the number of visible tiles changes depending on the current scroll position. It's fairly common for ~10MB to be protected.

So it's actually working as intended. The 3MB limit doesn't seem particularly effective, though. Maybe we should increase that limit, but remove the special protection for root tiles?

[Adrienne Walker]

(In reply to comment #6)

> So it's actually working as intended. The 3MB limit doesn't seem particularly effective, though. Maybe we should increase that limit, but remove the special protection for root tiles?

The special protection for root tiles (potentially) makes tab switching faster because you don't have to repaint all of those tiles.  I know we already have slow tab switching issues, so I'd worry a little about making this worse.

I think somebody was looking at keeping around the fbo that we were drawing into.  If that got implemented, then I'd feel a lot more comfortable tossing all of the textures from a tab, because we'd have something to display while resources were recreated.

[Iain Merrick]

Wouldn't we get the same improvement from *any* visible tiles, though, not just root ones?

[Adrienne Walker]

(In reply to comment #8)
> Wouldn't we get the same improvement from *any* visible tiles, though, not just root ones?

Ok sure, so long as we were keeping *enough* textures.

[James Robinson]

I really think the solution here is to preserve the tab's contents (as final composited output) when switching away, and that it should be implemented at the application level, not down here in the compositor.

[Vangelis Kokkevis]

(In reply to comment #10)
> I really think the solution here is to preserve the tab's contents (as final composited output) when switching away, and that it should be implemented at the application level, not down here in the compositor.

There could be an short-term solution that copies the contents of the backbuffer to another texture right before the tab is hidden. We can then safely discard all other textures.  When the tab is first displayed, we can show that texture and immediately start working on an update.

[Vangelis Kokkevis]

(In reply to comment #11)
> (In reply to comment #10)
> > I really think the solution here is to preserve the tab's contents (as final composited output) when switching away, and that it should be implemented at the application level, not down here in the compositor.
> 
> There could be an short-term solution that copies the contents of the backbuffer to another texture right before the tab is hidden. We can then safely discard all other textures.  When the tab is first displayed, we can show that texture and immediately start working on an update.

What should we do with this patch?  We need something like this if we want to reduce our memory footprint for background tabs.

[Iain Merrick]

Sorry, I should have closed off this issue. I think this is a working as intended and this patch isn't needed.

Alternative things we might want to do (but in other bugs):
- Save off the tab contents for fast display (but I agree with James, I think that's an app-level thing)
- Maybe ditch or increase the 3MB limit (we pretty much always protect more than 3MB anyway).

We could continue to protect the root tiles, and just discard down to 0. Or maybe we could *not* protect the root tiles, and discard down to (say) 5MB of LRU textures, which should capture enough for a fairly fast repaint.