Bug 90019 - [chromium] Add time spent painting to GPU benchmarking renderingStats() API.
Summary: [chromium] Add time spent painting to GPU benchmarking renderingStats() API.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-26 15:39 PDT by Dave Tu
Modified: 2012-07-24 17:26 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.72 KB, patch)
2012-06-26 15:39 PDT, Dave Tu
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2012-06-26 16:08 PDT, Dave Tu
no flags Details | Formatted Diff | Diff
Patch (69.57 KB, patch)
2012-07-13 15:46 PDT, Dave Tu
no flags Details | Formatted Diff | Diff
Patch (76.93 KB, patch)
2012-07-19 16:49 PDT, Dave Tu
no flags Details | Formatted Diff | Diff
Patch (76.14 KB, patch)
2012-07-20 16:51 PDT, Dave Tu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (399.10 KB, application/zip)
2012-07-20 19:49 PDT, WebKit Review Bot
no flags Details
Patch (76.11 KB, patch)
2012-07-23 18:16 PDT, Dave Tu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (353.78 KB, application/zip)
2012-07-23 20:44 PDT, WebKit Review Bot
no flags Details
Patch (79.06 KB, patch)
2012-07-24 15:43 PDT, Dave Tu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Tu 2012-06-26 15:39:04 PDT
[chromium] Add time spent painting to GPU benchmarking renderingStats() API.
Comment 1 Dave Tu 2012-06-26 15:39:54 PDT
Created attachment 149618 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-26 15:42:45 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 WebKit Review Bot 2012-06-26 15:43:05 PDT
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.
Comment 4 Dave Tu 2012-06-26 16:08:25 PDT
Created attachment 149624 [details]
Patch
Comment 5 James Robinson 2012-06-26 17:42:29 PDT
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
Comment 6 Nat Duca 2012-06-27 06:58:42 PDT
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?
Comment 7 Dave Tu 2012-06-27 12:54:53 PDT
(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?
Comment 8 Nat Duca 2012-07-02 12:36:07 PDT
(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.
Comment 9 Dave Tu 2012-07-13 15:46:06 PDT
Created attachment 152358 [details]
Patch
Comment 10 Nat Duca 2012-07-19 01:32:22 PDT
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?
Comment 11 Adrienne Walker 2012-07-19 11:06:55 PDT
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.
Comment 12 Dave Tu 2012-07-19 16:49:03 PDT
Created attachment 153376 [details]
Patch
Comment 13 Dave Tu 2012-07-19 16:50:07 PDT
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.
Comment 14 Dana Jansens 2012-07-19 16:54:19 PDT
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
Comment 15 Adrienne Walker 2012-07-20 10:02:47 PDT
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?
Comment 16 Nat Duca 2012-07-20 10:09:50 PDT
Good point. Because of complications on double-rolling, I'd suggest comments for now.
Comment 17 Dave Tu 2012-07-20 16:51:23 PDT
Created attachment 153622 [details]
Patch
Comment 18 WebKit Review Bot 2012-07-20 19:49:42 PDT
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
Comment 19 WebKit Review Bot 2012-07-20 19:49:49 PDT
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
Comment 20 Adrienne Walker 2012-07-22 11:27:49 PDT
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.
Comment 21 Dave Tu 2012-07-23 18:16:36 PDT
Created attachment 153930 [details]
Patch
Comment 22 Adam Barth 2012-07-23 18:45:02 PDT
Comment on attachment 153930 [details]
Patch

API change LGTM
Comment 23 Adam Barth 2012-07-23 18:46:16 PDT
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
Comment 24 WebKit Review Bot 2012-07-23 19:00:58 PDT
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
Comment 25 WebKit Review Bot 2012-07-23 20:44:05 PDT
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
Comment 26 WebKit Review Bot 2012-07-23 20:44:12 PDT
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
Comment 27 Dana Jansens 2012-07-24 09:10:27 PDT
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.
Comment 28 Dave Tu 2012-07-24 15:43:36 PDT
Created attachment 154161 [details]
Patch
Comment 29 Dave Tu 2012-07-24 15:47:22 PDT
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.
Comment 30 Dana Jansens 2012-07-24 15:49:15 PDT
(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 :))
Comment 31 WebKit Review Bot 2012-07-24 17:26:15 PDT
Comment on attachment 154161 [details]
Patch

Clearing flags on attachment: 154161

Committed r123553: <http://trac.webkit.org/changeset/123553>
Comment 32 WebKit Review Bot 2012-07-24 17:26:22 PDT
All reviewed patches have been landed.  Closing bug.