Bug 89786 – [Chromium] RenderPass textures are evicted at the end of every frame

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

[Chromium] Some textures are evicted at the end of every frame

Created attachment 149107 [details]
Patch

[James Robinson]

Comment on attachment 149107 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-1196
> -    m_implTextureManager->deleteEvictedTextures(m_implTextureAllocator.get());

So what is actually holding the render surface textures to the limit?

[Dana Jansens]

Comment on attachment 149107 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-1196
>> -    m_implTextureManager->deleteEvictedTextures(m_implTextureAllocator.get());
> 
> So what is actually holding the render surface textures to the limit?

Hiliariously, I don't think we actually hold ourselves to the max limit at all during drawing, we evict but don't delete, and end up allocating and keeping all the surfaces in the frame until the frame is done.

The delete here would be good to drop us back to the max limit. To actually stay below it, we should be deleting evicted textures between every reserve() and allocate() on a managed texture in LRC.

(In reply to comment #3)
> (From update of attachment 149107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149107&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-1196
> >> -    m_implTextureManager->deleteEvictedTextures(m_implTextureAllocator.get());
> > 
> > So what is actually holding the render surface textures to the limit?
> 
> Hiliariously, I don't think we actually hold ourselves to the max limit at all during drawing, we evict but don't delete, and end up allocating and keeping all the surfaces in the frame until the frame is done.
> 
> The delete here would be good to drop us back to the max limit. To actually stay below it, we should be deleting evicted textures between every reserve() and allocate() on a managed texture in LRC.

Can evict and delete be one operation?

[Dana Jansens]

Comment on attachment 149107 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-1196
>>>> -    m_implTextureManager->deleteEvictedTextures(m_implTextureAllocator.get());
>>> 
>>> So what is actually holding the render surface textures to the limit?
>> 
>> Hiliariously, I don't think we actually hold ourselves to the max limit at all during drawing, we evict but don't delete, and end up allocating and keeping all the surfaces in the frame until the frame is done.
>> 
>> The delete here would be good to drop us back to the max limit. To actually stay below it, we should be deleting evicted textures between every reserve() and allocate() on a managed texture in LRC.
> 
> Can evict and delete be one operation?

The TextureManager we're using right now is designed for cross-thread operation, really for a completely different use case. Evict is something done on the main thread so it doesn't actually do the delete (main thread can't access the GC3d).

reserve() causes the eviction, so you could call deleteEvictedTextures() after each reserve and that would have the effect.

[Dana Jansens]

We haven't been sticking to the max limit since I don't know when. Personally, I think we should just keep the delete in finishFrame and let the rest of the code be for now, and we'll fix it in a better way than adding more misuse of TextureManager.

Created attachment 149286 [details]
Patch

[Adrienne Walker]

Comment on attachment 149286 [details]
Patch

R=me.  I totally agree that this is a temporary pathc, but I think both caching render surfaces and actually deleting them seems like a positive step.

[WebKit Review Bot]

Comment on attachment 149286 [details]
Patch

Clearing flags on attachment: 149286

Committed r121172: <http://trac.webkit.org/changeset/121172>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.