Bug 217853

Summary: Add system trace points for flushing remote image buffers
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit2Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, sabouhallawa, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
simon.fraser: review+, ews-feeder: commit-queue-
Patch for landing none

Description Wenson Hsieh 2020-10-16 15:59:13 PDT
(Also, add it in WebKit2 layer so that we don't end up calling kdebug_trace millions of times throughout the course of the Design subtest).
Comment 1 Wenson Hsieh 2020-10-16 16:50:21 PDT
Created attachment 411631 [details]
Patch
Comment 2 Wenson Hsieh 2020-10-16 19:00:20 PDT
Comment on attachment 411631 [details]
Patch

EWS failure seems to be due to flaky tests.
Comment 3 Simon Fraser (smfr) 2020-10-16 19:58:27 PDT
Comment on attachment 411631 [details]
Patch

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

> Source/WTF/wtf/SystemTracing.h:118
> +    FlushRemoteImageBufferStart,
> +    FlushRemoteImageBufferEnd,

I'm not too hot on the "flush image buffer" terminology.
Comment 4 Wenson Hsieh 2020-10-16 20:04:33 PDT
Comment on attachment 411631 [details]
Patch

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

>> Source/WTF/wtf/SystemTracing.h:118
>> +    FlushRemoteImageBufferEnd,
> 
> I'm not too hot on the "flush image buffer" terminology.

How about "FlushDisplayListStart" and "FlushDisplayListEnd"?
Comment 5 Simon Fraser (smfr) 2020-10-16 20:26:41 PDT
Comment on attachment 411631 [details]
Patch

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

>>> Source/WTF/wtf/SystemTracing.h:118
>>> +    FlushRemoteImageBufferEnd,
>> 
>> I'm not too hot on the "flush image buffer" terminology.
> 
> How about "FlushDisplayListStart" and "FlushDisplayListEnd"?

The trace points encompass IPC encoding of the DL, and async dispatch forIPC and displayList.clear(), right? Do we want to track all those, or just the encoding? Or do we really want to track flushDrawingContext/flushDrawingContextAndWaitCommit ?
Comment 6 Wenson Hsieh 2020-10-16 20:29:57 PDT
(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 411631 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411631&action=review
> 
> >>> Source/WTF/wtf/SystemTracing.h:118
> >>> +    FlushRemoteImageBufferEnd,
> >> 
> >> I'm not too hot on the "flush image buffer" terminology.
> > 
> > How about "FlushDisplayListStart" and "FlushDisplayListEnd"?
> 
> The trace points encompass IPC encoding of the DL, and async dispatch forIPC
> and displayList.clear(), right? Do we want to track all those, or just the
> encoding? Or do we really want to track
> flushDrawingContext/flushDrawingContextAndWaitCommit ?

My intention was to track flushDrawingContext/flushDrawingContextAndWaitCommit in the remote image buffer case.
Comment 7 Simon Fraser (smfr) 2020-10-16 20:32:25 PDT
Maybe what you have is fine then. Maybe set data1 to 1 in the "and wait" case so we can tell them apart.
Comment 8 Wenson Hsieh 2020-10-16 20:37:07 PDT
(In reply to Simon Fraser (smfr) from comment #7)
> Maybe what you have is fine then. Maybe set data1 to 1 in the "and wait"
> case so we can tell them apart.

Sounds good — changed the second trace scope to `TraceScope tracingScope(FlushRemoteImageBufferStart, FlushRemoteImageBufferEnd, 1);`.
Comment 9 Wenson Hsieh 2020-10-16 20:40:24 PDT
Created attachment 411650 [details]
Patch for landing
Comment 10 EWS 2020-10-16 21:55:38 PDT
Committed r268636: <https://trac.webkit.org/changeset/268636>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411650 [details].
Comment 11 Radar WebKit Bug Importer 2020-10-16 21:56:15 PDT
<rdar://problem/70402506>