Web Inspector: added low-level instrumentation support for TimelineAgent

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

[Sergey Rogulenko]

Added low-level instrumentation support for TimelineAgent

[Sergey Rogulenko]

Created attachment 150125 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 150125 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.h:102
> +    static void startLowlevelInspectingForFrame(Frame*);
> +    static void stopLowlevelInspecting();
> +

This should not be exposed in instrumentation interface -- we should rather set this implicitly during other timeline events in InspectorTimelineAgent.

[Sergey Rogulenko]

Created attachment 150143 [details]
Patch

[Sergey Rogulenko]

Created attachment 150145 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 150145 [details]
Patch

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

LGTM, but please also provide the actual usage example before we land this.

> Source/WebCore/inspector/InspectorInstrumentation.h:39
> +#include "InspectorTimelineAgent.h"

We should not expose InspectorTimelineAgent to the rest of WebCore.

[Andrey Kosyakov]

Comment on attachment 150125 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:101
> +WTF::ThreadSpecific<InspectorTimelineAgent*>* InspectorTimelineAgent::s_inspectorTimelineAgent = new WTF::ThreadSpecific<InspectorTimelineAgent*>();

Please change this to use singleton-style initialization on demand, e.g.
static WTF::ThreadSpecific<InspectorTimelineAgent*>* InspectorTimelineAgent::threadSpecificInstance()
{
    AtomicallyInitializedStatic(WTF::ThreadSpecific<InspectorTimelineAgent*>, instance = new WTF::ThreadSpecific<InspectorTimelineAgent*>());
    return instance;
}

[Yury Semikhatsky]

Comment on attachment 150145 [details]
Patch

r- until Andrey's comment is addressed

[Sergey Rogulenko]

Created attachment 150169 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 150169 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:101
> +WTF::ThreadSpecific<InspectorTimelineAgent*>* InspectorTimelineAgent::ThreadSpecificInstance()

threadSpecificInstance()

> Source/WebCore/inspector/InspectorTimelineAgent.h:148
> +    static InspectorTimelineAgent* instance() { return **ThreadSpecificInstance(); }
> +    static void setInstance(InspectorTimelineAgent* inspectorTimelineAgent) { **ThreadSpecificInstance() = inspectorTimelineAgent; }

Why is this public?

[Pavel Feldman]

Comment on attachment 150169 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        * inspector/InspectorInstrumentation.cpp:

Please explain what happens and why here?

> Source/WebCore/inspector/InspectorInstrumentation.h:410
> +

Please roll back this change.

>> Source/WebCore/inspector/InspectorTimelineAgent.h:148
>> +    static void setInstance(InspectorTimelineAgent* inspectorTimelineAgent) { **ThreadSpecificInstance() = inspectorTimelineAgent; }
> 
> Why is this public?

I would rather move this method to the InspectorInstrumentation.

[Sergey Rogulenko]

Created attachment 150178 [details]
Patch

[Pavel Feldman]

Comment on attachment 150178 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:103
> +    AtomicallyInitializedStatic(WTF::ThreadSpecific<InspectorTimelineAgent*>*, instance = new WTF::ThreadSpecific<InspectorTimelineAgent*>());

We don't want to have several timelines running at a time and I wonder if we can make it clear on the declarative level. Deferring this to Yury as he is the one that added plumbing for the workers.

[Andrey Kosyakov]

Comment on attachment 150169 [details]
Patch

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

>>> Source/WebCore/inspector/InspectorTimelineAgent.h:148
>>> +    static void setInstance(InspectorTimelineAgent* inspectorTimelineAgent) { **ThreadSpecificInstance() = inspectorTimelineAgent; }
>> 
>> Why is this public?
> 
> I would rather move this method to the InspectorInstrumentation.

Why would we want to introduce a cyclical dependency between InspectorTimelineAgent and InspectorInstrumentation and make a setInstance() method public on InspectorInstrumentation, when it only should be used by InspectorTimelineAgent? Incapsulating setInstance()/instance() in timeline agent reduces the public interface and minimizes dependencies.

[Pavel Feldman]

> Why would we want to introduce a cyclical dependency between InspectorTimelineAgent and InspectorInstrumentation and make a setInstance() method public on InspectorInstrumentation, when it only should be used by InspectorTimelineAgent? Incapsulating setInstance()/instance() in timeline agent reduces the public interface and minimizes dependencies.

There is nothing wrong about bi-directional dependency between two inspector classes if it makes things simpler and properly reflects the semantics of what is going on. The fact is that WebCore should only talk to InspectorInstrumentation and there should no way for WebCore to get a hold of the agent instance. What you are suggesting is making clients wonder if they should do InspectorInstrumentation::willPaint or InspectorTimelineAgent::willPaint. Dependency-wise, this would be a bigger problem.

[Andrey Kosyakov]

