Bug 70645 - [chromium] Recycle tile-sized textures during commit to prevent reallocations
Summary: [chromium] Recycle tile-sized textures during commit to prevent reallocations
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
: 72896 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-10-21 13:55 PDT by Adrienne Walker
Modified: 2013-04-08 11:16 PDT (History)
9 users (show)

See Also:


Attachments
Patch (11.25 KB, patch)
2011-10-21 13:59 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
rebaselined (11.91 KB, patch)
2011-10-25 11:07 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (12.57 KB, patch)
2011-11-22 22:27 PST, Grace Kloba
jamesr: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
new patch (12.62 KB, patch)
2011-11-29 09:51 PST, Grace Kloba
no flags Details | Formatted Diff | Diff
Patch (18.42 KB, patch)
2011-11-30 15:45 PST, Grace Kloba
no flags Details | Formatted Diff | Diff
Patch (15.23 KB, patch)
2011-12-01 17:46 PST, Grace Kloba
no flags Details | Formatted Diff | Diff
Patch (8.93 KB, patch)
2011-12-01 19:03 PST, Grace Kloba
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2011-12-02 10:07 PST, Grace Kloba
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-10-21 13:55:53 PDT
[chromium] Recycle tile-sized textures during commit to prevent reallocations
Comment 1 Adrienne Walker 2011-10-21 13:59:22 PDT
Created attachment 112016 [details]
Patch
Comment 2 Adrienne Walker 2011-10-21 14:01:48 PDT
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.
Comment 3 Adrienne Walker 2011-10-25 11:07:21 PDT
Created attachment 112360 [details]
rebaselined
Comment 4 James Robinson 2011-10-25 12:20:33 PDT
So now that you've implemented it, is there any measurable impact?
Comment 5 Vangelis Kokkevis 2011-10-25 12:42:22 PDT
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?
Comment 6 Adrienne Walker 2011-10-25 13:28:44 PDT
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.
Comment 7 Vangelis Kokkevis 2011-10-25 22:57:27 PDT
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.
Comment 8 James Robinson 2011-11-21 13:04:22 PST
*** Bug 72896 has been marked as a duplicate of this bug. ***
Comment 9 Adrienne Walker 2011-11-21 14:16:35 PST
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.)
Comment 10 Grace Kloba 2011-11-21 14:59:21 PST
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.)
Comment 11 James Robinson 2011-11-21 16:41:06 PST
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.
Comment 12 Grace Kloba 2011-11-22 22:27:25 PST
Created attachment 116318 [details]
Patch
Comment 13 WebKit Review Bot 2011-11-23 03:31:53 PST
Comment on attachment 116318 [details]
Patch

Attachment 116318 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10606259
Comment 14 Vangelis Kokkevis 2011-11-23 11:10:38 PST
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?
Comment 15 Grace Kloba 2011-11-23 12:18:53 PST
(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.
Comment 16 Vangelis Kokkevis 2011-11-23 15:24:36 PST
(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.
Comment 17 Grace Kloba 2011-11-23 16:42:55 PST
(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.
Comment 18 James Robinson 2011-11-27 13:26:15 PST
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.
Comment 19 Grace Kloba 2011-11-28 10:51:16 PST
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.
Comment 20 James Robinson 2011-11-28 13:19:33 PST
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.
Comment 21 Grace Kloba 2011-11-28 13:47:29 PST
(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.
Comment 22 James Robinson 2011-11-28 15:20:39 PST
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.
Comment 23 Grace Kloba 2011-11-29 09:51:49 PST
Created attachment 116989 [details]
new patch

Addressed James' comment and fixed the test file.
Comment 24 WebKit Review Bot 2011-11-29 09:56:21 PST
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.
Comment 25 James Robinson 2011-11-29 12:44:27 PST
Where's the performance data?  We cannot land this without that.
Comment 26 Vangelis Kokkevis 2011-11-29 13:06:38 PST
> > > > 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.
Comment 27 Grace Kloba 2011-11-30 15:45:37 PST
Created attachment 117287 [details]
Patch
Comment 28 James Robinson 2011-11-30 16:25:51 PST
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?
Comment 29 Adrienne Walker 2011-11-30 17:08:34 PST
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.
Comment 30 Grace Kloba 2011-11-30 17:52:17 PST
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.
Comment 31 Adrienne Walker 2011-11-30 18:27:40 PST
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?
Comment 32 James Robinson 2011-11-30 18:30:31 PST
(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.
Comment 33 Grace Kloba 2011-11-30 18:38:09 PST
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.
Comment 34 Vangelis Kokkevis 2011-12-01 16:22:00 PST
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.
Comment 35 Grace Kloba 2011-12-01 17:46:31 PST
Created attachment 117533 [details]
Patch
Comment 36 James Robinson 2011-12-01 17:50:02 PST
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
Comment 37 Grace Kloba 2011-12-01 18:11:33 PST
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.
Comment 38 James Robinson 2011-12-01 18:23:16 PST
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.
Comment 39 Grace Kloba 2011-12-01 18:48:15 PST
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.
Comment 40 Grace Kloba 2011-12-01 19:03:04 PST
Created attachment 117541 [details]
Patch
Comment 41 Adrienne Walker 2011-12-02 09:07:18 PST
(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.
Comment 42 Grace Kloba 2011-12-02 09:44:49 PST
(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.
Comment 43 Adrienne Walker 2011-12-02 09:57:28 PST
(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.
Comment 44 Grace Kloba 2011-12-02 10:07:17 PST
Created attachment 117644 [details]
Patch
Comment 45 Adrienne Walker 2011-12-02 10:13:53 PST
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.  :)
Comment 46 James Robinson 2011-12-02 12:52:08 PST
Comment on attachment 117644 [details]
Patch

R=me
Comment 47 WebKit Review Bot 2011-12-02 14:30:23 PST
Comment on attachment 117644 [details]
Patch

Clearing flags on attachment: 117644

Committed r101854: <http://trac.webkit.org/changeset/101854>
Comment 48 WebKit Review Bot 2011-12-02 14:30:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 49 David Reveman 2012-02-07 17:12:10 PST
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.