Bug 64771 – [chromium] Seperate texture management for contents textures vs render surface textures

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Seperate texture management for contents textures vs render surface textures

[James Robinson]

Created attachment 101253 [details]
Patch

[Kenneth Russell]

Comment on attachment 101253 [details]
Patch

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

This looks good overall. Couple of questions. I'd like to get an unofficial review from enne or vangelis before setting r+.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:219
> +    m_contentsTextureManager->reduceMemoryToLimit(textureMemoryLowLimitBytes);
> +    m_contentsTextureManager->unprotectAllTextures();
> +    // Evict all RenderSurface textures.
> +    m_renderSurfaceTextureManager->unprotectAllTextures();
> +    m_renderSurfaceTextureManager->reduceMemoryToLimit(0);

Why are these two methods called in different orders on the different TextureManager instances?

> Source/WebCore/platform/graphics/chromium/TextureManager.h:69
>      void reduceMemoryToLimit(size_t);

Consider unifying this API with the new setLimits() API?

[Adrienne Walker]

Comment on attachment 101253 [details]
Patch

Given that there's now a disconnect between the two texture pools, do you think it's a problem that we can now go over the hard texture limit? If you had a lot of render surfaces, the content texture pool wouldn't know about them and could allocate up to the hard limit.  In aggregate, it'd be over the hard limit temporarily changed the limits and removed those.  It seems like an unlikely edge case and one that could only happen on active tabs.  I just wanted to mention it, but am not convinced it's serious enough to try to address.  Besides, it's way more elegant to have two separate texture managers than try to have one texture manager that's trying to track two different kinds of resources.

In general, this unofficially looks good to me.

[Kenneth Russell]

Comment on attachment 101253 [details]
Patch

Marking r+ based on enne's unofficial review.

[James Robinson]

Thanks!  I'll give some thought to how to handle the hard limit and unifying with the setLimit() API before landing as it does seem that there's an opportunity for simplification here.

[Vangelis Kokkevis]

Comment on attachment 101253 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:322
> +        renderSurfaceLimits.reclaimLimit = textureMemoryReclaimLimitBytes - m_contentsTextureManager->currentMemoryUseBytes();

Not sure I get this part.  if textureMemoryReclaimLimitBytes < m_contentsTextureManager->currentMemoryUseBytes() then wouldn't textureMemoryReclaimLimitBytes - m_contentsTextureManager->currentMemoryUseBytes() be negative?   Did you mean to say > instead of < ?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:325
> +    m_renderSurfaceTextureManager->setLimits(renderSurfaceLimits);

Maybe cleaner to call reduceMemoryToLimit() here instead of calling setLimits() as the limits will be reset again before the next draw.

[James Robinson]

(In reply to comment #6)
> (From update of attachment 101253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101253&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:322
> > +        renderSurfaceLimits.reclaimLimit = textureMemoryReclaimLimitBytes - m_contentsTextureManager->currentMemoryUseBytes();
> 
> Not sure I get this part.  if textureMemoryReclaimLimitBytes < m_contentsTextureManager->currentMemoryUseBytes() then wouldn't textureMemoryReclaimLimitBytes - m_contentsTextureManager->currentMemoryUseBytes() be negative?   Did you mean to say > instead of < ?
> 
Whoops, I got that backwards.  Will fix.

I also think that it might make sense to remove the notion of two texture limits in TextureManager itself and go back to just one limit + reduceMemoryToLimit().  The texture management sequence would look like this:

(0) during compositor intialization, set contents TextureManager limit to textureMemoryHighLimitBytes (this never changes)

for every frame:
paint stuff:
(1) protect all contents textures needed for this frame
(2) paint
(3) call reduceMemoryToLimit(textureMemoryReclaimLimitBytes) on the contents TextureManager to evict any unprotected textures over the reclaim limit
(4) upload textures

draw stuff:
(5) set the render surface TextureManager limit to textureMemoryHighLimitBytes less the contents TextureManager's current memory use. if there's no room, abort the composite since we can't possibly render and stay under budget
(6) draw
(7) unprotect all contents textures
(8) unprotect all render surface textures
(9) call reduceMemoryToLimit(textureMemoryReclaimLimitBytes) on the contents TextureManager
(10) call reduceMemoryToLimit(textureMemoryReclaimLimitBytes - contents TextureManager current memory use) on the render surface TextureManager

with the eventual goal that "paint stuff" happens without any specific interaction with the compositor thread (except for the uploads themselves), and "draw stuff" happens on the compositor thread.  The main difference in behavior between this and the existing code is that it more aggressively favors contents textures over render surface textures.  If the amount of textures used by the page is very close to the reclaim limit, this logic will keep as many contents textures as will fit and will evict all render surface textures.  I think that's a good tradeoff to make.

Patch on the way.

[James Robinson]

Created attachment 101546 [details]
Patch

[James Robinson]

Created attachment 101615 [details]
fix compile error in debug (oops)

[Vangelis Kokkevis]

Comment on attachment 101615 [details]
fix compile error in debug (oops)

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

I like where this is going.  As an aside, I'm curious how the texture manager will function in the threaded compositor world.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:314
> +    m_renderSurfaceTextureManager->unprotectAllTextures();

Render surface textures get unprotected after they're used (at the top of this function) so unprotecting them here has no effect.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:315
> +    m_renderSurfaceTextureManager->reduceMemoryToLimit(textureMemoryReclaimLimitBytes - m_contentsTextureManager->currentMemoryUseBytes());

reduceMemoryToLimit won't deal well with negative values.  You either need to fix this here or in the implementation of the method.

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:34
> +using namespace std;

I don't think the include or the "using" lines are needed based on the changes I see.

[James Robinson]

(In reply to comment #10)
> (From update of attachment 101615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101615&action=review
> 
> I like where this is going.  As an aside, I'm curious how the texture manager will function in the threaded compositor world.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:314
> > +    m_renderSurfaceTextureManager->unprotectAllTextures();
> 
> Render surface textures get unprotected after they're used (at the top of this function) so unprotecting them here has no effect.

Right, I forgot that we did render surface ones this way.  Call removed.

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:315
> > +    m_renderSurfaceTextureManager->reduceMemoryToLimit(textureMemoryReclaimLimitBytes - m_contentsTextureManager->currentMemoryUseBytes());
> 
> reduceMemoryToLimit won't deal well with negative values.  You either need to fix this here or in the implementation of the method.

Actually it won't even take negative numbers, size_t is unsigned.  Fixed here.

> 
> > Source/WebCore/platform/graphics/chromium/TextureManager.cpp:34
> > +using namespace std;
> 
> I don't think the include or the "using" lines are needed based on the changes I see.

Nope, they were for a previous iteration.  Removed.

[James Robinson]

Created attachment 101691 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 101691 [details]
Patch

(unofficial) LGTM from me.

[Kenneth Russell]

Comment on attachment 101691 [details]
Patch

Sounds good. r=me again.

[James Robinson]

Committed r91629: <http://trac.webkit.org/changeset/91629>