Bug 64771 - [chromium] Seperate texture management for contents textures vs render surface textures
Summary: [chromium] Seperate texture management for contents textures vs render surfac...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks: 64772
  Show dependency treegraph
 
Reported: 2011-07-18 18:12 PDT by James Robinson
Modified: 2011-07-22 18:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (11.50 KB, patch)
2011-07-18 18:22 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (13.78 KB, patch)
2011-07-20 19:29 PDT, James Robinson
no flags Details | Formatted Diff | Diff
fix compile error in debug (oops) (13.79 KB, patch)
2011-07-21 11:39 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (13.75 KB, patch)
2011-07-21 19:41 PDT, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-07-18 18:12:44 PDT
[chromium] Seperate texture management for contents textures vs render surface textures
Comment 1 James Robinson 2011-07-18 18:22:23 PDT
Created attachment 101253 [details]
Patch
Comment 2 Kenneth Russell 2011-07-19 12:29:07 PDT
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?
Comment 3 Adrienne Walker 2011-07-20 14:37:45 PDT
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.
Comment 4 Kenneth Russell 2011-07-20 15:04:33 PDT
Comment on attachment 101253 [details]
Patch

Marking r+ based on enne's unofficial review.
Comment 5 James Robinson 2011-07-20 15:19:39 PDT
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.
Comment 6 Vangelis Kokkevis 2011-07-20 15:52:51 PDT
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.
Comment 7 James Robinson 2011-07-20 18:56:58 PDT
(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.
Comment 8 James Robinson 2011-07-20 19:29:29 PDT
Created attachment 101546 [details]
Patch
Comment 9 James Robinson 2011-07-21 11:39:46 PDT
Created attachment 101615 [details]
fix compile error in debug (oops)
Comment 10 Vangelis Kokkevis 2011-07-21 16:02:26 PDT
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.
Comment 11 James Robinson 2011-07-21 19:39:03 PDT
(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.
Comment 12 James Robinson 2011-07-21 19:41:11 PDT
Created attachment 101691 [details]
Patch
Comment 13 Vangelis Kokkevis 2011-07-22 18:06:46 PDT
Comment on attachment 101691 [details]
Patch

(unofficial) LGTM from me.
Comment 14 Kenneth Russell 2011-07-22 18:09:24 PDT
Comment on attachment 101691 [details]
Patch

Sounds good. r=me again.
Comment 15 James Robinson 2011-07-22 18:55:34 PDT
Committed r91629: <http://trac.webkit.org/changeset/91629>