Bug 90019 – [chromium] Add time spent painting to GPU benchmarking renderingStats() API.

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

[Dave Tu]

[chromium] Add time spent painting to GPU benchmarking renderingStats() API.

[Dave Tu]

Created attachment 149618 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 149618 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Tu]

Created attachment 149624 [details]
Patch

[James Robinson]

Comment on attachment 149624 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:471
>      updateLayers(rootLayer(), updater);
> +
> +    m_timeSpentPainting = monotonicallyIncreasingTime() - updateBeginTime;

updateLayers() covers more than just painting.  for instance, a canvas 2d layer will do a flush here, image layers will prepare texture uploads, WebGL layers will issue multisample resolves etc

[Nat Duca]

Comment on attachment 149624 [details]
Patch

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

There's an interesting question of what this should report in skpicture/impl-side-painting mode.

These are different quantities in that case:
- time spent painting
- time spent rasterizing

In the regular "rasterize as you go mode" (bitmapcanvaslayertextureupdater) mode, should we just say "rasterizing time is zero"?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:471
>> +    m_timeSpentPainting = monotonicallyIncreasingTime() - updateBeginTime;
> 
> updateLayers() covers more than just painting.  for instance, a canvas 2d layer will do a flush here, image layers will prepare texture uploads, WebGL layers will issue multisample resolves etc

You're clobbering this every frame. That means we can only get the time spent painting in the last frame.

Should we instead be accumulating to this value? E.g. so we track the time we've spent paiting ever? That way we can check it twice and from the deltas, get the real time spent painting.

And, if thats the case, should we be concerned about numeric precision as this number gets larger?

[Dave Tu]

