[chromium] Record invalidated vs painted pixel counts for measuring effectiveness of per-tile painting
Created attachment 133494 [details] Patch
This goes out to all the peeps in Boston.
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?
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.
Unless the pixels painted by the updater, and uploaded, can differ?
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?
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.
Comment on attachment 133529 [details] Patch LGTM
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"?
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.
Created attachment 133553 [details] Patch
Removed the invalidated pixel metric, just counting pixels painted by webkit.
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?
(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.
(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.
Comment on attachment 133553 [details] Patch Clearing flags on attachment: 133553 Committed r111913: <http://trac.webkit.org/changeset/111913>
All reviewed patches have been landed. Closing bug.