Bug 77135 – [chromium] canvas demo is slow due to unnecessary resource cleanups

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alok Priyadarshi]

JSGameBench updates the FPS by calling innerHTML on the parentFPS div every frame. This in turn removes a layer from the compositing layer tree. When WebCore::FrameView::updateCompositingLayers() is called on the next layout, it tries to add the new FPS div by adding it to the tree through GraphicsLayer::setChildren(), which removes all child layers even though some of them will be added right back. As soon as we remove a layer from the tree we also evict all associated texture resources. This means we are unnecessarily deleting and updating a huge tiled layer every frame.

Relevant call stack:
WebCore::TiledLayerChromium::cleanupResources()  Line 98
WebCore::TiledLayerChromium::setLayerTreeHost(WebCore::CCLayerTreeHost * host=0x00000000)  Line 182 + 0xf bytes
WebCore::LayerChromium::setParent(WebCore::LayerChromium * layer=0x00000000)  Line 146 + 0x33 bytes
WebCore::LayerChromium::removeChild(WebCore::LayerChromium * child=0x0f27c680)  Line 185
WebCore::LayerChromium::removeFromParent()  Line 176
WebCore::GraphicsLayerChromium::removeFromParent()  Line 162
WebCore::GraphicsLayer::removeAllChildren()  Line 223 + 0xf bytes
WebCore::GraphicsLayer::setChildren(const WTF::Vector<WebCore::GraphicsLayer *,0> & newChildren={...})  Line 128
WebCore::GraphicsLayerChromium::setChildren(const WTF::Vector<WebCore::GraphicsLayer *,0> & children={...})  Line 115 + 0xc bytes
WebCore::RenderLayerCompositor::rebuildCompositingLayerTree(WebCore::RenderLayer * layer=0x0cd9b30c, WTF::Vector<WebCore::GraphicsLayer *,0> & childLayersOfEnclosingLayer={...})  Line 922 + 0x1e bytes
WebCore::RenderLayerCompositor::rebuildCompositingLayerTree(WebCore::RenderLayer * layer=0x0cd9bb0c, WTF::Vector<WebCore::GraphicsLayer *,0> & childLayersOfEnclosingLayer={...})  Line 912
WebCore::RenderLayerCompositor::rebuildCompositingLayerTree(WebCore::RenderLayer * layer=0x0cd9ba0c, WTF::Vector<WebCore::GraphicsLayer *,0> & childLayersOfEnclosingLayer={...})  Line 912
WebCore::RenderLayerCompositor::updateCompositingLayers(WebCore::CompositingUpdateType updateType=CompositingUpdateAfterLayoutOrStyleChange, WebCore::RenderLayer * updateRoot=0x0cd9ba0c)  Line 323
WebCore::FrameView::updateCompositingLayers()  Line 669
WebCore::FrameView::layout(bool allowSubtree=true)  Line 1154
WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()  Line 2974
WebKit::WebFrameImpl::layout()  Line 2076
WebKit::WebViewImpl::layout()  Line 1184
RenderWidget::DoDeferredUpdate()  Line 768 + 0x17 bytes
RenderWidget::DoDeferredUpdateAndSendInputAck()  Line 734
RenderWidget::OnSwapBuffersComplete()  Line 460
RenderViewImpl::OnViewContextSwapBuffersComplete()  Line 4292

[Alok Priyadarshi]

I am investigating if we can avoid calling TiledLayerChromium::setLayerTreeHost(0) when LayerChromium::setParent(0) is called.

Nat points that it could be tricky since we have ref-ptr cycle between CCLayerTreeHost and LayerChromium.

[James Robinson]

I think the real problem here is the weak pointer relationship between ManagedTextures (which layers own) and TextureManager (which the CCLayerTreeHost owns). ManagedTextures are associated with a TextureManager and have a weak pointer to them.  Layers can become detached from LTHs and in some cases outlive the LTH, but we don't want to let the ManagedTexture continue pointing to a bogus TextureManager after the LTH goes away.