(In reply to comment #6)
> (From update of attachment 149624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149624&action=review
> 
> There's an interesting question of what this should report in skpicture/impl-side-painting mode.
> 
> These are different quantities in that case:
> - time spent painting
> - time spent rasterizing
> 
> In the regular "rasterize as you go mode" (bitmapcanvaslayertextureupdater) mode, should we just say "rasterizing time is zero"?
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:471
> >> +    m_timeSpentPainting = monotonicallyIncreasingTime() - updateBeginTime;
> > 
> > updateLayers() covers more than just painting.  for instance, a canvas 2d layer will do a flush here, image layers will prepare texture uploads, WebGL layers will issue multisample resolves etc

So, "painting" is defined as specifically the GraphicsContext calls, and "rasterizing" includes the rest of all of that stuff?

> You're clobbering this every frame. That means we can only get the time spent painting in the last frame.
> 
> Should we instead be accumulating to this value? E.g. so we track the time we've spent paiting ever? That way we can check it twice and from the deltas, get the real time spent painting.

How can you ensure that you've checked it at adjacent frames? Maybe it should have both the total and the last one, or last ten?

> And, if thats the case, should we be concerned about numeric precision as this number gets larger?

No, double precision is far greater than the precision of the measurements, which is probably about millisecond-precision (depending on platform). But that leads to another question, are we okay with this level of precision on the individual measurements?

[Nat Duca]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 149624 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=149624&action=review
> > 
> > There's an interesting question of what this should report in skpicture/impl-side-painting mode.
> > 
> > These are different quantities in that case:
> > - time spent painting
> > - time spent rasterizing
> > 
> > In the regular "rasterize as you go mode" (bitmapcanvaslayertextureupdater) mode, should we just say "rasterizing time is zero"?
> > 
> > >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:471
> > >> +    m_timeSpentPainting = monotonicallyIncreasingTime() - updateBeginTime;
> > > 
> > > updateLayers() covers more than just painting.  for instance, a canvas 2d layer will do a flush here, image layers will prepare texture uploads, WebGL layers will issue multisample resolves etc
> 
> So, "painting" is defined as specifically the GraphicsContext calls, and "rasterizing" includes the rest of all of that stuff?

Painting is time spent calling webkit's paint method. Depending on what type of canvas we pass it, it could be either definition. You want to measure the time spent by us calling back to LayerPainterChromium's paint method.

> How can you ensure that you've checked it at adjacent frames? Maybe it should have both the total and the last one, or last ten?

One thing you could do is track the number of pixels painted. E.g. the cumulative area of the rectangles passed into paint. So we can get, e.g. megapixels/second instead of just paint time. That would be very useful.

If we need "exact time painting since last frame" we can use requestAnimationFrame, no?

> No, double precision is far greater than the precision of the measurements, which is probably about millisecond-precision (depending on platform). But that leads to another question, are we okay with this level of precision on the individual measurements?

monotonicallyIncreasingTime is accurate to microseconds. But, yes I'm happy with that. Paint times are in the tens to hundreds of ms per frame.

[Dave Tu]

Created attachment 152358 [details]
Patch

[Nat Duca]

Comment on attachment 152358 [details]
Patch

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

I'm happy. Enne?

> Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.cpp:-98
> -    SkPictureCanvasLayerTextureUpdater::prepareToUpdate(contentRect, tileSize, contentsWidthScale, contentsHeightScale, resultingOpaqueRect);

I think this class' prepareRect time spent in updater->paintContentsRect should increment timePainting. While you're at it, add a timeSpentRasterizing property to the stats that is separate that we bump when this class does drawPicture, lower in the file.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:218
> +    ++m_renderingStats.numAnimationFrames;

why ++x and not x++? Does webkit style guide advise on this?

[Adrienne Walker]

Comment on attachment 152358 [details]
Patch

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

I haven't looked much at rendering stats recently.  Do they ever get cleared, or do its values continually increase and the caller has to do any diff that they care about?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:218
>> +    ++m_renderingStats.numAnimationFrames;
> 
> why ++x and not x++? Does webkit style guide advise on this?

The style guide doesn't, but Chromium compositor code pretty consistently uses post-increment except in for loops.

[Dave Tu]

Created attachment 153376 [details]
Patch

[Dave Tu]

Comment on attachment 152358 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.cpp:-98
>> -    SkPictureCanvasLayerTextureUpdater::prepareToUpdate(contentRect, tileSize, contentsWidthScale, contentsHeightScale, resultingOpaqueRect);
> 
> I think this class' prepareRect time spent in updater->paintContentsRect should increment timePainting. While you're at it, add a timeSpentRasterizing property to the stats that is separate that we bump when this class does drawPicture, lower in the file.

Done. When I tested this, it doesn't go through BitmapSkPictureCanvasLayerTextureUpdater::prepareToUpdate at all. Are there chrome://flags that I should enable? And what should timeSpentRasterizing do when it's not enabled? Right now it just says 0.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:218
>>> +    ++m_renderingStats.numAnimationFrames;
>> 
>> why ++x and not x++? Does webkit style guide advise on this?
> 
> The style guide doesn't, but Chromium compositor code pretty consistently uses post-increment except in for loops.

Changed to post-increment.

[Dana Jansens]

Comment on attachment 152358 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.cpp:-98
>>> -    SkPictureCanvasLayerTextureUpdater::prepareToUpdate(contentRect, tileSize, contentsWidthScale, contentsHeightScale, resultingOpaqueRect);
>> 
>> I think this class' prepareRect time spent in updater->paintContentsRect should increment timePainting. While you're at it, add a timeSpentRasterizing property to the stats that is separate that we bump when this class does drawPicture, lower in the file.
> 
> Done. When I tested this, it doesn't go through BitmapSkPictureCanvasLayerTextureUpdater::prepareToUpdate at all. Are there chrome://flags that I should enable? And what should timeSpentRasterizing do when it's not enabled? Right now it just says 0.

--enable-per-tile-painting

[Adrienne Walker]

Comment on attachment 153376 [details]
Patch

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

Looks good to me in general.  Can you put up a new patch that applies to ToT?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderingStats.h:34
> +    double totalPaintTime;
> +    double totalRasterizeTime;

Can you add a comment here and in the other stats class about what the units of time are on these time variables? Alternatively rename them so they're more clear?

[Nat Duca]

Good point. Because of complications on double-rolling, I'd suggest comments for now.

[Dave Tu]

Created attachment 153622 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153622 [details]
Patch

Attachment 153622 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13301578

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-composite-modes.html
compositing/webgl/webgl-nonpremultiplied-blend.html
fast/loader/loadInProgress.html
platform/chromium/virtual/gpu/fast/canvas/canvas-resize-after-paint-without-layout.html
fast/loader/unload-form-post-about-blank.html
compositing/visibility/visibility-simple-canvas2d-layer.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-simple-canvas2d-layer.html
platform/chromium/virtual/gpu/fast/canvas/canvas-imageSmoothingEnabled-patterns.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/virtual/gpu/fast/canvas/canvas-transform-identity.html
platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-composite-modes-repaint.html
fast/canvas/webgl/webgl-composite-modes-repaint.html
TiledLayerChromiumTest.visibleContentOpaqueRegion
platform/chromium/virtual/gpu/fast/canvas/canvas-text-baseline.html
platform/chromium/virtual/gpu/fast/canvas/canvas-composite-fill-repaint.html
compositing/visibility/visibility-simple-webgl-layer.html
fast/frames/cached-frame-counter.html
compositing/webgl/webgl-background-color.html
platform/chromium/compositing/webgl-loses-compositor-context.html
TiledLayerChromiumTest.invalidateFromPrepare
platform/chromium/virtual/threaded/compositing/visibility/visibility-simple-webgl-layer.html
compositing/webgl/webgl-no-alpha.html
compositing/webgl/webgl-reflection.html
platform/chromium/virtual/gpu/fast/canvas/canvas-resize-reset.html
fast/canvas/webgl/webgl-composite-modes.html
platform/chromium/virtual/gpu/fast/canvas/canvas-empty-image-pattern.html
compositing/backface-visibility/backface-visibility-webgl.html
platform/chromium/virtual/gpu/fast/canvas/canvas-before-css.html

[WebKit Review Bot]

Created attachment 153641 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adrienne Walker]

Comment on attachment 153622 [details]
Patch

R=me.  Looks good to me.  If you double check that those layout test failures were spurious and also get somebody to approve the WebKit API changes (see comment #2), then this should be good to land.

[Dave Tu]

Created attachment 153930 [details]
Patch

[Adam Barth]

Comment on attachment 153930 [details]
Patch

API change LGTM

[Adam Barth]

Comment on attachment 153930 [details]
Patch

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

> Source/Platform/chromium/public/WebRenderingStats.h:35
> +    double totalPaintTime; // in seconds
> +    double totalRasterizeTime; // in seconds

nit: If I were writing this patch, I might have included the comment in the variable name:
totalPaintTimeInSeconds

[WebKit Review Bot]

Comment on attachment 153930 [details]
Patch

Rejecting attachment 153930 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/Platform/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13331204

[WebKit Review Bot]

Comment on attachment 153930 [details]
Patch

Attachment 153930 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13334177

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-composite-modes.html
fast/forms/range/slider-delete-while-dragging-thumb.html
compositing/webgl/webgl-nonpremultiplied-blend.html
fast/loader/loadInProgress.html
platform/chromium/virtual/gpu/fast/canvas/canvas-resize-after-paint-without-layout.html
fast/loader/unload-form-post-about-blank.html
compositing/visibility/visibility-simple-canvas2d-layer.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-simple-canvas2d-layer.html
platform/chromium/virtual/gpu/fast/canvas/canvas-imageSmoothingEnabled-patterns.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-composite-modes-repaint.html
fast/canvas/webgl/webgl-composite-modes-repaint.html
TiledLayerChromiumTest.visibleContentOpaqueRegion
platform/chromium/virtual/gpu/fast/canvas/canvas-composite-fill-repaint.html
compositing/visibility/visibility-simple-webgl-layer.html
fast/forms/range/slider-mouse-events.html
fast/frames/cached-frame-counter.html
compositing/webgl/webgl-background-color.html
platform/chromium/compositing/webgl-loses-compositor-context.html
TiledLayerChromiumTest.invalidateFromPrepare
platform/chromium/virtual/threaded/compositing/visibility/visibility-simple-webgl-layer.html
fast/forms/range/slider-onchange-event.html
compositing/webgl/webgl-no-alpha.html
compositing/webgl/webgl-reflection.html
fast/canvas/webgl/webgl-composite-modes.html
platform/chromium/virtual/gpu/fast/canvas/canvas-empty-image-pattern.html
compositing/backface-visibility/backface-visibility-webgl.html
platform/chromium/virtual/gpu/fast/canvas/canvas-before-css.html

[WebKit Review Bot]

Created attachment 153946 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Dana Jansens]

Comment on attachment 153930 [details]
Patch

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

>> Source/Platform/chromium/public/WebRenderingStats.h:35
>> +    double totalRasterizeTime; // in seconds
> 
> nit: If I were writing this patch, I might have included the comment in the variable name:
> totalPaintTimeInSeconds

+1 we do this in other places with units such as Bytes suffixed onto variables.

[Dave Tu]

Created attachment 154161 [details]
Patch

[Dave Tu]

Comment on attachment 153930 [details]
Patch

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

Found out that I missed a method signature in ImageLayerChromium, so the LayerChromium method was being called instead of the overriden method. That was causing the layout test failures. Does the OVERRIDE macro not generate compile-time errors or warnings?

>>> Source/Platform/chromium/public/WebRenderingStats.h:35
>>> +    double totalRasterizeTime; // in seconds
>> 
>> nit: If I were writing this patch, I might have included the comment in the variable name:
>> totalPaintTimeInSeconds
> 
> +1 we do this in other places with units such as Bytes suffixed onto variables.

Done.

[Dana Jansens]

(In reply to comment #29)
> (From update of attachment 153930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153930&action=review
> 
> Found out that I missed a method signature in ImageLayerChromium, so the LayerChromium method was being called instead of the overriden method. That was causing the layout test failures. Does the OVERRIDE macro not generate compile-time errors or warnings?

Only if you're using clang! (You should consider using it :))

[WebKit Review Bot]

Comment on attachment 154161 [details]
Patch

Clearing flags on attachment: 154161

Committed r123553: <http://trac.webkit.org/changeset/123553>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.