Bug 82057 – [chromium] Record painted pixel counts for measuring effectiveness of per-tile painting

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

[Dana Jansens]

[chromium] Record invalidated vs painted pixel counts for measuring effectiveness of per-tile painting

[Dana Jansens]

Created attachment 133494 [details]
Patch

[Dana Jansens]

This goes out to all the peeps in Boston.

[David Reveman]

Comment on attachment 133494 [details]
Patch

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

Dana, thanks for taking care of this!

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:482
> +        occlusion->overdrawMetrics().didPaint(m_paintRect);

Rather than reporting the paint rectangle here, could we have the PixelsPainted metric reflect the pixels that were actually painted by the texture updater? So if I turn on per-tile painting the PixelsPainted metric would go down. For BitmapCanvasLayerTextureUpdater report the area painted in LayerTextureUpdate::prepareToUpdate and for BitmapSkPictureCanvasLayerTextureUpdater instead report the playback areas in LayerTextureUpdate::prepareRect.

What do you think?

[Dana Jansens]

Comment on attachment 133494 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:482
>> +        occlusion->overdrawMetrics().didPaint(m_paintRect);
> 
> Rather than reporting the paint rectangle here, could we have the PixelsPainted metric reflect the pixels that were actually painted by the texture updater? So if I turn on per-tile painting the PixelsPainted metric would go down. For BitmapCanvasLayerTextureUpdater report the area painted in LayerTextureUpdate::prepareToUpdate and for BitmapSkPictureCanvasLayerTextureUpdater instead report the playback areas in LayerTextureUpdate::prepareRect.
> 
> What do you think?

That is covered by adding the pixels "drawn" (uploaded) transparent + opaque.

[Dana Jansens]

Unless the pixels painted by the updater, and uploaded, can differ?

[David Reveman]

Ah, I see. Yeah, it looks like this should do it! Could you add a unit test that uses explicit texture updater types to ensure that this works correctly?

[Dana Jansens]

Created attachment 133529 [details]
Patch

Added some comments to the overdraw metrics header. Especially to clarify that painted pixels != rasterized pixels, as we will be interested in tracking rasterized pixels in the future, in order to determine wins of different approaches to texture updating.

[David Reveman]

Comment on attachment 133529 [details]
Patch

LGTM

[Adrienne Walker]

Comment on attachment 133529 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:471
> +            if (occlusion)
> +                occlusion->overdrawMetrics().didInvalidate(tile->m_dirtyRect);
> +

I might just be bikeshedding here, but "invalidate" is kind of misleading.  It isn't "pixels invalidated" it's really "pixels within the visible layer rect that are not on tiles that are entirely occluded".  If anything, this value seems kind of redundant with the information that you're getting out of "total pixels drawn"?

[Dana Jansens]

Comment on attachment 133529 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:471
>> +
> 
> I might just be bikeshedding here, but "invalidate" is kind of misleading.  It isn't "pixels invalidated" it's really "pixels within the visible layer rect that are not on tiles that are entirely occluded".  If anything, this value seems kind of redundant with the information that you're getting out of "total pixels drawn"?

Yeh, I agree. I wanted the pixels that we "have to paint" vs "did paint", but it is the same as what we are uploading.

[Dana Jansens]

Created attachment 133553 [details]
Patch

[Dana Jansens]

Removed the invalidated pixel metric, just counting pixels painted by webkit.

[Adrienne Walker]

Comment on attachment 133553 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.cpp:151
> +    if (paintedCounterName) {
> +        // This must be in a different scope than the TRACE_EVENT2 above.
> +        TRACE_EVENT1("webkit", "CCOverdrawPaintMetrics", paintedCounterName, m_pixelsPainted);
> +    }

Why? Can you not TRACE_EVENT2 or TRACE_EVENT3, depending on which path you're in, so that all the values are grouped together?

[Dana Jansens]

(In reply to comment #13)
> (From update of attachment 133553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133553&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.cpp:151
> > +    if (paintedCounterName) {
> > +        // This must be in a different scope than the TRACE_EVENT2 above.
> > +        TRACE_EVENT1("webkit", "CCOverdrawPaintMetrics", paintedCounterName, m_pixelsPainted);
> > +    }
> 
> Why? Can you not TRACE_EVENT2 or TRACE_EVENT3, depending on which path you're in, so that all the values are grouped together?

There is currently no TRACE_EVENT3, though jbates has an open bug to allow more than 2 axes of data in a single trace/counter.

The scoping issue is because of compile errors, the macros expect only one such trace to exist within a scope.

[Adrienne Walker]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 133553 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133553&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.cpp:151
> > > +    if (paintedCounterName) {
> > > +        // This must be in a different scope than the TRACE_EVENT2 above.
> > > +        TRACE_EVENT1("webkit", "CCOverdrawPaintMetrics", paintedCounterName, m_pixelsPainted);
> > > +    }
> > 
> > Why? Can you not TRACE_EVENT2 or TRACE_EVENT3, depending on which path you're in, so that all the values are grouped together?
> 
> There is currently no TRACE_EVENT3, though jbates has an open bug to allow more than 2 axes of data in a single trace/counter.

Oh! My misunderstanding.

[WebKit Review Bot]

Comment on attachment 133553 [details]
Patch

Clearing flags on attachment: 133553

Committed r111913: <http://trac.webkit.org/changeset/111913>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.