Bug 105796 – Web Inspector: plumb trace events to Timeline agent

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

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.

[Andrey Kosyakov]

Created attachment 180791 [details]
Patch

[Pavel Feldman]

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?

[Andrey Kosyakov]

Created attachment 180876 [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.

[EFL EWS Bot]

Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15549833

[Early Warning System Bot]

Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15557772

[Build Bot]

Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15545874

[Pavel Feldman]

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.

[Early Warning System Bot]

Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15549836

[Andrey Kosyakov]

Created attachment 180881 [details]
Patch

Trying on bots..

[Build Bot]

Comment on attachment 180881 [details]
Patch

Attachment 180881 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15582050

[kov's GTK+ EWS bot]

Comment on attachment 180876 [details]
Patch

Attachment 180876 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15549854

[Andrey Kosyakov]

Created attachment 180922 [details]
Patch

Another try on bots..

[Andrey Kosyakov]

Created attachment 180932 [details]
Patch

Rebased, trying on bots again..

[Andrey Kosyakov]

Created attachment 186000 [details]
Patch

[Early Warning System Bot]

Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16308579

[Early Warning System Bot]

Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16305575

[EFL EWS Bot]

Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16293592

[Build Bot]

Comment on attachment 186000 [details]
Patch

Attachment 186000 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16301628

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 186000 [details]
Patch

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

[Andrey Kosyakov]

Created attachment 186015 [details]
Patch

[Early Warning System Bot]

Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16301659

[Early Warning System Bot]

Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16301660

[EFL EWS Bot]

Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16297656

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16297666

[WebKit Review Bot]

Comment on attachment 186015 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16301671

[Build Bot]

Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16297675

[Peter Beverloo (cr-android ews)]

Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16305620

[Pavel Feldman]

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

[Build Bot]

Comment on attachment 186015 [details]
Patch

Attachment 186015 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16293892

[Yury Semikhatsky]

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?

[Yury Semikhatsky]

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())

[Andrey Kosyakov]

Created attachment 186381 [details]
Patch

[Andrey Kosyakov]

Created attachment 186384 [details]
Patch

[Andrey Kosyakov]

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.

[Andrey Kosyakov]

Created attachment 186389 [details]
Patch

[Early Warning System Bot]

Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16370264

[Early Warning System Bot]

Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16377159

[Build Bot]

Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16371296

[EFL EWS Bot]

Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16379161

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 186389 [details]
Patch

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

[Build Bot]

Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16378177

[Peter Beverloo (cr-android ews)]

Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16375262

[Build Bot]

Comment on attachment 186389 [details]
Patch

Attachment 186389 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16374283

[Andrey Kosyakov]

Committed r144041: <http://trac.webkit.org/changeset/144041>

[Andrey Kosyakov]

Reverted r144041 for reason:

Broke build

Committed r144053: <http://trac.webkit.org/changeset/144053>

[Andrey Kosyakov]

Created attachment 190277 [details]
EWS run

[Andrey Kosyakov]

Committed r144062: <http://trac.webkit.org/changeset/144062>