Bug 83928 – [chromium] add instrumentation for compositing

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

[Andrey Kosyakov]

This plumbs willCommit() and didComposite() from CCThreadProxy and CCSingleThreadProxy to InspectorInsturmentation. Note that we currently only account for tasks happening on the main renderer thread, so in case of threaded compositing this is limited to commit.
See also bug 83842 for related inspector-side change.

[Andrey Kosyakov]

Created attachment 137122 [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]

Comment on attachment 137122 [details]
Patch

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

[Nat Duca]

Comment on attachment 137122 [details]
Patch

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

Looks cool. A few thoughts but nothing major. :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:68
> +    virtual void didComposite() = 0;

didComposite -> didBeginFrame --- we have willBeginFrame, this is the closing of that.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:284
>      if (!m_layerTreeHost->updateLayers(updater))

Fyi, this updateLayers call is the bulk of the time between willBeginFrame/didBeginFrame.

If painting is heavy, e.g. due to rounded corners or gradients etc, this will be big. But it includes singifiant costs more than just WebCore::*:paint(), for example texture upload.

I'd call this compositedPaint, or paintLayers. Some percentage of it will be caught by the existing paint instrumentation, but not all.

Do you want to hook this while you're at it? willPaintLayers/didPaintLayers would be a good name at least on the CCLayerTreeHostClient.

[Andrey Kosyakov]

(In reply to comment #4)
> Looks cool. A few thoughts but nothing major. :)
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:68
> > +    virtual void didComposite() = 0;
> 
> didComposite -> didBeginFrame --- we have willBeginFrame, this is the closing of that.

Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:284
> >      if (!m_layerTreeHost->updateLayers(updater))
> 
> Fyi, this updateLayers call is the bulk of the time between willBeginFrame/didBeginFrame.
> 
> If painting is heavy, e.g. due to rounded corners or gradients etc, this will be big. But it includes singifiant costs more than just WebCore::*:paint(), for example texture upload.
> 
> I'd call this compositedPaint, or paintLayers. Some percentage of it will be caught by the existing paint instrumentation, but not all.
> 
> Do you want to hook this while you're at it? willPaintLayers/didPaintLayers would be a good name at least on the CCLayerTreeHostClient.

Yup, I noticed that, it's just that I thought this should be a separate change (we're also likely want to tweak front-end for that).

[Andrey Kosyakov]

Created attachment 142490 [details]
Patch

[WebKit Review Bot]

Comment on attachment 142490 [details]
Patch

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

[Andrey Kosyakov]

ping :-)

[Adrienne Walker]

Comment on attachment 142490 [details]
Patch

The approach looks good, but please add unit tests in CCLayerTreeHostTest.cpp.

[Andrey Kosyakov]

Created attachment 143612 [details]
Patch

[Andrey Kosyakov]

(In reply to comment #9)
> (From update of attachment 142490 [details])
> The approach looks good, but please add unit tests in CCLayerTreeHostTest.cpp.

Fixed, though I decided we better test these on WebLayerTreeView, given it's a part of the public interface -- so adding WebLayerTreeViewTest.

[WebKit Review Bot]

Attachment 143612 [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/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 16 files


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

[James Robinson]

Comment on attachment 143612 [details]
Patch

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

Looks good but be careful when landing not to compile-bust chromium.  Safest is to land the chromium implementations of the new functions first, I think.

> Source/Platform/chromium/public/WebLayerTreeViewClient.h:44
> +    // Indicates that main thread tasks associated with frame rendering have completed.
> +    // Issued unconditionally, even if the context was lost in the process.
> +    virtual void didBeginFrame() = 0;

There are implementations of WebLayerTreeViewClient outside the WebKit repository (like ui/compositor/compositor.h in chromium), so adding a new pure virtual means you have to land the chromium-side impls first or add some #ifdef guards

nit: newline between the previous function and this comment block, please

>> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:28
>> +#include <gmock/gmock.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Just bulk select this whole #include block and run sort on it

>> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:30
>> +#include "platform/WebLayer.h"
> 
> Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

Here the stylebot is just wrong. a sort of the whole block is right.

> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:33
> +#include "platform/WebLayerTreeView.h"
> +#include "platform/WebLayerTreeViewClient.h"
> +#include "platform/WebThread.h"

nit: include this as <public/Web*.h>, please

[WebKit Review Bot]

Comment on attachment 143612 [details]
Patch

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

[Andrey Kosyakov]

Committed r118513: <http://trac.webkit.org/changeset/118513>

[Andrey Kosyakov]

(In reply to comment #13)
> (From update of attachment 143612 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143612&action=review
> 
> Looks good but be careful when landing not to compile-bust chromium.  Safest is to land the chromium implementations of the new functions first, I think.

Thanks! I think we just can have an empty default implementation of these methods rather than making them pure virtual. I made them pure originally to see the usages better (and I missed the one in ui/compositor/compositor.h that you pointed out as it only appears to be compiled for Chromium OS). Given that these are just notifications, I think we don't have to force clients to implement them.

> nit: newline between the previous function and this comment block, please

Fixed.

> >> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:28
> >> +#include <gmock/gmock.h>
> > 
> > Alphabetical sorting problem.  [build/include_order] [4]
> 
> Just bulk select this whole #include block and run sort on it

Sorry for that, I actually had the local version that check-webkit-style was happy with. I forgot to commit it locally before using webkit-patch upload -g HEAD to skip the inspector part of the patch -- so it picked an earlier revision for upload, while running style check on top of (correct) locally-modified version.

> > Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:33
> > +#include "platform/WebLayerTreeView.h"
> > +#include "platform/WebLayerTreeViewClient.h"
> > +#include "platform/WebThread.h"
> 
> nit: include this as <public/Web*.h>, please

Fixed.

I've also fixed a somewhat bold way in which test timeouts used to be handled, so that timeout task is canceled once we're done with the test.