[webkit-reviews] review denied: [Bug 70645] [chromium] Recycle tile-sized textures during commit to prevent reallocations : [Attachment 116318] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Grace Kloba
<klobag at chromium.org>'s request for review:
Bug 70645: [chromium] Recycle tile-sized textures during commit to prevent
reallocations
https://bugs.webkit.org/show_bug.cgi?id=70645

Attachment 116318: Patch
https://bugs.webkit.org/attachment.cgi?id=116318&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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::reclaimLimi
tBytes() - 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.