Bug 76740 - [Chromium] Avoid unnecessary full tile updates without breaking atomicity of commits.
Summary: [Chromium] Avoid unnecessary full tile updates without breaking atomicity of ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on: 72672
Blocks: 77376
  Show dependency treegraph
 
Reported: 2012-01-20 14:31 PST by David Reveman
Modified: 2012-02-08 21:26 PST (History)
4 users (show)

See Also:


Attachments
Patch (25.66 KB, patch)
2012-01-23 17:45 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (25.67 KB, patch)
2012-01-30 10:16 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (26.59 KB, patch)
2012-01-30 16:30 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (34.68 KB, patch)
2012-02-01 10:39 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (35.03 KB, patch)
2012-02-07 16:13 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (35.40 KB, patch)
2012-02-07 17:53 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (35.76 KB, patch)
2012-02-08 14:20 PST, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2012-01-20 14:31:40 PST
https://bugs.webkit.org/show_bug.cgi?id=72672 makes commits atomic by performing full tile updates into textures not currently used by compositor thread. The final batch of texture uploads can be performed without this requirement and such an optimization is very useful as a significant portion of commits are small enough to fit in one texture upload batch.
Comment 1 David Reveman 2012-01-23 17:45:24 PST
Created attachment 123674 [details]
Patch
Comment 2 Dana Jansens 2012-01-30 09:59:34 PST
Comment on attachment 123674 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:56
> +    explicit UpdatableTile(PassOwnPtr<LayerTextureUpdater::Texture> texture) : m_partialUpdate(0), m_texture(texture) { }

is 0 right for a bool?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1301
> +        // Number of textures used for draw should always be one.

