Bug 105796

Summary: Web Inspector: plumb trace events to Timeline agent
Product: WebKit Reporter: Andrey Kosyakov <caseq>
Component: Web Inspector (Deprecated)Assignee: Andrey Kosyakov <caseq>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, buildbot, dglazkov, fishd, gtk-ews, gyuyoung.kim, jamesr, keishi, levin+threading, loislo, peter+ews, pfeldman, philn, pmuellr, rakuco, rniwa, tkent+wkapi, vsevik, web-inspector-bugs, webkit-ews, webkit.review.bot, xan.lopez, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 105851    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
buildbot: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
pfeldman: review+, webkit-ews: commit-queue-
EWS run none

Description Andrey Kosyakov 2012-12-27 04:30:29 PST 
Some of the events that we want to display in Timeline panel are not convenient to plumb using present instrumentation interfaces. This adds support for handling trace events in Timeline panel and uses it to instrument deferred painting of layers.
Comment 1 Andrey Kosyakov 2012-12-27 04:37:40 PST 
Created attachment 180791 [details]
Patch
Comment 2 Pavel Feldman 2012-12-27 04:58:12 PST 
Comment on attachment 180791 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.h:122
> +    static void didReportTraceEvent(Page*, char phase, const char* categoryName, const char* name, unsigned long long id,

traceEvent ?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:117
> +        : m_size(numArguments), m_argumentNames(argumentNames), m_argumentTypes(argumentTypes), m_argumentValues(argumentValues)

stack these please.
who owns the argumentNames?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:123
> +    bool getBool(const char* name) const { return getParameter(name, TRACE_VALUE_TYPE_BOOL).m_bool; }

Place block on its own lines. asBool(const char* name).

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:140
> +    const TraceValueUnion& getParameter(const char* name, char expectedType) const

parameter(

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:226
> +        m_client->enableTraceEventsReporting(true);

setReportTraceEvents

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:243
> +    m_deferredPaintLayerId = 0;

Lets split these.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:378
> +    TraceEventHandler handler = m_traceEventHandlers.get(name);

Lets make name+phase a key.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:385
> +const char kLayerId[] = "layerId";

anonymous namespace for this one?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:692
> +    m_traceEventHandlers.set("Instrumentation::DeferPaint", &InspectorTimelineAgent::onDeferPaint);

I would whitelist the ones we want to expose + handle specifics using switch. Or lets introduce generic fallback handler. Or rename to onTraceEventDeferPaint

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:725
> +void InspectorTimelineAgent::pushSkippedRecord(const String& type)

mute? Can we also land this as a separate patch?
Comment 3 Andrey Kosyakov 2012-12-28 08:00:15 PST 
Created attachment 180876 [details]
Patch
Comment 4 WebKit Review Bot 2012-12-28 08:06:08 PST 
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 5 EFL EWS Bot 2012-12-28 08:07:08 PST 
Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15549833
Comment 6 Early Warning System Bot 2012-12-28 08:07:43 PST 
Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15557772
Comment 7 Build Bot 2012-12-28 08:08:25 PST 
Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15545874
Comment 8 Pavel Feldman 2012-12-28 08:08:59 PST 
Comment on attachment 180876 [details]
Patch

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

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:200
> +        ASSERT_NOT_REACHED();

That should be fine.
Comment 9 Early Warning System Bot 2012-12-28 08:09:53 PST 
Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15549836
Comment 10 Andrey Kosyakov 2012-12-28 08:56:02 PST 
Created attachment 180881 [details]
Patch

Trying on bots..
Comment 11 Build Bot 2012-12-28 09:01:15 PST 
Comment on attachment 180881 [details]
Patch

Attachment 180881 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15582050
Comment 12 kov's GTK+ EWS bot 2012-12-28 09:11:09 PST 
Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15549854
Comment 13 Andrey Kosyakov 2012-12-29 00:48:50 PST 
Created attachment 180922 [details]
Patch

Another try on bots..
Comment 14 Andrey Kosyakov 2012-12-29 04:12:12 PST 
Created attachment 180932 [details]
Patch

Rebased, trying on bots again..
Comment 15 Andrey Kosyakov 2013-02-01 03:21:54 PST 
Created attachment 186000 [details]
Patch
Comment 16 Early Warning System Bot 2013-02-01 03:38:12 PST 
Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16308579
Comment 17 Early Warning System Bot 2013-02-01 03:40:15 PST 
Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16305575
Comment 18 EFL EWS Bot 2013-02-01 04:08:15 PST 
Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16293592
Comment 19 Build Bot 2013-02-01 04:12:45 PST 
Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16301628
Comment 20 WebKit Review Bot 2013-02-01 04:30:17 PST 
Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16298575
Comment 21 WebKit Review Bot 2013-02-01 04:47:09 PST 
Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16301636
Comment 22 Andrey Kosyakov 2013-02-01 05:03:34 PST 
Created attachment 186015 [details]
Patch
Comment 23 Early Warning System Bot 2013-02-01 05:15:37 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16301659
Comment 24 Early Warning System Bot 2013-02-01 05:17:06 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16301660
Comment 25 EFL EWS Bot 2013-02-01 05:17:07 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16297656
Comment 26 WebKit Review Bot 2013-02-01 05:31:11 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16297661
Comment 27 Build Bot 2013-02-01 05:37:20 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16297666
Comment 28 WebKit Review Bot 2013-02-01 05:39:38 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16300602
Comment 29 kov's GTK+ EWS bot 2013-02-01 05:56:17 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16301671
Comment 30 Build Bot 2013-02-01 06:01:14 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16297675
Comment 31 Peter Beverloo (cr-android ews) 2013-02-01 06:03:58 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16305620
Comment 32 Pavel Feldman 2013-02-01 08:47:33 PST 
Comment on attachment 186015 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:175
> +    if (m_client && m_pageAgent)

// Workers case.

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:2
> +* Copyright (C) 2012 Google Inc. All rights reserved.

2013
Comment 33 Build Bot 2013-02-01 10:55:14 PST 
Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16293892
Comment 34 Yury Semikhatsky 2013-02-04 04:19:27 PST 
Comment on attachment 186015 [details]
Patch

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

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:57
> +        return &instance;

Is it supposed to be called only on the main thread? If no, the initialization should be guarded with a mutex.

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:78
> +        m_processors[index] = m_processors.last();

I don't think this place will be a bottleneck and would probably just call remove(index) here.

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:142
> +    HashMap<String, EventTypeEntry>::iterator it = m_handlersByType.find(name);

How do you know that the processor is not destroyed when processEventOnAnyThread is called from another thread?

> Source/WebCore/inspector/TimelineTraceEventProcessor.h:69
> +        bool m_bool;

Add default constructor.

> Source/WebCore/inspector/TimelineTraceEventProcessor.h:90
> +            : m_argumentCount(0)

Initialize other parameters?
Comment 35 Yury Semikhatsky 2013-02-04 04:46:10 PST 
Comment on attachment 186015 [details]
Patch

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

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:92
> +        Vector<RefPtr<TimelineTraceEventProcessor> > processors;

Consider passing inlineCapacity = 2

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:123
> +    static TraceValueUnion missingValue;

const

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:190
> +    Vector<TraceEvent> events;

ASSERT(isMainThread())
Comment 36 Andrey Kosyakov 2013-02-04 08:24:22 PST 
Created attachment 186381 [details]
Patch
Comment 37 Andrey Kosyakov 2013-02-04 08:29:07 PST 
Created attachment 186384 [details]
Patch
Comment 38 Andrey Kosyakov 2013-02-04 08:38:14 PST 
Comment on attachment 186015 [details]
Patch

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

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:175
>> +    if (m_client && m_pageAgent)
> 
> // Workers case.

done.

>> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:2
>> +* Copyright (C) 2012 Google Inc. All rights reserved.
> 
> 2013

done

>> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:57
>> +        return &instance;
> 
> Is it supposed to be called only on the main thread? If no, the initialization should be guarded with a mutex.

It's guaranteed to be _first_ called on the main thread (we would only subscribe from the main thread).

>> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:78
>> +        m_processors[index] = m_processors.last();
> 
> I don't think this place will be a bottleneck and would probably just call remove(index) here.

done.

>> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:92
>> +        Vector<RefPtr<TimelineTraceEventProcessor> > processors;
> 
> Consider passing inlineCapacity = 2

done.

>> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:123
>> +    static TraceValueUnion missingValue;
> 
> const

done.

>> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:142
>> +    HashMap<String, EventTypeEntry>::iterator it = m_handlersByType.find(name);
> 
> How do you know that the processor is not destroyed when processEventOnAnyThread is called from another thread?

It's reference counted and one of the copies is held by the caller.

>> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:190
>> +    Vector<TraceEvent> events;
> 
> ASSERT(isMainThread())

done

>> Source/WebCore/inspector/TimelineTraceEventProcessor.h:69
>> +        bool m_bool;
> 
> Add default constructor.

done

>> Source/WebCore/inspector/TimelineTraceEventProcessor.h:90
>> +            : m_argumentCount(0)
> 
> Initialize other parameters?

Also done for m_name.
Comment 39 Andrey Kosyakov 2013-02-04 08:38:45 PST 
Created attachment 186389 [details]
Patch
Comment 40 Early Warning System Bot 2013-02-04 09:03:26 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16370264
Comment 41 Early Warning System Bot 2013-02-04 09:09:14 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16377159
Comment 42 Build Bot 2013-02-04 09:13:26 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16371296
Comment 43 EFL EWS Bot 2013-02-04 09:16:02 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16379161
Comment 44 WebKit Review Bot 2013-02-04 09:35:30 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16375254
Comment 45 WebKit Review Bot 2013-02-04 09:44:04 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16369290
Comment 46 Build Bot 2013-02-04 09:47:29 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16378177
Comment 47 Peter Beverloo (cr-android ews) 2013-02-04 10:22:07 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16375262
Comment 48 Build Bot 2013-02-04 11:49:37 PST 
Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16374283
Comment 49 Andrey Kosyakov 2013-02-26 04:55:27 PST 
Committed r144041: <http://trac.webkit.org/changeset/144041>
Comment 50 Andrey Kosyakov 2013-02-26 06:24:45 PST 
Reverted r144041 for reason:

Broke build

Committed r144053: <http://trac.webkit.org/changeset/144053>
Comment 51 Andrey Kosyakov 2013-02-26 07:02:16 PST 
Created attachment 190277 [details]
EWS run
Comment 52 Andrey Kosyakov 2013-02-26 08:30:53 PST 
Committed r144062: <http://trac.webkit.org/changeset/144062>