Bug 75637 – [Chromium] Protect visible textures before updating them

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[Xianzhu Wang]

Currently, tile textures are reserved (requested for new ones or protected for existing ones) in TiledLayerChromium::prepareToUpdateTiles(). If a new tile causes the memory to exceed the preferred memory limit, a least used texture will be replaced. However, sometimes the texture being replaced will be used soon. (In fact, at the time, the least used texture is more likely to be used soon.)

For example, a layer has the following tiles to be drawn in the current cycle:
1 2 3 4
5 6 7 8
Tiles 1 to 4 don't have textures, and 5 to 8 have.
And the layer also have tiles 9 to 12 which have textures but are no longer visible.

Assume the preferred memory limit can contain 6 textures.

TextureManager will do the following for the tiles:
1: replaceTexture(5 => 1)
2: replaceTexture(6 => 2)
3: replaceTexture(7 => 3)
4: replaceTexture(8 => 4)
5: replaceTexture(9 => 5)
6: replaceTexture(10 => 6)
7: replaceTexture(11 => 7)
8: replaceTexture(12 => 8)

The textures of tiles 5 to 8 are replaced and then recreated and repainted.

If we protect all the visible tiles (5 to 8) of all layers before CCLayerTreeHost::paintLayerContents(), TextureManager will do the following:
1: replaceTexture(9 => 1)
2: replaceTexture(10 => 2)
3: replaceTexture(11 => 3)
4: replaceTexture(12 => 4)
5: do nothing
6: do nothing
7: do nothing
8: do nothing

[Grace Kloba]

Currently replaceTexture is based on m_preferredMemoryLimitBytes. If we use the max(m_preferredMemoryLimitBytes, currentlyProtectedByes) and currentlyProtectedByes is set after each call to reduceMemoryToLimit(), will this help?

[Xianzhu Wang]

