Bug 70352 – [chromium] Preserve offscreen tiles instead of immediately recycling them

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] Preserve offscreen tiles instead of immediately recycling them

[Adrienne Walker]

Created attachment 111493 [details]
Patch

[Adrienne Walker]

I went back and forth a lot about whether to move the texture recycling or just remove it, and ended up on the removal end after some discussions with gman and nduca.

Thoughts?

[Nat Duca]

Comment on attachment 111493 [details]
Patch

Yay for changes with more deletions than additions! This looks awesome.

[James Robinson]

Comment on attachment 111493 [details]
Patch

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

This looks great!

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-90
> -    m_unusedTiles.clear();

can you delete m_unusedTiles from the class definition in the .h as well? It seems unused.

[Alexandre Elias]

LGTM, I was about to make similar changes.  Although I think we will probably need recycle textures in the future, in the TextureManager; I'd be surprised if every driver reacted elegantly to the churn.

[Adrienne Walker]

(In reply to comment #5)
> LGTM, I was about to make similar changes.  Although I think we will probably need recycle textures in the future, in the TextureManager; I'd be surprised if every driver reacted elegantly to the churn.

Absolutely agreed that the TextureManager is a better place to do this recycling.  My preference is to keep the code simple for now and, when we have a good use case that demonstrates a performance problem, use that to tune the right pool size, rather than prematurely doing it now via guesswork.

[Adrienne Walker]

Committed r97809: <http://trac.webkit.org/changeset/97809>

[Vangelis Kokkevis]

(In reply to comment #7)
> Committed r97809: <http://trac.webkit.org/changeset/97809>

I like the cleanup but I think what we may see with this change is that our texture usage keep growing until we hit the texture reclaim limit. At that point, every time we cross a tile boundary we'll be deleting a row of texture tiles and allocating a new one. The good news is that the tiles we'll be deleting will likely be from a part of the page that's pretty far away from our current scroll position. The bad news is that we'll be churning (deleting and recreating) textures a lot.

Our texture manager is unfortunately not able to recycle the actual GL texture objects. Given that our tiles are all the same size it would be very convenient if it did.  Maybe that's a natural next step?

[Adrienne Walker]

(In reply to comment #8)
> (In reply to comment #7)
> > Committed r97809: <http://trac.webkit.org/changeset/97809>
> 
> I like the cleanup but I think what we may see with this change is that our texture usage keep growing until we hit the texture reclaim limit. At that point, every time we cross a tile boundary we'll be deleting a row of texture tiles and allocating a new one.

Yeah, having texture usage grow was my intention.  Rather than have the layer do this reclamation, this will let the texture manager handle it naturally using LRU.  If the active page has nothing else going on, I don't see any problem with texture usage sitting up at the soft limit, because it means that scrolling the entire page will be really fast.

> Our texture manager is unfortunately not able to recycle the actual GL texture objects. Given that our tiles are all the same size it would be very convenient if it did.  Maybe that's a natural next step?

I am not entirely convinced of how much of a problem this is.  We are already destroying and recreating tons of textures as layers are destroyed and created or as we switch tab focus.  I think there are other tasks that are a higher priority to take care of, like improving the responsiveness of long commits by interleaving input and drawing.