Bug 66065 - [chromium] Split tiler into main thread / compositor thread versions
Summary: [chromium] Split tiler into main thread / compositor thread versions
Status: RESOLVED FIXED
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:
Depends on: 58834
Blocks: 66430 66609
  Show dependency treegraph
 
Reported: 2011-08-11 09:17 PDT by Adrienne Walker
Modified: 2011-08-19 16:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (81.07 KB, patch)
2011-08-11 09:20 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (101.43 KB, patch)
2011-08-12 21:42 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (98.78 KB, patch)
2011-08-17 16:26 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (98.77 KB, patch)
2011-08-17 16:30 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (98.53 KB, patch)
2011-08-18 12:01 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (98.52 KB, patch)
2011-08-18 13:40 PDT, Adrienne Walker
jamesr: review+
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-08-11 09:17:21 PDT
[chromium] Split tiler into main thread / compositor thread versions
Comment 1 Adrienne Walker 2011-08-11 09:20:43 PDT
Created attachment 103634 [details]
Patch
Comment 2 Adrienne Walker 2011-08-11 09:27:10 PDT
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.
Comment 3 James Robinson 2011-08-11 10:59:28 PDT
Please preserve http://trac.webkit.org/changeset/92867, wherever that code ends up.
Comment 4 Adrienne Walker 2011-08-11 11:02:46 PDT
(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.
Comment 5 Adrienne Walker 2011-08-12 21:42:33 PDT
Created attachment 103855 [details]
Patch
Comment 6 James Robinson 2011-08-15 15:53:10 PDT
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
Comment 7 Adrienne Walker 2011-08-15 16:18:37 PDT
(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.
Comment 8 James Robinson 2011-08-15 16:24:04 PDT
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....
Comment 9 Nat Duca 2011-08-15 16:27:51 PDT
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;
}
Comment 10 Adrienne Walker 2011-08-15 16:42:24 PDT
(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.
Comment 11 Adrienne Walker 2011-08-15 16:44:39 PDT
(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.
Comment 12 James Robinson 2011-08-15 17:49:57 PDT
(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.
Comment 13 James Robinson 2011-08-15 17:50:40 PDT
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.
Comment 14 Adrienne Walker 2011-08-17 16:26:59 PDT
Created attachment 104276 [details]
Patch
Comment 15 Adrienne Walker 2011-08-17 16:30:52 PDT
Created attachment 104278 [details]
Patch
Comment 16 Adrienne Walker 2011-08-17 16:31:28 PDT
(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.
Comment 17 James Robinson 2011-08-17 16:58:21 PDT
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.
Comment 18 Adrienne Walker 2011-08-17 17:14:54 PDT
(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.
Comment 19 James Robinson 2011-08-17 17:16:49 PDT
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()
Comment 20 James Robinson 2011-08-17 17:19:43 PDT
(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.
Comment 21 James Robinson 2011-08-17 19:47:12 PDT
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.
Comment 22 Adrienne Walker 2011-08-18 10:40:17 PDT
(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.
Comment 23 Adrienne Walker 2011-08-18 12:01:09 PDT
Created attachment 104376 [details]
Patch
Comment 24 Adrienne Walker 2011-08-18 12:03:48 PDT
(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.
Comment 25 Adrienne Walker 2011-08-18 13:40:50 PDT
Created attachment 104390 [details]
Patch
Comment 26 Adrienne Walker 2011-08-18 13:44:23 PDT
(In reply to comment #25)
> Created an attachment (id=104390) [details]
> Patch

Rebaselined.
Comment 27 James Robinson 2011-08-18 16:03:27 PDT
Comment on attachment 104390 [details]
Patch

Great, R=me.  I wish that generally we had better ways of detecting when we were leaking resources.
Comment 28 Adrienne Walker 2011-08-19 11:07:49 PDT
Committed r93424: <http://trac.webkit.org/changeset/93424>
Comment 29 Tony Chang 2011-08-19 16:22:31 PDT
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?