(In reply to comment #1)
> Currently replaceTexture is based on m_preferredMemoryLimitBytes. If we use the max(m_preferredMemoryLimitBytes, currentlyProtectedByes) and currentlyProtectedByes is set after each call to reduceMemoryToLimit(), will this help?

If I understood your method correct, using my example to simulate shows it doesn't work well: as used = 8 textures and max(preferred, protected) = 8 textures, any request of new texture will still trigger replaceTexture, so what happens is just the same as the current code. If we further raise the threshold of replaceTexture, remove/add will happen which costs even more than replaceTexture.

I prefer doing explicit protection before updating the layers. With it, the textures that are no longer visible are not protected and will be replaced or removed when needed, and visible textures won't be replaced.

[James Robinson]

(In reply to comment #2)
> (In reply to comment #1)
> > Currently replaceTexture is based on m_preferredMemoryLimitBytes. If we use the max(m_preferredMemoryLimitBytes, currentlyProtectedByes) and currentlyProtectedByes is set after each call to reduceMemoryToLimit(), will this help?
> 
> If I understood your method correct, using my example to simulate shows it doesn't work well: as used = 8 textures and max(preferred, protected) = 8 textures, any request of new texture will still trigger replaceTexture, so what happens is just the same as the current code. If we further raise the threshold of replaceTexture, remove/add will happen which costs even more than replaceTexture.
> 
> I prefer doing explicit protection before updating the layers. With it, the textures that are no longer visible are not protected and will be replaced or removed when needed, and visible textures won't be replaced.

I think you're right.  The intent with the preferred/max limits is that currently visible textures should always get priority and kick out anything below the preferred limit if there's contention, and that we should only keep non-visible textures around if the total use is under the preferred limit.

[Xianzhu Wang]

Created attachment 126192 [details]
patch

[Dana Jansens]

Comment on attachment 126192 [details]
patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:460
> +        if (it.representsTargetRenderSurface() || it->alwaysReserveTextures())

I think you want !it.representsItself()

Right now you protect both in the representsContributingSurface() and representsItself() case. A layer that owns a surface and draws will be protected twice. You care only to catch layers that draw something IIUC so you just want representsItself().

[Xianzhu Wang]

Comment on attachment 126192 [details]
patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:460
>> +        if (it.representsTargetRenderSurface() || it->alwaysReserveTextures())
> 
> I think you want !it.representsItself()
> 
> Right now you protect both in the representsContributingSurface() and representsItself() case. A layer that owns a surface and draws will be protected twice. You care only to catch layers that draw something IIUC so you just want representsItself().

Does the code in CCLayerTreeHost::reserveTextures() (where I copied some code :) have the same problem?

[Dana Jansens]

If I am understanding correctly, yes it does.

@enne does a contributing layer need to reserve textures if it doesn't draw?

[Vangelis Kokkevis]

Comment on attachment 126192 [details]
patch

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

The texture recycling patch was reverted yesterday:
http://trac.webkit.org/changeset/107014

as it was causing a lot of unnecessary repaints. Your patch will need to first re-enable texture recycling.  But before we do that we should make sure that things work as expected and we don't end up churning through textures again.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:460
>>> +        if (it.representsTargetRenderSurface() || it->alwaysReserveTextures())
>> 
>> I think you want !it.representsItself()
>> 
>> Right now you protect both in the representsContributingSurface() and representsItself() case. A layer that owns a surface and draws will be protected twice. You care only to catch layers that draw something IIUC so you just want representsItself().
> 
> Does the code in CCLayerTreeHost::reserveTextures() (where I copied some code :) have the same problem?

I don't think it will be protected twice as regardless of whether the layer has a RS attached to it or not, it should only appear once in the list.

[Dana Jansens]

(In reply to comment #8)
> I don't think it will be protected twice as regardless of whether the layer has a RS attached to it or not, it should only appear once in the list.

If a layer draws and owns a surface also, it will be in the list for the RenderSurface it owns, and in the list for the RenderSurface it contributes to. The iterator class will visit the layer in both places, with the "representsX" flag to differentiate (also a third time for visiting the the layer's render surface as a target but that's already skipped).

[Vangelis Kokkevis]

(In reply to comment #9)
> (In reply to comment #8)
> > I don't think it will be protected twice as regardless of whether the layer has a RS attached to it or not, it should only appear once in the list.
> 
> If a layer draws and owns a surface also, it will be in the list for the RenderSurface it owns, and in the list for the RenderSurface it contributes to. The iterator class will visit the layer in both places, with the "representsX" flag to differentiate (also a third time for visiting the the layer's render surface as a target but that's already skipped).

Hmm, the way the code is setup, if a layer owns a RenderSurface, it also contributes to it.  See:

http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp&exact_package=chromium&q=layertreehostcommon&type=cs&l=320

So it will only be visited twice, once in the list of the RS it owns and once in the list of its ancestor's RS.  If we bail out for it.representsTargetRenderSurface() we should only be protecting it exactly once.

[Dana Jansens]

(In reply to comment #10)
> Hmm, the way the code is setup, if a layer owns a RenderSurface, it also contributes to it.  See:
> 
> http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp&exact_package=chromium&q=layertreehostcommon&type=cs&l=320
> 
> So it will only be visited twice, once in the list of the RS it owns and once in the list of its ancestor's RS.  If we bail out for it.representsTargetRenderSurface() we should only be protecting it exactly once.

Sorry this is derailing things a bit Xianzhu, I guess it's worth getting things right. Vangelis, you're absolutely right in your understanding of the renderSurfaceLayerList structure. What's missing is how the iterators run over this structure.

http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/tests/CCLayerIteratorTest.cpp&type=cs&l=259

This layer is hit once in each of the three states during a back-to-front iteration. The iterators do this so that you can easily act differently on targets, contributing surfaces, and drawing surfaces. Most uses of the iterators only care to act on layers when representsItself(), while maybe doing something wrt the surfaces when representsContributingRenderSurface() or representsTargetRenderSurface().

[Xianzhu Wang]

(In reply to comment #11)
> Sorry this is derailing things a bit Xianzhu, I guess it's worth getting things right. Vangelis, you're absolutely right in your understanding of the renderSurfaceLayerList structure. What's missing is how the iterators run over this structure.
> 

No problem. In deed I learned from the discussion :)

> The texture recycling patch was reverted yesterday:
> http://trac.webkit.org/changeset/107014
> 
> as it was causing a lot of unnecessary repaints. Your patch will need to first re-enable texture recycling.  But before we do that we should make sure that things work as expected and we don't end up churning through textures again.
> 

I noticed the revert but I thought the problem remained with only replace/recreate changed to remove/add. Now I've realized that they are different at the memory limits, so this bug no longer exists.

Will file another bug about TiledLayerChromium::protectVisibleTileTextures().

[Adrienne Walker]

(In reply to comment #5)
> (From update of attachment 126192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126192&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:460
> > +        if (it.representsTargetRenderSurface() || it->alwaysReserveTextures())
> 
> I think you want !it.representsItself()

You're quite right.  Here's https://bugs.webkit.org/show_bug.cgi?id=78258 to fix the reserveTextures part of that.