Bug 70645 – [chromium] Recycle tile-sized textures during commit to prevent reallocations

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Adrienne Walker]

[chromium] Recycle tile-sized textures during commit to prevent reallocations

[Adrienne Walker]

Created attachment 112016 [details]
Patch

[Adrienne Walker]

I know I said maybe I'd wait on doing this, but Alex and Vangelis finally convinced me that maybe this wouldn't be too hard and I should just do it.

[Adrienne Walker]

Created attachment 112360 [details]
rebaselined

[James Robinson]

So now that you've implemented it, is there any measurable impact?

[Vangelis Kokkevis]

Comment on attachment 112360 [details]
rebaselined

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

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:254
> +            if (m_evictedTextures[i].textureId) {

Do you ever expect the textureId to be 0 ?  Should this be an assert() instead?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:113
> +void CCLayerTreeHost::beginCommitOnImplThread(CCLayerTreeHostImpl*)

Why not remove the parameter completely from the method signature?

[Adrienne Walker]

Comment on attachment 112360 [details]
rebaselined

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

>> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:254
>> +            if (m_evictedTextures[i].textureId) {
> 
> Do you ever expect the textureId to be 0 ?  Should this be an assert() instead?

I'm a little bit leery of putting an assert here for textures that are reserved but not ever bound, because it's so far from any offending code.  However, that seems like bad behavior that we'd want to catch, so I'll investigate if adding an assert here looks plausible.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:113
>> +void CCLayerTreeHost::beginCommitOnImplThread(CCLayerTreeHostImpl*)
> 
> Why not remove the parameter completely from the method signature?

I'll just make this function beginCommit() on the main thread.

[Vangelis Kokkevis]

Comment on attachment 112360 [details]
rebaselined

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

>>> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:254
>>> +            if (m_evictedTextures[i].textureId) {
>> 
>> Do you ever expect the textureId to be 0 ?  Should this be an assert() instead?
> 
> I'm a little bit leery of putting an assert here for textures that are reserved but not ever bound, because it's so far from any offending code.  However, that seems like bad behavior that we'd want to catch, so I'll investigate if adding an assert here looks plausible.

It seems to me that textures will only be added to m_evictedTextures via TextureManager::removeTexture().  Is it possible that we'll ever get an info struct there with a zero for a texture id?  If that's so, then we would be incorrectly subtracting its size from m_memoryUseBytes.

[James Robinson]

*** Bug 72896 has been marked as a duplicate of this bug. ***

[Adrienne Walker]

klobag: Are you seeing a performance impact from the texture manager churn? That's really the reason I held off on landing this patch.  I wasn't convinced that the extra complexity was worth it.

(Also, it's not as if the code that used to locally recycle tiles ever worked after we added the TextureManager, because the textures would have just been evicted without the TiledLayerChromium's knowledge.)

[Grace Kloba]

It is hard to tell exactly. When we tried not clear zero after creating a tile, we noticed the time jump in the trace view where the following glTexSubImage2D is called. This implies that there may be a cost in creating the texture and allocating memory in the driver.

I have a patch to the TextureManager. I will update so that you can take a look.

(In reply to comment #9)
> klobag: Are you seeing a performance impact from the texture manager churn? That's really the reason I held off on landing this patch.  I wasn't convinced that the extra complexity was worth it.
> 
> (Also, it's not as if the code that used to locally recycle tiles ever worked after we added the TextureManager, because the textures would have just been evicted without the TiledLayerChromium's knowledge.)

[James Robinson]

What we need here is an automated performance test sensitive enough to detect a change like this. Run it with the patch, run it without, compare the #s.

[Grace Kloba]

Created attachment 116318 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116318 [details]
Patch

Attachment 116318 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10606259

[Vangelis Kokkevis]

Comment on attachment 116318 [details]
Patch

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

Trying to recycle textures is a good thing but I think the way it's implemented it will offer a limited benefit as it will only work between commits.  Every time we do a commit we aggressively delete textures that are over the reclaim limit.  Maybe we should simply add the ones that are tile-sized and have a high probability of being re-usable to a list that we look into first when we need to allocate a new texture.

> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:53
> +    return m_token && size == m_size && format == m_format && m_textureManager->hasTexture(m_token) && m_ready;

What problem is the use of m_ready trying to solve?

[Grace Kloba]

(In reply to comment #14)
> (From update of attachment 116318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116318&action=review
> 
> Trying to recycle textures is a good thing but I think the way it's implemented it will offer a limited benefit as it will only work between commits.  Every time we do a commit we aggressively delete textures that are over the reclaim limit.  Maybe we should simply add the ones that are tile-sized and have a high probability of being re-usable to a list that we look into first when we need to allocate a new texture.
> 

This patch is to avoid deleting textures during each commit. When request texture is called, we check whether it will make the total memory over the reclaim limit. If yes, we reuse the existing texture. Currently we just allocate a new one and delete an old one in commit. In this patch, we reuse the old one in request. So in scrolling, after we reach reclaim limit, we should not allocate tile textures any more.

> > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:53
> > +    return m_token && size == m_size && format == m_format && m_textureManager->hasTexture(m_token) && m_ready;
> 
> What problem is the use of m_ready trying to solve?

In TiledLayerChromium::pushPropertiesTo(), we sync the textureId if it is valid. Currently we may have a valid ManagedTexture while its textureId is 0 as allocate hasn't been called. In draw, we treat 0 textureId as no content. With this change, if the textureId is reused and before it is ready, we don't want to sync to the impl tree. Hope this makes sense.

[Vangelis Kokkevis]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 116318 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=116318&action=review
> > 
> > Trying to recycle textures is a good thing but I think the way it's implemented it will offer a limited benefit as it will only work between commits.  Every time we do a commit we aggressively delete textures that are over the reclaim limit.  Maybe we should simply add the ones that are tile-sized and have a high probability of being re-usable to a list that we look into first when we need to allocate a new texture.
> > 
> 
> This patch is to avoid deleting textures during each commit. When request texture is called, we check whether it will make the total memory over the reclaim limit. If yes, we reuse the existing texture. Currently we just allocate a new one and delete an old one in commit. In this patch, we reuse the old one in request. So in scrolling, after we reach reclaim limit, we should not allocate tile textures any more.

Oh, I see. Thanks for the explanation. 


> 
> > > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:53
> > > +    return m_token && size == m_size && format == m_format && m_textureManager->hasTexture(m_token) && m_ready;
> > 
> > What problem is the use of m_ready trying to solve?
> 
> In TiledLayerChromium::pushPropertiesTo(), we sync the textureId if it is valid. Currently we may have a valid ManagedTexture while its textureId is 0 as allocate hasn't been called. In draw, we treat 0 textureId as no content. With this change, if the textureId is reused and before it is ready, we don't want to sync to the impl tree. Hope this makes sense.

Hmm, don't you still want to pass the textureId to the impl side so that the new contents get uploaded?  If the textureId is zero then a new texture will be created for it.  If it's not, then we'll upload the new contents to it.  

I'm confused because it looks like if you have a new ManagedTexture then in TiledLayerChromium::pushPropertiesTo() isValid() will return false and you will early-out.  That means that the textureId for the corresponding DrawableTile will be zero. This is fine if the ManagedTexture doesn't use a recycled GL texture as when on the thread we call bindTexture() a new GL texture will be allocated for it as expected.  However if a recycled GL texture should be used then bindTexture() will still try to create a new GL texture instead of using the existing one.

Or maybe I'm misunderstanding something.  


 that ends up getting a recycled GL texture then you won't call syncTextureId with that recycled textureId which means that on the impl side the textureId will be zero and we'll try to allocate a new texture for it.

[Grace Kloba]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (From update of attachment 116318 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=116318&action=review
> > > 
> > > Trying to recycle textures is a good thing but I think the way it's implemented it will offer a limited benefit as it will only work between commits.  Every time we do a commit we aggressively delete textures that are over the reclaim limit.  Maybe we should simply add the ones that are tile-sized and have a high probability of being re-usable to a list that we look into first when we need to allocate a new texture.
> > > 
> > 
> > This patch is to avoid deleting textures during each commit. When request texture is called, we check whether it will make the total memory over the reclaim limit. If yes, we reuse the existing texture. Currently we just allocate a new one and delete an old one in commit. In this patch, we reuse the old one in request. So in scrolling, after we reach reclaim limit, we should not allocate tile textures any more.
> 
> Oh, I see. Thanks for the explanation. 
> 
> 
> > 
> > > > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:53
> > > > +    return m_token && size == m_size && format == m_format && m_textureManager->hasTexture(m_token) && m_ready;
> > > 
> > > What problem is the use of m_ready trying to solve?
> > 
> > In TiledLayerChromium::pushPropertiesTo(), we sync the textureId if it is valid. Currently we may have a valid ManagedTexture while its textureId is 0 as allocate hasn't been called. In draw, we treat 0 textureId as no content. With this change, if the textureId is reused and before it is ready, we don't want to sync to the impl tree. Hope this makes sense.
> 
> Hmm, don't you still want to pass the textureId to the impl side so that the new contents get uploaded?  If the textureId is zero then a new texture will be created for it.  If it's not, then we'll upload the new contents to it.  
> 

We want to upload the textureId when the new contents is drawn through LayerTextureUpdaterBitmap::updateTextureRect(). We set m_ready to be true inside bindTexture(). In the case where the textureId is not recycled, this is where textureId is allocated. In the case where the textureId is recycled, we set m_ready to be true so that the next pushPropertiesTo() will push it to the impl side. Hope this explains.

> I'm confused because it looks like if you have a new ManagedTexture then in TiledLayerChromium::pushPropertiesTo() isValid() will return false and you will early-out.  That means that the textureId for the corresponding DrawableTile will be zero. This is fine if the ManagedTexture doesn't use a recycled GL texture as when on the thread we call bindTexture() a new GL texture will be allocated for it as expected.  However if a recycled GL texture should be used then bindTexture() will still try to create a new GL texture instead of using the existing one.
> 
> Or maybe I'm misunderstanding something.  
> 
> 
>  that ends up getting a recycled GL texture then you won't call syncTextureId with that recycled textureId which means that on the impl side the textureId will be zero and we'll try to allocate a new texture for it.

[James Robinson]

Comment on attachment 116318 [details]
Patch

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

I think we really need to see some performance data before landing this, or any other potential optimization that adds complexity to the system. It's possible that this will make things faster.  It's also possible that this could make things slower by adding additional data dependencies that didn't exist before. Without some actual data it's impossible to guess.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:307
> +    if (TextureManager::reclaimLimitBytes() > contentsMemoryUseBytes)
> +        m_renderSurfaceTextureManager->setReclaimLimitBytes(TextureManager::reclaimLimitBytes() - contentsMemoryUseBytes);
> +    else
> +        m_renderSurfaceTextureManager->setReclaimLimitBytes(0);

this looks like an unrelated change - this patch is about tile textures, not render surface textures correct? in order to change behavior for tiles you need to modify the contents texture manager

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:240
> +int TextureManager::requestTexture(TextureToken token, IntSize size, unsigned format)

texture IDs are of type unsigned, not of type int. it looks like you are trying to pass two pieces of data via this return value - the texture ID and a boolean indicating whether the request succeeded. To do that, use two different parameters - a return value and a reference parameter, perhaps

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:272
> +        info.textureId = replaceTexture(token, info);
> +        if (info.textureId > 0)
> +            return info.textureId;

if the replaceTexture() call fails then this will replace info.textureId with 0. That doesn't seem right.

[Grace Kloba]

Comment on attachment 116318 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:307
>> +        m_renderSurfaceTextureManager->setReclaimLimitBytes(0);
> 
> this looks like an unrelated change - this patch is about tile textures, not render surface textures correct? in order to change behavior for tiles you need to modify the contents texture manager

The key change made in this patch is in TextureManager, which is shared by render surface textures, right? In the TextureManager, we decide whether we want to replace instead of adding a texture based on the reclaim limit. As five lines below we will call reduceMemoryToLimit based on this new limit, we try to set it to the TextureManager before calling drawLayersInternal() where request texture calling from.

>> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:240
>> +int TextureManager::requestTexture(TextureToken token, IntSize size, unsigned format)
> 
> texture IDs are of type unsigned, not of type int. it looks like you are trying to pass two pieces of data via this return value - the texture ID and a boolean indicating whether the request succeeded. To do that, use two different parameters - a return value and a reference parameter, perhaps

Indeed, it was using one return value to do two things. I can change it if the other way is preferred.

>> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:272
>> +            return info.textureId;
> 
> if the replaceTexture() call fails then this will replace info.textureId with 0. That doesn't seem right.

info.textureId is init to 0. If replaceTexture() failed, it keeps 0 value and continues to call addTexture. This is as designed.

[James Robinson]

There are different texture managers for render surfaces and content texture.  Tiles are in the latter, so any changes to the render surface texture manager will not impact tile textures.

[Grace Kloba]

(In reply to comment #20)
> There are different texture managers for render surfaces and content texture.  Tiles are in the latter, so any changes to the render surface texture manager will not impact tile textures.

Agree that there are different texture manager instance. But I think reuse texture Id can be applied to both texture manager. If you think render surface should be applied in the separate CL, we can remove from here.

[James Robinson]

Let's do one thing at a time. Right now we don't have evidence that recycling either type of ID will be a win and the allocation patterns for the two are pretty different, so I'd prefer not to change both allocators until we know if it's a win.

[Grace Kloba]

Created attachment 116989 [details]
new patch

Addressed James' comment and fixed the test file.

[WebKit Review Bot]

Attachment 116989 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Where's the performance data?  We cannot land this without that.

[Vangelis Kokkevis]

> > > > What problem is the use of m_ready trying to solve?
> > > 
> > > In TiledLayerChromium::pushPropertiesTo(), we sync the textureId if it is valid. Currently we may have a valid ManagedTexture while its textureId is 0 as allocate hasn't been called. In draw, we treat 0 textureId as no content. With this change, if the textureId is reused and before it is ready, we don't want to sync to the impl tree. Hope this makes sense.
> > 
> > Hmm, don't you still want to pass the textureId to the impl side so that the new contents get uploaded?  If the textureId is zero then a new texture will be created for it.  If it's not, then we'll upload the new contents to it.  
> > 
> 
> We want to upload the textureId when the new contents is drawn through LayerTextureUpdaterBitmap::updateTextureRect(). We set m_ready to be true inside bindTexture(). In the case where the textureId is not recycled, this is where textureId is allocated. In the case where the textureId is recycled, we set m_ready to be true so that the next pushPropertiesTo() will push it to the impl side. Hope this explains.
> 
> > I'm confused because it looks like if you have a new ManagedTexture then in TiledLayerChromium::pushPropertiesTo() isValid() will return false and you will early-out.  That means that the textureId for the corresponding DrawableTile will be zero. This is fine if the ManagedTexture doesn't use a recycled GL texture as when on the thread we call bindTexture() a new GL texture will be allocated for it as expected.  However if a recycled GL texture should be used then bindTexture() will still try to create a new GL texture instead of using the existing one.
> > 
> > Or maybe I'm misunderstanding something.  
> > 
> > 
> >  that ends up getting a recycled GL texture then you won't call syncTextureId with that recycled textureId which means that on the impl side the textureId will be zero and we'll try to allocate a new texture for it.

That doesn't seem completely consistent though with how things work now.  isValid() will return true even if the textureId is zero.  syncTextureId() will be called in pushPropertiesTo(), it just so happens that  since the textureId is zero, the tile will be skipped at draw time.  It seems to me that the logic to skip recycled tiles should be done at draw time where instead of checking whether the textureID is zero, we call a method that tell us whether the tile has valid contents.

[Grace Kloba]

Created attachment 117287 [details]
Patch

[James Robinson]

Comment on attachment 117287 [details]
Patch

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

This looks very close but there's still some unnecessary complexity here it looks like.

> Source/WebCore/platform/graphics/chromium/TextureManager.h:52
> +    static PassOwnPtr<TextureManager> create(size_t memoryLimitBytes, size_t reclaimLimitBytes, int maxTextureSize)

i'm not sure i understand why we need an additional parameter here - we seem to always set it to the default

> Source/WebCore/platform/graphics/chromium/TextureManager.h:67
> +    void setReclaimLimitBytes(size_t);

I don't understand why this API is being added - in this patch we always initialize the limit to TextureManager::reclaimLimitBytes() and never set it to anything else, so why does this need to be configurable at runtime?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:263
> +        tiledLayer->syncTextureId(i, j, tile->texture()->textureId(), tile->texture()->contentsValid());

what's the advantage of pushing a texture ID over that's not valid? should we just not sync textures that do not have valid contents to the impl side?

[Adrienne Walker]

Comment on attachment 117287 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:92
> +    m_contentsValid = m_textureId > 0;

Why would this ever be non-zero? It doesn't look like any recycling is occurring during allocateTexture, only during reserve.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:263

> 
> what's the advantage of pushing a texture ID over that's not valid? should we just not sync textures that do not have valid contents to the impl side?

I don't follow the need for contentsValid() at all.  When would this texture id be zero? If we reserve a texture and it succeeds, we always bind and upload it, which means that it should have a non-zero texture id during commit.  Even if tile->texture()->textureId() were somehow zero, then maybe we shouldn't sync the tile at all.

[Grace Kloba]

Comment on attachment 117287 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:92
>> +    m_contentsValid = m_textureId > 0;
> 
> Why would this ever be non-zero? It doesn't look like any recycling is occurring during allocateTexture, only during reserve.

I want to catch the case where allocateTexture fails. Now in draw, we check contentsValid instead of checking textureId is 0.

>> Source/WebCore/platform/graphics/chromium/TextureManager.h:52
>> +    static PassOwnPtr<TextureManager> create(size_t memoryLimitBytes, size_t reclaimLimitBytes, int maxTextureSize)
> 
> i'm not sure i understand why we need an additional parameter here - we seem to always set it to the default

It is similar to memoryLimitBytes, here is the default and setReclaimLimitBytes() can override it to the latest value.

>> Source/WebCore/platform/graphics/chromium/TextureManager.h:67
>> +    void setReclaimLimitBytes(size_t);
> 
> I don't understand why this API is being added - in this patch we always initialize the limit to TextureManager::reclaimLimitBytes() and never set it to anything else, so why does this need to be configurable at runtime?

In CCLayerTreeHost::beginCommitOnImplThread(), this is called to set the latest reclaim limit. If reclaimLimitBytes() is not constant, e.g. if it depends on the viewport size, we need to adjust it on the fly.

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:263
>>> +        tiledLayer->syncTextureId(i, j, tile->texture()->textureId(), tile->texture()->contentsValid());
>> 
>> what's the advantage of pushing a texture ID over that's not valid? should we just not sync textures that do not have valid contents to the impl side?
> 
> 

I was taking Vangelis's previous comment literally. I tried to make drawTiles not depending on checking textureId against 0. But I think your question is valid, if contents is not valid, we should just continue.

[Adrienne Walker]

Comment on attachment 117287 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:92
>>> +    m_contentsValid = m_textureId > 0;
>> 
>> Why would this ever be non-zero? It doesn't look like any recycling is occurring during allocateTexture, only during reserve.
> 
> I want to catch the case where allocateTexture fails. Now in draw, we check contentsValid instead of checking textureId is 0.

Maybe I'm just confused, but when does allocateTexture fail?

[James Robinson]

(In reply to comment #30)
> >> Source/WebCore/platform/graphics/chromium/TextureManager.h:67
> >> +    void setReclaimLimitBytes(size_t);
> > 
> > I don't understand why this API is being added - in this patch we always initialize the limit to TextureManager::reclaimLimitBytes() and never set it to anything else, so why does this need to be configurable at runtime?
> 
> In CCLayerTreeHost::beginCommitOnImplThread(), this is called to set the latest reclaim limit. If reclaimLimitBytes() is not constant, e.g. if it depends on the viewport size, we need to adjust it on the fly.
> 

The reclaim limit is constant.  At the point where we start adjusting on the fly we can change this code to match but not before.

[Grace Kloba]

Comment on attachment 117287 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:92
>>>> +    m_contentsValid = m_textureId > 0;
>>> 
>>> Why would this ever be non-zero? It doesn't look like any recycling is occurring during allocateTexture, only during reserve.
>> 
>> I want to catch the case where allocateTexture fails. Now in draw, we check contentsValid instead of checking textureId is 0.
> 
> Maybe I'm just confused, but when does allocateTexture fail?

You are right, it seems it can never fail. So no need for checking.

[Vangelis Kokkevis]

Comment on attachment 117287 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:263
>>>> +        tiledLayer->syncTextureId(i, j, tile->texture()->textureId(), tile->texture()->contentsValid());
>>> 
>>> what's the advantage of pushing a texture ID over that's not valid? should we just not sync textures that do not have valid contents to the impl side?
>> 
>> 
> 
> I was taking Vangelis's previous comment literally. I tried to make drawTiles not depending on checking textureId against 0. But I think your question is valid, if contents is not valid, we should just continue.

James, Enne and myself chatted about this. The conclusion was that if you simply remove the contentsValid boolean and all associated tests, and keep the rest of the code as you have it, it should all work as expected.

[Grace Kloba]

Created attachment 117533 [details]
Patch

[James Robinson]

Comment on attachment 117533 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:74
> +    bool m_contentsValid;

I believe you can remove this

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:272
> +        if (textureId > 0)

textureId is of type unsigned so it can never be < 0 and WebKit style prefers that checks for == 0 be written this way:

if (textureId)

> Source/WebCore/platform/graphics/chromium/TextureManager.h:52
> +    static PassOwnPtr<TextureManager> create(size_t memoryLimitBytes, size_t reclaimLimitBytes, int maxTextureSize)

this change should not be part of this patch - it's not about texture recycling. please remove it

> Source/WebCore/platform/graphics/chromium/TextureManager.h:67
> +    void setReclaimLimitBytes(size_t);

same here - this patch should only have changes related to texture recycling. we can do this later if/when it becomes useful

[Grace Kloba]

Comment on attachment 117533 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:74
>> +    bool m_contentsValid;
> 
> I believe you can remove this

Hmm, can we assume when TiledLayerChromium::pushPropertiesTo() is called, ManagedTexture::bindTexture() is already called? Then why we used to check whether textureId is 0 in drawTiles()?

>> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:272
>> +        if (textureId > 0)
> 
> textureId is of type unsigned so it can never be < 0 and WebKit style prefers that checks for == 0 be written this way:
> 
> if (textureId)

Done.

>> Source/WebCore/platform/graphics/chromium/TextureManager.h:52
>> +    static PassOwnPtr<TextureManager> create(size_t memoryLimitBytes, size_t reclaimLimitBytes, int maxTextureSize)
> 
> this change should not be part of this patch - it's not about texture recycling. please remove it

This is needed to decide when recycling texture is triggered. See line 270.

[James Robinson]

Comment on attachment 117533 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/TextureManager.h:52
>>> +    static PassOwnPtr<TextureManager> create(size_t memoryLimitBytes, size_t reclaimLimitBytes, int maxTextureSize)
>> 
>> this change should not be part of this patch - it's not about texture recycling. please remove it
> 
> This is needed to decide when recycling texture is triggered. See line 270.

Line 270 of TextureManager could (should?) be comparing against reclaimLimitBytes().  Let's not make things look configurable that aren't actually configurable.

[Grace Kloba]

Comment on attachment 117533 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/TextureManager.h:52
>>>> +    static PassOwnPtr<TextureManager> create(size_t memoryLimitBytes, size_t reclaimLimitBytes, int maxTextureSize)
>>> 
>>> this change should not be part of this patch - it's not about texture recycling. please remove it
>> 
>> This is needed to decide when recycling texture is triggered. See line 270.
> 
> Line 270 of TextureManager could (should?) be comparing against reclaimLimitBytes().  Let's not make things look configurable that aren't actually configurable.

If we hard code reclaimLimitBytes(), it assumes CCLayerTreeHost::beginCommitOnImplThread() will use it to reduce the memory. Anyway, I do whatever you want.

[Grace Kloba]

Created attachment 117541 [details]
Patch

[Adrienne Walker]

(In reply to comment #37)
> (From update of attachment 117533 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117533&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:74
> >> +    bool m_contentsValid;
> > 
> > I believe you can remove this
> 
> Hmm, can we assume when TiledLayerChromium::pushPropertiesTo() is called, ManagedTexture::bindTexture() is already called? Then why we used to check whether textureId is 0 in drawTiles()?

Yes, we can assume that.  I think that textureId check is vestigial and made more sense at some point in the psat.

[Grace Kloba]

(In reply to comment #41)
> (In reply to comment #37)
> > (From update of attachment 117533 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117533&action=review
> > 
> > >> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:74
> > >> +    bool m_contentsValid;
> > > 
> > > I believe you can remove this
> > 
> > Hmm, can we assume when TiledLayerChromium::pushPropertiesTo() is called, ManagedTexture::bindTexture() is already called? Then why we used to check whether textureId is 0 in drawTiles()?
> 
> Yes, we can assume that.  I think that textureId check is vestigial and made more sense at some point in the psat.

Ok, then I will also remove texureId 0 checking. So it does mean it is valid if tile is not null.

[Adrienne Walker]

(In reply to comment #42)

> Ok, then I will also remove texureId 0 checking. So it does mean it is valid if tile is not null.

Even if it's not applicable, I don't think you should remove the check, because I'd prefer that code to remain robust to future errors.  Texture id 0 is never valid, so I'd like that to checkerboard if it somehow happens at runtime.

[Grace Kloba]

Created attachment 117644 [details]
Patch

[Adrienne Walker]

Comment on attachment 117644 [details]
Patch

Thanks for taking the time to simplify this patch down to just the essentials.  This looks great to me.  :)

[James Robinson]

Comment on attachment 117644 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 117644 [details]
Patch

Clearing flags on attachment: 117644

Committed r101854: <http://trac.webkit.org/changeset/101854>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Reveman]

I reverted this change as it was causing a serious performance regression:
https://bugs.webkit.org/show_bug.cgi?id=78020

A possible alternative discussed is to land https://bugs.webkit.org/show_bug.cgi?id=76295 and implement this kind of texture reuse on the impl side.