Bug 76740 – [Chromium] Avoid unnecessary full tile updates without breaking atomicity of commits.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

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.

[David Reveman]

Created attachment 123674 [details]
Patch

[Dana Jansens]

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.

[David Reveman]

Created attachment 124567 [details]
Patch

[David Reveman]

(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!

[Dana Jansens]

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.

[Adrienne Walker]

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.

[Dana Jansens]

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?

[David Reveman]

(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.

[David Reveman]

Created attachment 124625 [details]
Patch

[David Reveman]

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.

[James Robinson]

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.

[Dana Jansens]

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?

[David Reveman]

Created attachment 124969 [details]
Patch

[David Reveman]

(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.

[David Reveman]

(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.

[David Reveman]

Created attachment 125950 [details]
Patch

[David Reveman]

Created attachment 125969 [details]
Patch

Remove 'using namespace std' from header and use std::numeric_limits::max() instead of ~0

[James Robinson]

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

[David Reveman]

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.

[David Reveman]

Created attachment 126152 [details]
Patch

[Dana Jansens]

Comment on attachment 126152 [details]
Patch

I like :)

[James Robinson]

Comment on attachment 126152 [details]
Patch

OK cool, the small functions make that bit a lot easier to read IMO.

[WebKit Review Bot]

Comment on attachment 126152 [details]
Patch

Clearing flags on attachment: 126152

Committed r107177: <http://trac.webkit.org/changeset/107177>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.