Bug 88639 - Web Inspector: Add message loop instrumentation to public API and timeline agent
Summary: Web Inspector: Add message loop instrumentation to public API and timeline agent
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: eustas.bug
URL:
Keywords:
Depends on:
Blocks: 88325
  Show dependency treegraph
 
Reported: 2012-06-08 02:09 PDT by eustas.bug
Modified: 2012-06-13 02:41 PDT (History)
18 users (show)

See Also:


Attachments
Diagram 1: WebView has an implicit contract to hold both WebDevToolsAgent and WebDevToolsAgentClient references (41.64 KB, image/png)
2012-06-08 02:09 PDT, eustas.bug
no flags Details
Diagram 2: Implementation of WebDevToolsAgent holds reference to WebDevToolsAgentClient (34.68 KB, image/png)
2012-06-08 02:10 PDT, eustas.bug
no flags Details
Diagram 3: Implicit contract is turned to explicit in chromium: WebDevToolsAgentClient implementation has access to corresponding WebDevToolsAgent. (81.15 KB, image/png)
2012-06-08 02:11 PDT, eustas.bug
no flags Details
Patch (21.61 KB, patch)
2012-06-08 02:26 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (21.68 KB, patch)
2012-06-09 01:50 PDT, eustas.bug
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eustas.bug 2012-06-08 02:09:49 PDT
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.
Comment 1 eustas.bug 2012-06-08 02:10:31 PDT
Created attachment 146515 [details]
Diagram 2: Implementation of WebDevToolsAgent holds reference to WebDevToolsAgentClient
Comment 2 eustas.bug 2012-06-08 02:11:11 PDT
Created attachment 146516 [details]
Diagram 3: Implicit contract is turned to explicit in chromium: WebDevToolsAgentClient implementation has access to corresponding WebDevToolsAgent.
Comment 3 eustas.bug 2012-06-08 02:26:25 PDT
Created attachment 146520 [details]
Patch
Comment 4 WebKit Review Bot 2012-06-08 02:29:18 PDT
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 James Robinson 2012-06-08 12:39:20 PDT
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?
Comment 6 Andrey Kosyakov 2012-06-08 13:55:54 PDT
(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.
Comment 7 eustas.bug 2012-06-08 14:17:13 PDT
(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.
Comment 8 James Robinson 2012-06-08 18:17:24 PDT
OK, that all makes sense. Chromium public API LGTM in that case, I'll defer to the inspector folks on all the rest.
Comment 9 eustas.bug 2012-06-09 01:50:36 PDT
Created attachment 146694 [details]
Patch

Rebased
Comment 10 WebKit Review Bot 2012-06-09 03:16:24 PDT
Comment on attachment 146694 [details]
Patch

Clearing flags on attachment: 146694

Committed r119897: <http://trac.webkit.org/changeset/119897>
Comment 11 WebKit Review Bot 2012-06-09 03:16:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Fisher (:fishd, Google) 2012-06-10 23:00:44 PDT
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 13 eustas.bug 2012-06-12 23:27:44 PDT
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 14 eustas.bug 2012-06-13 02:41:28 PDT
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