nit: two.
Comment 3 David Reveman 2012-01-30 10:16:58 PST
Created attachment 124567 [details]
Patch
Comment 4 David Reveman 2012-01-30 10:20:03 PST
(In reply to comment #2)
> (From update of attachment 123674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123674&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:56
> > +    explicit UpdatableTile(PassOwnPtr<LayerTextureUpdater::Texture> texture) : m_partialUpdate(0), m_texture(texture) { }
> 
> is 0 right for a bool?

fixed.

> 
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1301
> > +        // Number of textures used for draw should always be one.
> 
> nit: two.

fixed.

Thanks!
Comment 5 Dana Jansens 2012-01-30 14:16:19 PST
Comment on attachment 123674 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:87
>  

So we don't forget: If the number of updates including partial updates in this batch exceeds the regular maximum number per-batch, early out to push all the partial updates off to a next batch.
Comment 6 Adrienne Walker 2012-01-30 15:45:46 PST
Comment on attachment 124567 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:70
>      IntRect m_updateRect;
> +    bool m_partialUpdate;

It seems like this boolean is redundant to m_updateRect.  m_updateRect is the not the full tile rect iff it's a partial update, yeah?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:400
> +            // Partial update doesn't apply to new textures.
> +            if (!tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat)) {
> +                partialUpdate = false;
> +                newTexture = false;
> +            }

nit: Can you call newTexture something clearer? It's kind of non-intuitive to read this code and see the new texture case set newTexture to false.
Comment 7 Dana Jansens 2012-01-30 15:48:07 PST
Comment on attachment 124567 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:70
>> +    bool m_partialUpdate;
> 
> It seems like this boolean is redundant to m_updateRect.  m_updateRect is the not the full tile rect iff it's a partial update, yeah?

maybe m_partialUpdate is a bad name? the LTH can apply restrictions to allow a limited number of partial updates to actually be done without double buffering. maybe m_doubleBufferedUpdate or something?
Comment 8 David Reveman 2012-01-30 16:25:16 PST
(In reply to comment #6)
> (From update of attachment 124567 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124567&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:70
> >      IntRect m_updateRect;
> > +    bool m_partialUpdate;
> 
> It seems like this boolean is redundant to m_updateRect.  m_updateRect is the not the full tile rect iff it's a partial update, yeah?

No, it's only a partial update if requestPartialTextureUpdate() returned true. I think Dana is right, this variable needs a better name.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:400
> > +            // Partial update doesn't apply to new textures.
> > +            if (!tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat)) {
> > +                partialUpdate = false;
> > +                newTexture = false;
> > +            }
> 
> nit: Can you call newTexture something clearer? It's kind of non-intuitive to read this code and see the new texture case set newTexture to false.

Will do.
Comment 9 David Reveman 2012-01-30 16:30:51 PST
Created attachment 124625 [details]
Patch
Comment 10 David Reveman 2012-01-30 16:34:22 PST
Latest patch makes sure partial updates are done separately. I'll fix the m_partialUpdate variable name tomorrow. In general this patch would be much cleaner if https://bugs.webkit.org/show_bug.cgi?id=72752 landed prior to it.
Comment 11 James Robinson 2012-01-30 16:40:52 PST
I don't think landing https://bugs.webkit.org/show_bug.cgi?id=72752 first is a good idea. https://bugs.webkit.org/show_bug.cgi?id=72752 will regress performance on the platforms where we need incremental uploads the most.
Comment 12 Dana Jansens 2012-01-30 17:37:52 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=124625&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:89
> +    if (m_partialEntries.size() && index != m_entryIndex)

What about letting partial updates go on if they fit inside count? So 5-1-4 could become 5-5 at least?
Comment 13 David Reveman 2012-02-01 10:39:28 PST
Created attachment 124969 [details]
Patch
Comment 14 David Reveman 2012-02-01 10:45:30 PST
(In reply to comment #12)
> View in context: https://bugs.webkit.org/attachment.cgi?id=124625&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:89
> > +    if (m_partialEntries.size() && index != m_entryIndex)
> 
> What about letting partial updates go on if they fit inside count? So 5-1-4 could become 5-5 at least?

Fixed in latest patch. Also added a unit test to verify this behavior.

I kept the m_partialUpdate variable name and instead restructured the code and added some comments to TiledLayerChromium.cpp which I hope makes things clear.
Comment 15 David Reveman 2012-02-01 10:46:27 PST
(In reply to comment #11)
> I don't think landing https://bugs.webkit.org/show_bug.cgi?id=72752 first is a good idea. https://bugs.webkit.org/show_bug.cgi?id=72752 will regress performance on the platforms where we need incremental uploads the most.

That's fine. I'll make sure to clean this up after 72752 lands.
Comment 16 David Reveman 2012-02-07 16:13:25 PST
Created attachment 125950 [details]
Patch
Comment 17 David Reveman 2012-02-07 17:53:21 PST
Created attachment 125969 [details]
Patch

Remove 'using namespace std' from header and use std::numeric_limits::max() instead of ~0
Comment 18 James Robinson 2012-02-08 12:26:08 PST
Comment on attachment 125969 [details]
Patch

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

This looks good, but I'd really like to see some of the stuff in TiledLayerChromium::prepareToUpdateTiles() split out. Lots of nits but content looks good.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:56
> +    explicit UpdatableTile(PassOwnPtr<LayerTextureUpdater::Texture> texture) : m_partialUpdate(false), m_texture(texture) { }

one statement per line. expand this initialization out.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:380
> +            bool partialUpdate = false;

this is a lot of lines of code to add to an already large and hard-to-follow function. can you break any of this out into a helper?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:384
> +                // For tiles with valid textures, new textures need to be

the excessive line wrapping here makes this comment hard for me to read

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:400
> +                ASSERT(layerTreeHost());

this ASSERT() isn't super valuable - we can never enter this function with a null LTH

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:405
> +                // TODO(reveman): Decide if partial update should be allowed

webkit style is FIXME, not TODO(name)

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:417
> +                ASSERT(layerTreeHost());

ASSERT() not helpful

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:418
> +                ASSERT(tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat));

this is just ASSERT()ing on logic 20 lines up. maybe if this was a helper function it'd be easier to structure the code so that things like this are more obvious?

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:63
> +        Texture(FakeLayerTextureUpdater* layer, PassOwnPtr<ManagedTexture> texture) : LayerTextureUpdater::Texture(texture), m_layer(layer) { }

one statement per line, so expand initialization

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:72
> +    FakeLayerTextureUpdater() : m_prepareCount(0), m_updateCount(0) { }

expand
Comment 19 David Reveman 2012-02-08 14:19:46 PST
Thanks for looking at this.

(In reply to comment #18)
> (From update of attachment 125969 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125969&action=review
> 
> This looks good, but I'd really like to see some of the stuff in TiledLayerChromium::prepareToUpdateTiles() split out. Lots of nits but content looks good.
> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:56
> > +    explicit UpdatableTile(PassOwnPtr<LayerTextureUpdater::Texture> texture) : m_partialUpdate(false), m_texture(texture) { }
> 
> one statement per line. expand this initialization out.

fixed.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:380
> > +            bool partialUpdate = false;
> 
> this is a lot of lines of code to add to an already large and hard-to-follow function. can you break any of this out into a helper?

let me know what you think about the updated patch.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:384
> > +                // For tiles with valid textures, new textures need to be
> 
> the excessive line wrapping here makes this comment hard for me to read

I know there's no 80 column max in the webkit style but I can't make myself do that :) The new patch shouldn't have this problem.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:400
> > +                ASSERT(layerTreeHost());
> 
> this ASSERT() isn't super valuable - we can never enter this function with a null LTH

TiledLayerChromiumTest.pushDirtyTiles causes this to be entered with a null LTH somehow. Maybe just improper use by the unit test but I left the layerTreeHost() checks in the patch for now..

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:405
> > +                // TODO(reveman): Decide if partial update should be allowed
> 
> webkit style is FIXME, not TODO(name)

fixed.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:417
> > +                ASSERT(layerTreeHost());
> 
> ASSERT() not helpful

removed from latest patch.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:418
> > +                ASSERT(tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat));
> 
> this is just ASSERT()ing on logic 20 lines up. maybe if this was a helper function it'd be easier to structure the code so that things like this are more obvious?

removed from latest patch.

> 
> > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:63
> > +        Texture(FakeLayerTextureUpdater* layer, PassOwnPtr<ManagedTexture> texture) : LayerTextureUpdater::Texture(texture), m_layer(layer) { }
> 
> one statement per line, so expand initialization

fixed.

> 
> > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:72
> > +    FakeLayerTextureUpdater() : m_prepareCount(0), m_updateCount(0) { }
> 
> expand

fixed.
Comment 20 David Reveman 2012-02-08 14:20:04 PST
Created attachment 126152 [details]
Patch
Comment 21 Dana Jansens 2012-02-08 14:30:05 PST
Comment on attachment 126152 [details]
Patch

I like :)
Comment 22 James Robinson 2012-02-08 15:38:59 PST
Comment on attachment 126152 [details]
Patch

OK cool, the small functions make that bit a lot easier to read IMO.
Comment 23 WebKit Review Bot 2012-02-08 21:26:13 PST
Comment on attachment 126152 [details]
Patch

Clearing flags on attachment: 126152

Committed r107177: <http://trac.webkit.org/changeset/107177>
Comment 24 WebKit Review Bot 2012-02-08 21:26:19 PST
All reviewed patches have been landed.  Closing bug.