We maintain this today by saying that whenever a layer leaves the LTH, we destroy all ManagedTextures.  We could refine this and only destroy the ManagedTextures when the layer becomes associated with a different LTH so that if a layer leaves and then reenters the same LTH it keeps the same ManagedTexture alive, but we'd still have to do something about the case where a layer becomes detached and then outlives the destruction of the LTH that it was associated with.

[Alok Priyadarshi]

Created attachment 124196 [details]
proposed patch

This patch fixes the JSGameBench canvas demo. I am still testing if it has other unintended consequences.

[James Robinson]

Comment on attachment 124196 [details]
proposed patch

This won't work if the LayerChromium is detached and then the CCLayerTreeHost is destroyed before the LayerChromium is since the LayerChromium's destructor will try to tear down ManagedTextures which still have a raw pointer to the TextureManager, which died when the host died.

[Alok Priyadarshi]

(In reply to comment #4)
> (From update of attachment 124196 [details])
> This won't work if the LayerChromium is detached and then the CCLayerTreeHost is destroyed before the LayerChromium is since the LayerChromium's destructor will try to tear down ManagedTextures which still have a raw pointer to the TextureManager, which died when the host died.

CCLayerTreeHost cannot be destroyed because LayerChromium holds a ref-ptr to the LTH.
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerChromium.h#L245

But I agree this is a very flimsy design, and this patch makes it even worse. BTW do you know of a case where a Layer is not destroyed after being removed from a tree, i.e. the layer either moves to another tree or just stays detached forever.

[James Robinson]

It's fairly common for a layer to outlive the LTH. WebGLRenderingContexts have DrawingBuffers which have PlatformLayers (WebGLLayerChromiums). The context itself is held alive via a javascript reference which is often garbage collected after the page containing the canvas is navigated away from or destroyed. 2D canvas has the same thing with the reference chain HTMLCanvasElement -> ImageBuffer -> ImageBufferData::m_platformLayer.  I'm not completely sure about video / plugin layers off the top of my head, but it wouldn't shock me if they outlived the page in some circumstances.

I agree this is a pile of poo.  Let's figure out what we can do that works for now, and then keep notes of what to improve for the future.

What if we treated GraphicsLayer::setChildren() as more of a bulk operation rather than as a serieis of individual operations that have to all maintain internal invariants?  Could we do something smarter there?

[Alok Priyadarshi]

(In reply to comment #6)
> It's fairly common for a layer to outlive the LTH. WebGLRenderingContexts have DrawingBuffers which have PlatformLayers (WebGLLayerChromiums). The context itself is held alive via a javascript reference which is often garbage collected after the page containing the canvas is navigated away from or destroyed. 2D canvas has the same thing with the reference chain HTMLCanvasElement -> ImageBuffer -> ImageBufferData::m_platformLayer.  I'm not completely sure about video / plugin layers off the top of my head, but it wouldn't shock me if they outlived the page in some circumstances.
> 
> I agree this is a pile of poo.  Let's figure out what we can do that works for now, and then keep notes of what to improve for the future.

This patch works. As I noted in my previous response, LTH will not get destroyed because the layer keeps a ref-ptr. Note that I am not calling setLayerTreeHost(0) when the layer is removed from the tree.

> What if we treated GraphicsLayer::setChildren() as more of a bulk operation rather than as a serieis of individual operations that have to all maintain internal invariants?  Could we do something smarter there?

Yes we could make setChildren() smarter by not removing the layers common between the current child-list and new child-list. But we would also need to fix other functions that move layers in the tree. RenderLayer::removeOnlyThisLayer() is one such example.

[James Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > It's fairly common for a layer to outlive the LTH. WebGLRenderingContexts have DrawingBuffers which have PlatformLayers (WebGLLayerChromiums). The context itself is held alive via a javascript reference which is often garbage collected after the page containing the canvas is navigated away from or destroyed. 2D canvas has the same thing with the reference chain HTMLCanvasElement -> ImageBuffer -> ImageBufferData::m_platformLayer.  I'm not completely sure about video / plugin layers off the top of my head, but it wouldn't shock me if they outlived the page in some circumstances.
> > 
> > I agree this is a pile of poo.  Let's figure out what we can do that works for now, and then keep notes of what to improve for the future.
> 
> This patch works. As I noted in my previous response, LTH will not get destroyed because the layer keeps a ref-ptr. Note that I am not calling setLayerTreeHost(0) when the layer is removed from the tree.
> 
"Works" is a stretch - I think this patch breaks other things.  We rely on "have LTH pointer set" meaning "currently in the compositing tree" in other places in the code, for example for canvas / webgl rate limiting, triggering new frames for canvas / webgl / video / plugins, and probably a few other things that I'm forgetting.  I don't think it's value to simply leave the pointer there when a layer leaves the tree and hope that everything magically works out.

I'm OK with a quick fix but it has to be better than this.

[James Robinson]

We could make TextureManager managed the weak pointers explicitly and not attempt to clear ManagedTextures when nulling the LTH.  Then if a layer leaves one LTH we can leave the ManagedTextures alive.  If the LTH goes away before the layer, then ~TextureManager nulls out the ManagedTexture's m_manager pointers and the ManagedTexture is effectively dead.  If the layer reenters the same tree then its ManagedTexture is still valid.

The one tricky case is if the layer enters a different LTH all of its ManagedTextures that pointed to the previous LTH's TextureManager have to be dropped and regenerated with the new TextureManager.  Canvas2DLayerChromium's m_frontTexture will have this problem.  I think this is solvable in setLTH() by comparing the ManagedTexture's TextureManager pointer with the new LTH's contentsTextureManager and resetting if they differ.

[Alok Priyadarshi]

Created attachment 124764 [details]
proposed patch

This patch caches the last valid LTH and clears the resources only when the layer is moved into another valid LTH. One problem that still needs to be solved is what happens when a layer is deleted after the LTH.

James suggested this - "If the LTH goes away before the layer, then ~TextureManager nulls out the ManagedTexture's m_manager pointers and the ManagedTexture is effectively dead." But looking at ManagedTexture implementation, this does not seem feasible without major refactoring. ManagedTexture assumes that m_textureManager is valid in quite a few places. Am I missing something obvious?

Vangelis suggested adding the orphaned layers to a dummy branch in LTH. This will fix the issue of releasing the layers as soon as LTH is deleted, but if ref-count the layers in the dummy branch, the layers themselves will not get deleted when they are detached from the tree.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


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

[James Robinson]

Comment on attachment 124764 [details]
proposed patch

I'm afraid this doesn't work either, pointer identity != object identity in C++.  Consider:

Layer removed from LTH A
LTH A destroyed
new LTH B allocated at the same addresses as LTH A
layer added to LTH B
pointer comparison passes, but all other state on the layer is still bogus!

This also doesn't address the problems I pointed out earlier with leaving the LTH pointer set while the layer is out of the tree.

[James Robinson]

(In reply to comment #10)
> Created an attachment (id=124764) [details]
> proposed patch
> 
> This patch caches the last valid LTH and clears the resources only when the layer is moved into another valid LTH. One problem that still needs to be solved is what happens when a layer is deleted after the LTH.
> 
> James suggested this - "If the LTH goes away before the layer, then ~TextureManager nulls out the ManagedTexture's m_manager pointers and the ManagedTexture is effectively dead." But looking at ManagedTexture implementation, this does not seem feasible without major refactoring. ManagedTexture assumes that m_textureManager is valid in quite a few places. Am I missing something obvious?
> 

It's just adding some null checks and ASSERT()s.  In particular isValid() and reserve() have to return false when the manager is null, most other functions should just ASSERT().  Did you even try this?

[Alok Priyadarshi]

(In reply to comment #13)
> (In reply to comment #10)
> > Created an attachment (id=124764) [details] [details]
> > proposed patch
> > 
> > This patch caches the last valid LTH and clears the resources only when the layer is moved into another valid LTH. One problem that still needs to be solved is what happens when a layer is deleted after the LTH.
> > 
> > James suggested this - "If the LTH goes away before the layer, then ~TextureManager nulls out the ManagedTexture's m_manager pointers and the ManagedTexture is effectively dead." But looking at ManagedTexture implementation, this does not seem feasible without major refactoring. ManagedTexture assumes that m_textureManager is valid in quite a few places. Am I missing something obvious?
> > 
> 
> It's just adding some null checks and ASSERT()s.  In particular isValid() and reserve() have to return false when the manager is null, most other functions should just ASSERT().  Did you even try this?

I did not try it. It looked hairy. Most importantly I did not see any reference to ManagedTexture from TextureManager, which only holds texture tokens and ids. Do you know if there is a way to map the texture ids/tokens to ManagedTexture?

[James Robinson]

That's what I was suggesting you add.

[Alok Priyadarshi]

(In reply to comment #9)
> We could make TextureManager managed the weak pointers explicitly and not attempt to clear ManagedTextures when nulling the LTH.  Then if a layer leaves one LTH we can leave the ManagedTextures alive.  If the LTH goes away before the layer, then ~TextureManager nulls out the ManagedTexture's m_manager pointers and the ManagedTexture is effectively dead.  If the layer reenters the same tree then its ManagedTexture is still valid.

This reasonable and easy to implement. In fact I have it working locally.

> The one tricky case is if the layer enters a different LTH all of its ManagedTextures that pointed to the previous LTH's TextureManager have to be dropped and regenerated with the new TextureManager.  Canvas2DLayerChromium's m_frontTexture will have this problem.  I think this is solvable in setLTH() by comparing the ManagedTexture's TextureManager pointer with the new LTH's contentsTextureManager and resetting if they differ.

This is really tricky to handle in TiledLayerChromium::setLayerTreeHost(). To avoid resetting all ManagedTextures held in tiles, we need to make sure that:
1. TextureManager is the same. This is simple - just iterate through the tile textures and compare TextureManager pointers.
2. The dirty-rect stored in the layer (tiles) is still valid. Is it? If not we would need to invalidate all tiles which defeats the whole purpose.
3. TiledLayerChromium::m_textureUpdater can be reused. The tiles hold a weak reference to TiledLayerChromium::m_textureUpdater via UpdatableTile->m_texture->m_textureUpdater. As soon as m_textureUpdater is recreated, all tiles need to deleted because they will be holding garbage.

This is turning out to be more complicated than I anticipated. I wonder if making setChildren() smarter would be better.

[Alok Priyadarshi]

Created attachment 125201 [details]
proposed patch

This is another approach that just collects all layers being orphaned during layout and releases them at the end of layout.

[WebKit Review Bot]

Attachment 125201 [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/platform/graphics/chromium/cc/CCLayerTreeHost.h:177:  The parameter name "layer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:178:  The parameter name "layer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 6 files


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

[James Robinson]

(In reply to comment #16)
> > The one tricky case is if the layer enters a different LTH all of its ManagedTextures that pointed to the previous LTH's TextureManager have to be dropped and regenerated with the new TextureManager.  Canvas2DLayerChromium's m_frontTexture will have this problem.  I think this is solvable in setLTH() by comparing the ManagedTexture's TextureManager pointer with the new LTH's contentsTextureManager and resetting if they differ.
> 
> This is really tricky to handle in TiledLayerChromium::setLayerTreeHost(). To avoid resetting all ManagedTextures held in tiles, we need to make sure that:
> 1. TextureManager is the same. This is simple - just iterate through the tile textures and compare TextureManager pointers.
> 2. The dirty-rect stored in the layer (tiles) is still valid. Is it? If not we would need to invalidate all tiles which defeats the whole purpose.

ManagedTextures don't have dirty rects. There is no case in which you need to worry about the dirty rects.  If the TextureManager is different, then request a new token from the new TextureManager and stick it in the ManagedTexture.  Then the next time we go to paint the layer will see that isValid() is false on that tile and grow the dirty rect as appropriate, but you don't need to add any new logic for that.

> 3. TiledLayerChromium::m_textureUpdater can be reused. The tiles hold a weak reference to TiledLayerChromium::m_textureUpdater via UpdatableTile->m_texture->m_textureUpdater. As soon as m_textureUpdater is recreated, all tiles need to deleted because they will be holding garbage.

Sure, just do that if you need to.

> 
> This is turning out to be more complicated than I anticipated. I wonder if making setChildren() smarter would be better.

That only solves one type of mutations.  I think you think this is more complicated than it really is.

[James Robinson]

Comment on attachment 125201 [details]
proposed patch

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

This again won't work.  Layout can happen at nearly any time, not just when called by WebViewImpl, since javascript can force layout to happen inline.

> Source/WebCore/ChangeLog:8
> +        Do not release layer resources during Layout.

bizarre capitalization of a random Word here.

> Source/WebCore/ChangeLog:9
> +        Collect all render layers being orphaned and release them at the end of Layout.

same Here

> Source/WebKit/chromium/src/WebViewImpl.cpp:1183
>          // setFrameRect may have the side-effect of causing existing page
>          // layout to be invalidated, so layout needs to be called last.
> +        if (m_layerTreeHost)
> +            m_layerTreeHost->beginLayout();

there are many other ways to enter layout beside this one. Set a breakpoint in GraphicsLayerChromium::setChildren() and see.

I don't think this approach is scalable.

[James Robinson]

Making ManagedTextures robust to TextureManager deletion would be useful for https://bugs.webkit.org/show_bug.cgi?id=77655 as well, could you upload that part to that bug if you have it working already, Alok?  I guess I'll just have to write the rest of the patch myself.

[Alok Priyadarshi]

(In reply to comment #21)
> Making ManagedTextures robust to TextureManager deletion would be useful for https://bugs.webkit.org/show_bug.cgi?id=77655 as well, could you upload that part to that bug if you have it working already, Alok?  I guess I'll just have to write the rest of the patch myself.

I have uploaded the patch for TextureManager.

[Alok Priyadarshi]

(In reply to comment #19)
> (In reply to comment #16)
> > > The one tricky case is if the layer enters a different LTH all of its ManagedTextures that pointed to the previous LTH's TextureManager have to be dropped and regenerated with the new TextureManager.  Canvas2DLayerChromium's m_frontTexture will have this problem.  I think this is solvable in setLTH() by comparing the ManagedTexture's TextureManager pointer with the new LTH's contentsTextureManager and resetting if they differ.
> > 
> > This is really tricky to handle in TiledLayerChromium::setLayerTreeHost(). To avoid resetting all ManagedTextures held in tiles, we need to make sure that:
> > 1. TextureManager is the same. This is simple - just iterate through the tile textures and compare TextureManager pointers.
> > 2. The dirty-rect stored in the layer (tiles) is still valid. Is it? If not we would need to invalidate all tiles which defeats the whole purpose.
> 
> ManagedTextures don't have dirty rects. There is no case in which you need to worry about the dirty rects.  If the TextureManager is different, then request a new token from the new TextureManager and stick it in the ManagedTexture.  Then the next time we go to paint the layer will see that isValid() is false on that tile and grow the dirty rect as appropriate, but you don't need to add any new logic for that.

I was concerned about UpdatableTile::m_dirtyRect which is set inside TiledLayerChromium::invalidateRect(). If a layer moves within the same tree or to another tree, are those dirty rects still valid? Currently we invalidate the whole layer.

> 
> > 3. TiledLayerChromium::m_textureUpdater can be reused. The tiles hold a weak reference to TiledLayerChromium::m_textureUpdater via UpdatableTile->m_texture->m_textureUpdater. As soon as m_textureUpdater is recreated, all tiles need to deleted because they will be holding garbage.
> 
> Sure, just do that if you need to.

Deleting all tiles is equivalent to call cleanupResources(), so you really want to reuse m_textureUpdater. This can be done but would require caching and comparing all parameters (CCLayerTreeHost::m_settings and LayerRendererCapabilities) that determine the type of LayerTextureUpdater. I can give it a try. I got discouraged because it started looking super ugly.

> > 
> > This is turning out to be more complicated than I anticipated. I wonder if making setChildren() smarter would be better.
> 
> That only solves one type of mutations.

You are right. Thats why I was aiming for a more general solution.

[James Robinson]

Created attachment 125452 [details]
Patch

[James Robinson]

With this patch I don't see any extra texture uploads on jsgamebench canvas demo, other than the FPS counter div which is changing every frame.

[Alok Priyadarshi]

Comment on attachment 125452 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:124
> +    if (m_useDoubleBuffering && host)

Is there a reason for moving the check for m_useDouldeBuffering here?

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:-109
> -    // If we're changing hosts then we need to free up any resources

Now that we are not releasing resources with setLayerTreeHost(0), we should do that at least in the destructor of layers.

> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:49
> +    void setTextureManager(TextureManager*);

Is there a difference between clearManager() and setTextureManager(0)?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:264
> +void TiledLayerChromium::setLayerTreeHost(CCLayerTreeHost* host)

I still see one problem here that I mentioned previously. If the host changes, the subclasses may need to update the texture-updater, because the type of texture-updater depends on host settings and layer-renderer-capabilities. Also this is based on the assumption that when a layer moves from one host to another, the dirty-rect remains valid. I am not sure if we can make that assumption, but if we do, a comment shoudl be added here to that effect.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:267
> +    if (host) {

you do not want to do this if (host == layerTreeHost())

[James Robinson]

Comment on attachment 125452 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:124
>> +    if (m_useDoubleBuffering && host)
> 
> Is there a reason for moving the check for m_useDouldeBuffering here?

It's shorter, where would you put it?

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:-109
>> -    // If we're changing hosts then we need to free up any resources
> 
> Now that we are not releasing resources with setLayerTreeHost(0), we should do that at least in the destructor of layers.

cleanupResources() just clears out member variables, which the destructor does anyway

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:49
>> +    void setTextureManager(TextureManager*);
> 
> Is there a difference between clearManager() and setTextureManager(0)?

Yes, setTextureManager calls unregisterTexture on the previous owner. That's not necessary if the manager is going away

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:264
>> +void TiledLayerChromium::setLayerTreeHost(CCLayerTreeHost* host)
> 
> I still see one problem here that I mentioned previously. If the host changes, the subclasses may need to update the texture-updater, because the type of texture-updater depends on host settings and layer-renderer-capabilities. Also this is based on the assumption that when a layer moves from one host to another, the dirty-rect remains valid. I am not sure if we can make that assumption, but if we do, a comment shoudl be added here to that effect.

No, that's wrong. Tiled layers don't move between hosts and the dirty rect would be orthogonal even if they could.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:267
>> +    if (host) {
> 
> you do not want to do this if (host == layerTreeHost())

it doesn't change behavior at all since setTextureManager() to the same value is a no-op, but we don't have to iterate if the host is the same as the old one. will add an early-out here before landing

[Alok Priyadarshi]

Comment on attachment 125452 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:264
>>> +void TiledLayerChromium::setLayerTreeHost(CCLayerTreeHost* host)
>> 
>> I still see one problem here that I mentioned previously. If the host changes, the subclasses may need to update the texture-updater, because the type of texture-updater depends on host settings and layer-renderer-capabilities. Also this is based on the assumption that when a layer moves from one host to another, the dirty-rect remains valid. I am not sure if we can make that assumption, but if we do, a comment shoudl be added here to that effect.
> 
> No, that's wrong. Tiled layers don't move between hosts and the dirty rect would be orthogonal even if they could.

Sorry I do not understand. Both - ImageLayerChromium and ContentLayerChromium use LTH to create a particular type of texture-updater. If LTH is changed shouldn't texture-updater be recreated/updated as well?
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp#L174
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp#L126

[James Robinson]

The LTH isn't changed for Image or Content layers.

[Alok Priyadarshi]

(In reply to comment #29)
> The LTH isn't changed for Image or Content layers.

Ah. I was working under a different assumption given your earlier comment:
https://bugs.webkit.org/show_bug.cgi?id=77135#c9

LGTM then. It would be nice to enforce this assumption via an ASSERT though.

[James Robinson]

(In reply to comment #30)
> (In reply to comment #29)
> > The LTH isn't changed for Image or Content layers.
> 
> Ah. I was working under a different assumption given your earlier comment:
> https://bugs.webkit.org/show_bug.cgi?id=77135#c9
> 
> LGTM then. It would be nice to enforce this assumption via an ASSERT though.

Sorry for the confusion - in general layers can change LTH, but in the particular case of Image and Content this doesn't happen.

[James Robinson]

Created attachment 125733 [details]
Patch

[James Robinson]

Updating to get EWS runs since the dependent patch has landed and this applies cleanly to ToT.

[Vangelis Kokkevis]

Comment on attachment 125733 [details]
Patch

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

Somewhat tangentially related to this CL but can we also remove ImageLayerChromium::createTexture() .  I don't think it's being used anywhere (I was trying to see if there are any other ManagedTextures that would require notification when the LTH changes and found that method).

> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:66
> +    if (manager == m_textureManager)

Doesn't this suffer from the same pointer equality issue you mentioned in a previous review?  If there's a new LTH that happens to create a TextureManager at the same address as the old one, then things will go out of whack.  The sequence would be:

Layer->setLTH(LTH1)   : managed texture's manager is LTH1->m_contentsManager
Layer->setLTH(0)      : managed texture's manager is still LTH1->m_contentsManager
Destroy LTH1
Create LTH2 .  It just so happens that LTH2->m_contentsManager == LTH1->m_contentsManager
Layer->setLTH(LTH2)   : managed texture won't know that it really moved to a new manager.

[James Robinson]

(In reply to comment #34)
> (From update of attachment 125733 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125733&action=review
> 
> Somewhat tangentially related to this CL but can we also remove ImageLayerChromium::createTexture() .  I don't think it's being used anywhere (I was trying to see if there are any other ManagedTextures that would require notification when the LTH changes and found that method).
> 
> > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:66
> > +    if (manager == m_textureManager)
> 
> Doesn't this suffer from the same pointer equality issue you mentioned in a previous review?  If there's a new LTH that happens to create a TextureManager at the same address as the old one, then things will go out of whack.  The sequence would be:
> 
> Layer->setLTH(LTH1)   : managed texture's manager is LTH1->m_contentsManager
> Layer->setLTH(0)      : managed texture's manager is still LTH1->m_contentsManager
> Destroy LTH1

This step will also destroy LTH1->m_contentsTextureManager, and ~TextureManager will call clearManager() on every ManagedTexture referring to the TextureManager setting their m_textureManager pointer to 0

> Create LTH2 .  It just so happens that LTH2->m_contentsManager == LTH1->m_contentsManager
> Layer->setLTH(LTH2)   : managed texture won't know that it really moved to a new manager.

This is fine since ManagedTexture will compare the new TextureManager pointer to 0 and see that it's different.

[Vangelis Kokkevis]

(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 125733 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=125733&action=review
> > 
> > Somewhat tangentially related to this CL but can we also remove ImageLayerChromium::createTexture() .  I don't think it's being used anywhere (I was trying to see if there are any other ManagedTextures that would require notification when the LTH changes and found that method).
> > 
> > > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:66
> > > +    if (manager == m_textureManager)
> > 
> > Doesn't this suffer from the same pointer equality issue you mentioned in a previous review?  If there's a new LTH that happens to create a TextureManager at the same address as the old one, then things will go out of whack.  The sequence would be:
> > 
> > Layer->setLTH(LTH1)   : managed texture's manager is LTH1->m_contentsManager
> > Layer->setLTH(0)      : managed texture's manager is still LTH1->m_contentsManager
> > Destroy LTH1
> 
> This step will also destroy LTH1->m_contentsTextureManager, and ~TextureManager will call clearManager() on every ManagedTexture referring to the TextureManager setting their m_textureManager pointer to 0
> 
> > Create LTH2 .  It just so happens that LTH2->m_contentsManager == LTH1->m_contentsManager
> > Layer->setLTH(LTH2)   : managed texture won't know that it really moved to a new manager.
> 
> This is fine since ManagedTexture will compare the new TextureManager pointer to 0 and see that it's different.

Ah, of course.. You're right. Nevermind!

[Kenneth Russell]

Comment on attachment 125733 [details]
Patch

Nice. rs=me

[James Robinson]

Committed r106891: <http://trac.webkit.org/changeset/106891>