Bug 66065 – [chromium] Split tiler into main thread / compositor thread versions

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

[Adrienne Walker]

[chromium] Split tiler into main thread / compositor thread versions

[Adrienne Walker]

Created attachment 103634 [details]
Patch

[Adrienne Walker]

This isn't a real patch, but it's a strawperson attempt at what splitting the tiler into an main thread (invalidation) and compositor thread (upload and drawing) version might look like.

There's a number of FIXME's in this patch that I'm going to file as dependent patches so that the actual split can involve as little functionality change as possible.

[James Robinson]

Please preserve http://trac.webkit.org/changeset/92867, wherever that code ends up.

[Adrienne Walker]

(In reply to comment #3)
> Please preserve http://trac.webkit.org/changeset/92867, wherever that code ends up.

Oh, I figure there's going to be a boatload of conflicts.  That's why I want to land the functionality changes separately and then maybe just re-do the code moving around with wherever ToT is at that point.

[Adrienne Walker]

Created attachment 103855 [details]
Patch

[James Robinson]

Comment on attachment 103855 [details]
Patch

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

I'm not sure about having two subclasses of LayerTilerChromium.  It feels like this is more of a has-a relationship than an is-a relationship.  Could TiledLayerChromium and CCTiledLayerImpl both own a LayerTilerChromium and have the pushPropertiesTo() function do a copy across the two?  It seems like there isn't all that much state we need to hoist over (the tile map, which we're explicitly syncing, and a half-dozen properties).  I really feel that LayerTilerChromium should be an implementation detail of TiledLayerChromium and not part of the API.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:67
> +        explicit Tile() : m_i(-1), m_j(-1) { }

nit: no explicit necessary

> Source/WebCore/platform/graphics/chromium/LayerTilerUpdater.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

times they are a'changing. 2011

[Adrienne Walker]

(In reply to comment #6)
> (From update of attachment 103855 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103855&action=review
> 
> I'm not sure about having two subclasses of LayerTilerChromium.  It feels like this is more of a has-a relationship than an is-a relationship.  Could TiledLayerChromium and CCTiledLayerImpl both own a LayerTilerChromium and have the pushPropertiesTo() function do a copy across the two?  It seems like there isn't all that much state we need to hoist over (the tile map, which we're explicitly syncing, and a half-dozen properties).  I really feel that LayerTilerChromium should be an implementation detail of TiledLayerChromium and not part of the API.

I'm ok with a has-a relationship between LayerTilerChromium and LayerTilerUpdater/CClayerTilerImpl.  That's much cleaner.

However, unless you see some way to make the root layer into a LayerChromium, I'm not sure how to reuse the tiler logic for the root as well.  Pushing the LayerTilerChromium-derived logic up into the respective layer classes would make this tricky, unless I'm missing something.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:67
> > +        explicit Tile() : m_i(-1), m_j(-1) { }
> 
> nit: no explicit necessary

Good catch.  I had removed a parameter there.

[James Robinson]

Yeah the root layer stuff sucks.  That's what https://bugs.webkit.org/show_bug.cgi?id=58834 is hoping to fix.

Maybe you can explicitly make a TiledLayerChromium for the root content if you hooked up the proper client and such?  Gets very yucky.  Lemme see if I can just fix the scissor stuff so this is all in the LayerChromium tree....

[Nat Duca]

Comment on attachment 103855 [details]
Patch

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

This patch makes me very :3. Thank you Enne.

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp:74
> +void LayerTilerChromium::pushPropertiesTo(LayerTilerChromium* tiler) const

This sort of caught me off guard, I was expecting a CCLayerTilerImpl* here... the hazards perhaps of reading a patch from top to bottom?

> Source/WebCore/platform/graphics/chromium/LayerTilerUpdater.cpp:5
> + * modification, are permitted provided that the following conditions

Is this a mainthread concept right?

Should we be putting new compositor files in cc/ with the prefix? Iirw, all files eventually go up in cc/ that are compositor related... the only reason things are in platform/graphics/chromium is to avoid complications from renaming mid-development.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilerImpl.h:41
> +class CCLayerTilerImpl : public LayerTilerChromium {

Recapping IM discussion, can we make CCLayerTilerImpl and LayerTiler not share any common base class? I think there's goodness that comes from it being impossible to cast between impl and mainthread representation. Maybe have a LayerTilerData class for common data, and keep it as a member that you push...

LayerTilerData {
  ... common math, state that both threads have
}

LayerTilerChromium {
  PushPropertiesTo(CCLayerTilerImpl*)
  {
    impl->SetCommonData(m_data);
  }
  LayerTilerData m_data;
}

[Adrienne Walker]

(In reply to comment #8)
> Yeah the root layer stuff sucks.  That's what https://bugs.webkit.org/show_bug.cgi?id=58834 is hoping to fix.

Maybe I'll just wait for that patch then if the root gets a tiled layer.  Trying to do that myself would likely just end up with me writing the same patch you're writing.

So, once that lands, I could rename LayerTilerChromium (in this original patch) to LayerTilingData and have TiledLayerChromium and CCTiledLayerImpl both have-a LayerTilingData.

[Adrienne Walker]

(In reply to comment #10)
> (In reply to comment #8)
> > Yeah the root layer stuff sucks.  That's what https://bugs.webkit.org/show_bug.cgi?id=58834 is hoping to fix.
> 
> Maybe I'll just wait for that patch then if the root gets a tiled layer.  Trying to do that myself would likely just end up with me writing the same patch you're writing.
> 
> So, once that lands, I could rename LayerTilerChromium (in this original patch) to LayerTilingData and have TiledLayerChromium and CCTiledLayerImpl both have-a LayerTilingData.

Er, CCLayerTilingData, I mean.

[James Robinson]

(In reply to comment #10)
> (In reply to comment #8)
> > Yeah the root layer stuff sucks.  That's what https://bugs.webkit.org/show_bug.cgi?id=58834 is hoping to fix.
> 
> Maybe I'll just wait for that patch then if the root gets a tiled layer.  Trying to do that myself would likely just end up with me writing the same patch you're writing.
> 
> So, once that lands, I could rename LayerTilerChromium (in this original patch) to LayerTilingData and have TiledLayerChromium and CCTiledLayerImpl both have-a LayerTilingData.

Exactly.  I'll try to resolve 58843 ASAP.

[James Robinson]

Comment on attachment 103855 [details]
Patch

r-, gonna wait for the root layer to get sorted out then change the is-a in this patch to a has-a relationship.

[Adrienne Walker]

Created attachment 104276 [details]
Patch

[Adrienne Walker]

Created attachment 104278 [details]
Patch

[Adrienne Walker]

(In reply to comment #15)
> Created an attachment (id=104278) [details]
> Patch

This patch is built on top of the patch from 58834.  I finished re-re-refactoring based on the above comments, so I'm uploading another stab at this.

[James Robinson]

Comment on attachment 104278 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:53
> +    UpdatableTile(PassOwnPtr<ManagedTexture> tex) : m_tex(tex) { }

need "explicit" as in "c++ is an <explicit word here> language"

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:258
> +    for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) {

how can we be sure that this will hit every tile on tiledLayer?  I'm a little worried that when we start scrolling on the CCTiledLayerImpl side we could pretty easily miss some tiles in this iteration.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:327
> +        m_unusedTiles.append(static_pointer_cast<UpdatableTile>(m_tiler->takeTile(removeKeys[i].first, removeKeys[i].second)));

static_pointer_cast<>? I'm not familiar with that, where do we pick it up from?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:86
> +DrawableTile* CCTiledLayerImpl::tileAt(int i, int j) const
> +{
> +    return static_cast<DrawableTile*>(m_tiler->tileAt(i, j));
> +}

i wonder if instead of static_cast<>'ing to Tile subclasses if it'd be better to templatize CCTiledLayerData on the Tile subclass.  You'd have to move the implementation of tileAt() into the header, but would it be too much? When we make a CCTiledLayerData we know what kind of Tile we want to put into it and we never mix.

[Adrienne Walker]

(In reply to comment #17)
> (From update of attachment 104278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104278&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:53
> > +    UpdatableTile(PassOwnPtr<ManagedTexture> tex) : m_tex(tex) { }
> 
> need "explicit" as in "c++ is an <explicit word here> language"

Done.

> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:258
> > +    for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) {
> 
> how can we be sure that this will hit every tile on tiledLayer?  I'm a little worried that when we start scrolling on the CCTiledLayerImpl side we could pretty easily miss some tiles in this iteration.

I'm not sure what your worry is here.  That loop will iterate through everything in the hash map.

> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:327
> > +        m_unusedTiles.append(static_pointer_cast<UpdatableTile>(m_tiler->takeTile(removeKeys[i].first, removeKeys[i].second)));
> 
> static_pointer_cast<>? I'm not familiar with that, where do we pick it up from?

It comes from RefPtr.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> 2011

Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:86
> > +DrawableTile* CCTiledLayerImpl::tileAt(int i, int j) const
> > +{
> > +    return static_cast<DrawableTile*>(m_tiler->tileAt(i, j));
> > +}
> 
> i wonder if instead of static_cast<>'ing to Tile subclasses if it'd be better to templatize CCTiledLayerData on the Tile subclass.  You'd have to move the implementation of tileAt() into the header, but would it be too much? When we make a CCTiledLayerData we know what kind of Tile we want to put into it and we never mix.

I went back and forth on that one and didn't really feel strongly one way or the other.  I'm happy to change it to use templates.

[James Robinson]

Comment on attachment 104278 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:63
> +private:
> +    OwnPtr<ManagedTexture> m_tex;

I think you need a virtual destructor here or when a UpdatableTile is destroyed through a Tile* pointer this ~OwnPtr<> won't get called

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.h:68
> +    class Tile : public RefCounted<Tile> {
> +        WTF_MAKE_NONCOPYABLE(Tile);
> +    public:
> +        Tile() : m_i(-1), m_j(-1) { }

yeah this seems like a problem - this map holds a set of RefPtr<Tile>, but there's no virtual d'tor declared so when a Tile is destroyed through this path it won't invoke ~UpdatableTile()

[James Robinson]

(In reply to comment #18)
> > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:258
> > > +    for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) {
> > 
> > how can we be sure that this will hit every tile on tiledLayer?  I'm a little worried that when we start scrolling on the CCTiledLayerImpl side we could pretty easily miss some tiles in this iteration.
> 
> I'm not sure what your worry is here.  That loop will iterate through everything in the hash map.

It iterates through the TiledLayerChromium's m_tiler::TileMap. My concern is that there might be entries in the CCTiledLayerImpl's m_tiler::TileMap that aren't in this one - maybe that's just a theoretical concern at this point.

> 
> > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:327
> > > +        m_unusedTiles.append(static_pointer_cast<UpdatableTile>(m_tiler->takeTile(removeKeys[i].first, removeKeys[i].second)));
> > 
> > static_pointer_cast<>? I'm not familiar with that, where do we pick it up from?
> 
> It comes from RefPtr.
> 

Interesting.  Looks like the idea is from boost - learn something new every day.

> 
> > > Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:86
> > > +DrawableTile* CCTiledLayerImpl::tileAt(int i, int j) const
> > > +{
> > > +    return static_cast<DrawableTile*>(m_tiler->tileAt(i, j));
> > > +}
> > 
> > i wonder if instead of static_cast<>'ing to Tile subclasses if it'd be better to templatize CCTiledLayerData on the Tile subclass.  You'd have to move the implementation of tileAt() into the header, but would it be too much? When we make a CCTiledLayerData we know what kind of Tile we want to put into it and we never mix.
> 
> I went back and forth on that one and didn't really feel strongly one way or the other.  I'm happy to change it to use templates.

Your call.  I think you'll need to add a virtual d'tor with this pattern but that's not huge.

[James Robinson]

Comment on attachment 104278 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.h:68
>> +        Tile() : m_i(-1), m_j(-1) { }
> 
> yeah this seems like a problem - this map holds a set of RefPtr<Tile>, but there's no virtual d'tor declared so when a Tile is destroyed through this path it won't invoke ~UpdatableTile()

Yeah this is definitely a real bug, so R- because of that.  Otherwise I think this patch looks pretty great.

[Adrienne Walker]

(In reply to comment #21)
> (From update of attachment 104278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104278&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.h:68
> >> +        Tile() : m_i(-1), m_j(-1) { }
> > 
> > yeah this seems like a problem - this map holds a set of RefPtr<Tile>, but there's no virtual d'tor declared so when a Tile is destroyed through this path it won't invoke ~UpdatableTile()
> 
> Yeah this is definitely a real bug, so R- because of that.  Otherwise I think this patch looks pretty great.

Good catch.  :)

I'll take a closer look at what templatifying this code would look like and will upload another patch.

[Adrienne Walker]

Created attachment 104376 [details]
Patch

[Adrienne Walker]

(In reply to comment #23)
> Created an attachment (id=104376) [details]
> Patch

Adding templates subtracted a lot from readability in my opinion.  I did go add the virtual destructor and address the other review nits.

Also, the worry about iterating through tiles that weren't there was spot on.  If a tile got removed from the TiledLayerChromium side, there was nothing that would evict it from the other.  So, during the setTilerProperties function, I now just reset the CCTiledLayerImpl version so that it will be synced exactly.

[Adrienne Walker]

Created attachment 104390 [details]
Patch

[Adrienne Walker]

(In reply to comment #25)
> Created an attachment (id=104390) [details]
> Patch

Rebaselined.

[James Robinson]

Comment on attachment 104390 [details]
Patch

Great, R=me.  I wish that generally we had better ways of detecting when we were leaking resources.

[Adrienne Walker]

Committed r93424: <http://trac.webkit.org/changeset/93424>

[Tony Chang]

Comment on attachment 104390 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:67
> +    void invalidateRect(const IntRect& contentRect);

clang doesn't like this method because the parent class (LayerChromium) has "virtual void invalidateRect(const FloatRect& dirtyRect) {}".  Should we rename the method here or did you mean to override the parent method?