Bug 90264 - Web Inspector: added low-level instrumentation support for TimelineAgent
Summary: Web Inspector: added low-level instrumentation support for TimelineAgent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 90277
  Show dependency treegraph
 
Reported: 2012-06-29 02:54 PDT by Sergey Rogulenko
Modified: 2012-07-05 09:14 PDT (History)
13 users (show)

See Also:


Attachments
Patch (7.24 KB, patch)
2012-06-29 03:00 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (6.95 KB, patch)
2012-06-29 05:06 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (6.64 KB, patch)
2012-06-29 05:11 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2012-06-29 06:53 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (5.54 KB, patch)
2012-06-29 07:48 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2012-07-03 02:45 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (6.00 KB, patch)
2012-07-03 06:05 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (7.58 KB, patch)
2012-07-03 08:24 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (9.69 KB, patch)
2012-07-03 08:49 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (9.15 KB, patch)
2012-07-03 09:39 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2012-07-03 10:40 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (8.81 KB, patch)
2012-07-04 08:34 PDT, Sergey Rogulenko
no flags Details | Formatted Diff | Diff
Patch (8.78 KB, patch)
2012-07-04 08:59 PDT, Sergey Rogulenko
pfeldman: review+
rogulenko: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Rogulenko 2012-06-29 02:54:34 PDT
Added low-level instrumentation support for TimelineAgent
Comment 1 Sergey Rogulenko 2012-06-29 03:00:54 PDT
Created attachment 150125 [details]
Patch
Comment 2 Andrey Kosyakov 2012-06-29 03:10:00 PDT
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.
Comment 3 Sergey Rogulenko 2012-06-29 05:06:41 PDT
Created attachment 150143 [details]
Patch
Comment 4 Sergey Rogulenko 2012-06-29 05:11:17 PDT
Created attachment 150145 [details]
Patch
Comment 5 Andrey Kosyakov 2012-06-29 05:15:22 PDT
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.
Comment 6 Andrey Kosyakov 2012-06-29 05:41:17 PDT
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;
}
Comment 7 Yury Semikhatsky 2012-06-29 06:18:46 PDT
Comment on attachment 150145 [details]
Patch

r- until Andrey's comment is addressed
Comment 8 Sergey Rogulenko 2012-06-29 06:53:24 PDT
Created attachment 150169 [details]
Patch
Comment 9 Andrey Kosyakov 2012-06-29 06:56:10 PDT
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?
Comment 10 Pavel Feldman 2012-06-29 07:25:18 PDT
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.
Comment 11 Sergey Rogulenko 2012-06-29 07:48:28 PDT
Created attachment 150178 [details]
Patch
Comment 12 Pavel Feldman 2012-06-29 07:52:03 PDT
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.
Comment 13 Andrey Kosyakov 2012-06-29 07:55:57 PDT
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.
Comment 14 Pavel Feldman 2012-06-29 07:59:46 PDT
> 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.
Comment 15 Andrey Kosyakov 2012-06-29 08:16:34 PDT
(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).
Comment 16 Pavel Feldman 2012-06-29 08:23:34 PDT
(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.
Comment 17 Sergey Rogulenko 2012-07-03 02:45:14 PDT
Created attachment 150566 [details]
Patch
Comment 18 Andrey Kosyakov 2012-07-03 05:28:39 PDT
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?
Comment 19 Sergey Rogulenko 2012-07-03 05:49:38 PDT
(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?
Comment 20 Sergey Rogulenko 2012-07-03 06:05:42 PDT
Created attachment 150593 [details]
Patch
Comment 21 Sergey Rogulenko 2012-07-03 08:24:38 PDT
Created attachment 150614 [details]
Patch
Comment 22 Sergey Rogulenko 2012-07-03 08:49:32 PDT
Created attachment 150619 [details]
Patch
Comment 23 Andrey Kosyakov 2012-07-03 08:54:53 PDT
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.
Comment 24 Sergey Rogulenko 2012-07-03 09:39:36 PDT
Created attachment 150628 [details]
Patch
Comment 25 Andrey Kosyakov 2012-07-03 09:55:53 PDT
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>(...)
Comment 26 Pavel Feldman 2012-07-03 10:06:16 PDT
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
Comment 27 Sergey Rogulenko 2012-07-03 10:40:38 PDT
Created attachment 150641 [details]
Patch
Comment 28 Pavel Feldman 2012-07-04 05:20:55 PDT
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.
Comment 29 Sergey Rogulenko 2012-07-04 08:34:28 PDT
Created attachment 150804 [details]
Patch
Comment 30 Pavel Feldman 2012-07-04 08:41:40 PDT
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"
Comment 31 Sergey Rogulenko 2012-07-04 08:59:48 PDT
Created attachment 150809 [details]
Patch
Comment 32 Andrey Kosyakov 2012-07-05 09:14:12 PDT
Committed r121908: <http://trac.webkit.org/changeset/121908>