Bug 158723 - Add system tracing points for requestAnimationFrame() workflow
Summary: Add system tracing points for requestAnimationFrame() workflow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-13 20:03 PDT by Said Abou-Hallawa
Modified: 2016-06-21 15:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (22.70 KB, patch)
2016-06-14 17:37 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (15.66 KB, patch)
2016-06-14 18:36 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (17.37 KB, patch)
2016-06-15 20:08 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (16.38 KB, patch)
2016-06-21 14:03 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (16.32 KB, patch)
2016-06-21 14:40 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2016-06-13 20:03:21 PDT
We need to measure how long each step takes and identify the perf issues especially for iOS.
Comment 1 Said Abou-Hallawa 2016-06-14 17:37:38 PDT
Created attachment 281302 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-06-14 17:42:34 PDT
Comment on attachment 281302 [details]
Patch

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

Too much logging left in this patch!

> Source/WebCore/dom/ScriptedAnimationController.cpp:147
> +    WTFLogAlways("ScriptedAnimationController::serviceScriptedAnimations at %f", monotonicTimeNow);

You don't want to commit this logging.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:95
> +    WTFLogAlways("DisplayRefreshMonitor::displayDidRefresh()");

No logging here.
Comment 3 Said Abou-Hallawa 2016-06-14 18:36:39 PDT
Created attachment 281309 [details]
Patch
Comment 4 Simon Fraser (smfr) 2016-06-15 09:13:58 PDT
Comment on attachment 281309 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:41
>  

No blank line here.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:396
> +    TraceScope(RAFUIDidRefreshDisplayStart, RAFUIDidRefreshDisplayEnd);

Odd that you don't name the variable here.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:429
> +            TraceScope tracingScope(RAFWK2LayerFlushStart, RAFWK2LayerFlushEnd);

This one...

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:494
> +    TraceScope tracingScope(RAFWK2BackingStoreFlushStart, RAFWK2BackingStoreFlushEnd);

... and this basically track the same scope. You should remove one of them.
Comment 5 Said Abou-Hallawa 2016-06-15 20:08:16 PDT
Created attachment 281431 [details]
Patch
Comment 6 Said Abou-Hallawa 2016-06-21 14:00:31 PDT
Comment on attachment 281309 [details]
Patch

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

>> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:41
>>  
> 
> No blank line here.

The blank line is removed.

>> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:396
>> +    TraceScope(RAFUIDidRefreshDisplayStart, RAFUIDidRefreshDisplayEnd);
> 
> Odd that you don't name the variable here.

A variable is added to keep the scope till the end of the function.

>> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:429
>> +            TraceScope tracingScope(RAFWK2LayerFlushStart, RAFWK2LayerFlushEnd);
> 
> This one...

This trace point was removed.
Comment 7 Said Abou-Hallawa 2016-06-21 14:03:17 PDT
Created attachment 281772 [details]
Patch
Comment 8 Simon Fraser (smfr) 2016-06-21 14:33:06 PDT
Comment on attachment 281772 [details]
Patch

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

> Source/WTF/wtf/SystemTracing.h:82
> +inline void WTFTraceCodeRecord(TracePointCode code)

I don't think this needs the WTF prefix, since it's in the WTF namespace.

Let's call it TracePoint().
Comment 9 Said Abou-Hallawa 2016-06-21 14:40:14 PDT
Created attachment 281775 [details]
Patch
Comment 10 WebKit Commit Bot 2016-06-21 15:30:07 PDT
Comment on attachment 281775 [details]
Patch

Clearing flags on attachment: 281775

Committed r202297: <http://trac.webkit.org/changeset/202297>
Comment 11 WebKit Commit Bot 2016-06-21 15:30:13 PDT
All reviewed patches have been landed.  Closing bug.