(In reply to comment #14)

> There is nothing wrong about bi-directional dependency between two inspector classes if it makes things simpler and properly reflects the semantics of what is going on.

Please explain your notion of "simpler". For me, "simpler" is less methods in public interface and less dependencies. Also, I think that having setInstance() a private method of InspectorTimelineAgent is very important to express the fact that it's InspectorTimelineAgent that manages its instances. It should not be possible for other classes to do InspectorInstrumentation::setInspectorTimelineAgentInstance().


> What you are suggesting is making clients wonder if they should do InspectorInstrumentation::willPaint or InspectorTimelineAgent::willPaint. Dependency-wise, this would be a bigger problem.

This argument sounds a bit artificial to me -- none of the clients include InspectorTimelineAgent at the moment (this should also be pretty easy to enforce).

[Pavel Feldman]

(In reply to comment #15)
> (In reply to comment #14)
> 
> > There is nothing wrong about bi-directional dependency between two inspector classes if it makes things simpler and properly reflects the semantics of what is going on.
> 
> Please explain your notion of "simpler". For me, "simpler" is less methods in public interface and less dependencies. 

I am sorry, this starts to sound like non-constructive discussion to me, I'd prefer to cut it here.

Also, I think that having setInstance() a private method of InspectorTimelineAgent is very important to express the fact that it's InspectorTimelineAgent that manages its instances. It should not be possible for other classes to do InspectorInstrumentation::setInspectorTimelineAgentInstance().

And it would not be possible for them since they can't get a hold of the actual instance.
> 
> > What you are suggesting is making clients wonder if they should do InspectorInstrumentation::willPaint or InspectorTimelineAgent::willPaint. Dependency-wise, this would be a bigger problem.
> 
> This argument sounds a bit artificial to me -- none of the clients include InspectorTimelineAgent at the moment (this should also be pretty easy to enforce).

We try to use C++ terms for dependency management. Now that the team has spent lots of time separating the instrumentation and agent APIs using pure C++ terms, it is very strange to hear that it is artificial.

[Sergey Rogulenko]

Created attachment 150566 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 150566 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.h:257
> +    static WTF::ThreadSpecific<InspectorTimelineAgent*>* threadSpecificTimelineAgentInstance();

why is this public?

[Sergey Rogulenko]

(In reply to comment #18)
> (From update of attachment 150566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150566&action=review
> 
> > Source/WebCore/inspector/InspectorInstrumentation.h:257
> > +    static WTF::ThreadSpecific<InspectorTimelineAgent*>* threadSpecificTimelineAgentInstance();
> 
> why is this public?

Do we want to keep any of these 3 methods public?

[Sergey Rogulenko]

Created attachment 150593 [details]
Patch

[Sergey Rogulenko]

Created attachment 150614 [details]
Patch

[Sergey Rogulenko]

Created attachment 150619 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 150619 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:468
> +void InspectorTimelineAgent::didCompleteCurrentRecord(const String& type, bool hasLowlevelDetails)

I don't think we need to pass hasLowLevelDetails explicitly to didCompleteCurrentRecord() -- instead, use something like m_levelLowLevelEventsEnabledStackMark = m_recordStack.size(); then disable them once we popped back to this depth.

[Sergey Rogulenko]

Created attachment 150628 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 150628 [details]
Patch

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

LGTM with a nit. Pavel, please have a look!

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:500
> +    if (m_lowlevelEventsEnabledStackMark == int(m_recordStack.size())) {

static_cast<int>(...)

[Pavel Feldman]

Comment on attachment 150628 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1140
> +InspectorTimelineAgent* InspectorInstrumentation::timelineAgentInstance()

Nit: here and below: there is no need for "Instance" suffix.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1153
> +    return instance;

You should return reference here.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:513
> +    , m_lowlevelEventsEnabledStackMark(-1)

Can we get away with 0 as the default value?

> Source/WebCore/inspector/InspectorTimelineAgent.h:165
> +    void pushCurrentRecord(PassRefPtr<InspectorObject>, const String& type, bool captureCallStack, Frame*, bool hasLowlevelDetails = false);

hasLowLevelDetails

> Source/WebCore/inspector/InspectorTimelineAgent.h:167
> +        

Remove new chars

[Sergey Rogulenko]

Created attachment 150641 [details]
Patch

[Pavel Feldman]

Comment on attachment 150641 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:488
> +    if (m_lowlevelEventsEnabledStackMark && m_lowlevelEventsEnabledStackMark == static_cast<int>(m_recordStack.size())) {

The idea behind making default = 0 was so that we could make new member unsigned and get rid of such casts.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:543
> +            InspectorInstrumentation::setTimelineAgent(this);

You should call setTimelineAgent from within InspectorTimelineAgent::start. Otherwise call sites get confused.

> Source/WebCore/inspector/InspectorTimelineAgent.h:202
> +    int m_lowlevelEventsEnabledStackMark;

As I mentioned earlier, this should be m_lowLevelEventsEnabledStackMark. Furthermore, I don't think you need it. Instead of special-casing paint instrumentation, you should check whether there is a paint record in the stack upon the resize / decompress processing.

[Sergey Rogulenko]

Created attachment 150804 [details]
Patch

[Pavel Feldman]

Comment on attachment 150804 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:513
> +    , m_lowLevelEventsEnabledStackMark(0)

I would rename lowLevel to orphan for consistency. Otherwise looks good.

> Source/WebCore/inspector/InspectorTimelineAgent.h:202
> +    unsigned int m_lowLevelEventsEnabledStackMark;

just "unsigned"

[Sergey Rogulenko]

Created attachment 150809 [details]
Patch

[Andrey Kosyakov]

Committed r121908: <http://trac.webkit.org/changeset/121908>