Bug 88639 – Web Inspector: Add message loop instrumentation to public API and timeline agent

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

Created attachment 146514 [details]
Diagram 1: WebView has an implicit contract to hold both WebDevToolsAgent and WebDevToolsAgentClient references

Message loop instrumentation will show when the render thread is busy.
That way developer can discover if a render thread business causes low fps, or not.

Attached diagrams demonstrate that there is an implicit 1-to-1 relationship between WebDevToolsAgent and WebDevToolsAgentClient.
So the contract for these new methods is: WebDevToolsAgentClient is controlling the notifications subscription for corresponding WebDevToolsAgent.

Created attachment 146515 [details]
Diagram 2: Implementation of WebDevToolsAgent holds reference to WebDevToolsAgentClient

Created attachment 146516 [details]
Diagram 3: Implicit contract is turned to explicit in chromium: WebDevToolsAgentClient implementation has access to corresponding WebDevToolsAgent.

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

[James Robinson]

Thank you for the diagrams!  My one concern is that many WebViews (and thus many WebDevToolsAgents) share the same message loop.  Is it the embedder's responsibility to call instrument(Will|Did)ProcessTask on every active WebDevToolsAgent in the process when processing a task?  That feels like it could be pretty slow.

Does the inspector currently keep track of any data at a process-wide level, or does everything hang from Page and below?

[Andrey Kosyakov]

(answering on behalf of Eugene to minimize the round-trip :))

(In reply to comment #5)
> Thank you for the diagrams!  My one concern is that many WebViews (and thus many WebDevToolsAgents) share the same message loop.  Is it the embedder's responsibility to call instrument(Will|Did)ProcessTask on every active WebDevToolsAgent in the process when processing a task?  That feels like it could be pretty slow.

The embedder is only supposed to install a message loop observer and start calling instrument(Will|Did)ProcessTask after receiving startMessageLoopMonitoring() callback via InspectorClient (and deinstall it upon stopMessageLoopMonitoring()) -- so there shouldn't be any overhead unless Timeline is being recorded. 

In case there are multiple instances WebDevToolsAgent recording timeline simultaneously, there will likely be multiple observers installed, each associated with each own WebDevToolsAgent and dispatching notifications on all events.

> 
> Does the inspector currently keep track of any data at a process-wide level, or does everything hang from Page and below?

AFAIK we always associate instrumentation calls with a Page or a related object (e.g. a Frame or a Document). In case of message loop instrumentation, the instrumentation methods also receive Page, but this is rather to get the associated inspector agent (and corresponding front-end). Every agent that monitors the message loop will receive notifications about all events, but I think that's fair -- since we use this for performance monitoring, we want some indication that our page is slow because it shares a renderer with some other busy WebView.

(In reply to comment #5)
> Thank you for the diagrams!  My one concern is that many WebViews (and thus many WebDevToolsAgents) share the same message loop.  Is it the embedder's responsibility to call instrument(Will|Did)ProcessTask on every active WebDevToolsAgent in the process when processing a task?  That feels like it could be pretty slow.
> 
> Does the inspector currently keep track of any data at a process-wide level, or does everything hang from Page and below?

Currently embedder is not supposed to distribute notifications to all agents, it will deliver notifications only to agent that initiated instrumentation.

The idea behind this is the following: when user press record timeline button, timeline subscribes itself for different notifications (javascript GC, message loop, etc.). These notifications represent activity that influences all the views in the current process. The same time timeline agent catches some other instrumentation notifications that are related to specific page. So, when we combine both types of data, we allow user to see what processing is done for specific view alongside with global process business.

Without global data, it is uncertain, why page is slow: it may be slow because of some processing that is not instrumented yet, because of cpu resources sharing between views, or because of some synchronization (for example, with gpu processing).

So, because there is no distribuion, notifications are quick enough. When instrumentation is on (recording timeline) i can't notice a difference with usual timeline recording.

My observations show, that there is 1-3 message loop tasks per frame, so it comes about one hundred per second.

[James Robinson]

OK, that all makes sense. Chromium public API LGTM in that case, I'll defer to the inspector folks on all the rest.

Created attachment 146694 [details]
Patch

Rebased

[WebKit Review Bot]

Comment on attachment 146694 [details]
Patch

Clearing flags on attachment: 146694

Committed r119897: <http://trac.webkit.org/changeset/119897>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

Comment on attachment 146694 [details]
Patch

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

> Source/WebKit/chromium/public/WebDevToolsAgentClient.h:64
> +    virtual void startMessageLoopMonitoring() { }

It looks like there is some redundancy here with WebThread::{add,remove}TaskObserver.
Perhaps you just needed to add a willProcessTask method to WebThread::TaskObserver?

Also, historically, we have avoided using the term "message loop" in webkit since
most code in webkit uses the term "run loop".

Comment on attachment 146694 [details]
Patch

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

>> Source/WebKit/chromium/public/WebDevToolsAgentClient.h:64
>> +    virtual void startMessageLoopMonitoring() { }
> 
> It looks like there is some redundancy here with WebThread::{add,remove}TaskObserver.
> Perhaps you just needed to add a willProcessTask method to WebThread::TaskObserver?
> 
> Also, historically, we have avoided using the term "message loop" in webkit since
> most code in webkit uses the term "run loop".

Using WebThread::TaskObserver is not better:
1) It is inner class, so it can't be forward-declared
2) WebThread is "an interface to an embedder-defined thread implementation";
   it is used to provide threading to WebKit, mostly for compositing.
   In our case, rendering thread is owned by embedder, so we can't state it is a WebThread.

Comment on attachment 146694 [details]
Patch

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

>>> Source/WebKit/chromium/public/WebDevToolsAgentClient.h:64
>>> +    virtual void startMessageLoopMonitoring() { }
>> 
>> It looks like there is some redundancy here with WebThread::{add,remove}TaskObserver.
>> Perhaps you just needed to add a willProcessTask method to WebThread::TaskObserver?
>> 
>> Also, historically, we have avoided using the term "message loop" in webkit since
>> most code in webkit uses the term "run loop".
> 
> Using WebThread::TaskObserver is not better:
> 1) It is inner class, so it can't be forward-declared
> 2) WebThread is "an interface to an embedder-defined thread implementation";
>    it is used to provide threading to WebKit, mostly for compositing.
>    In our case, rendering thread is owned by embedder, so we can't state it is a WebThread.

Oops.
I was too quick to judge ;-)

We've found solution that both:
1) Reuses WebThread
2) Do not require commits to Chromium project to work

Please, see: https://bugs.webkit.org/show_bug.cgi?id=88978