Bug 135427 - Web Inspector: MessageDispatcher should not synchronously dispatch all backend messages
Summary: Web Inspector: MessageDispatcher should not synchronously dispatch all backen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-30 14:08 PDT by Brian Burg
Modified: 2014-07-31 12:47 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.70 KB, patch)
2014-07-30 14:25 PDT, Brian Burg
timothy: review+
timothy: commit-queue-
Details | Formatted Diff | Diff
example output 1 (206.15 KB, text/plain)
2014-07-30 14:32 PDT, Brian Burg
no flags Details
example output 2 (339.40 KB, text/plain)
2014-07-30 14:33 PDT, Brian Burg
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-07-30 14:08:36 PDT
patch coming
Comment 1 Brian Burg 2014-07-30 14:25:03 PDT
Created attachment 235761 [details]
Patch
Comment 2 Joseph Pecoraro 2014-07-30 14:30:11 PDT
Comment on attachment 235761 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Protocol/MessageDispatcher.js:50
> +        this._dispatchTimeout = setTimeout(this.dispatchNextQueuedMessageFromBackend.bind(this), 0);

We should avoid this.dispatchNextQueuedMessageFromBackend.bind every time this happens and have a pre-bound version of this that we can call. Or eliminate all uses of "this." and convert to "WebInspector." and we won't need a bound function.
Comment 3 Brian Burg 2014-07-30 14:32:35 PDT
Created attachment 235763 [details]
example output 1
Comment 4 Brian Burg 2014-07-30 14:33:47 PDT
Created attachment 235764 [details]
example output 2

the first attachment shows logspew when reloading cnn.com as the inspector is viewing resources.
the second is the same reload while looking at timeline overviews.
Comment 5 Timothy Hatcher 2014-07-30 17:16:58 PDT
Comment on attachment 235761 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:176
> +            var processingDuration = Date.now() - processingStartTime;

performance.now()! You wil get better numbers.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:223
> +        var processingDuration = Date.now() - processingStartTime;

Ditto.

> Source/WebInspectorUI/UserInterface/Protocol/MessageDispatcher.js:32
> +    var startTime = Date.now();

performance.now()

> Source/WebInspectorUI/UserInterface/Protocol/MessageDispatcher.js:36
> +    var i;
> +    for (i = 0; i < this.messagesToDispatch.length; ++i) {

I would put var i = 0; on the first line and leave the for loop initialize statement empty. Saves on var changes and will make the JIT happier.

> Source/WebInspectorUI/UserInterface/Protocol/MessageDispatcher.js:39
> +        if (Date.now() - startTime > timeLimitPerRunLoop)

performance.now()
Comment 6 Brian Burg 2014-07-31 12:45:56 PDT
Comment on attachment 235761 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:176
>> +            var processingDuration = Date.now() - processingStartTime;
> 
> performance.now()! You wil get better numbers.

Not enabled for Mac and Win ports, apparently. Tracked here: <https://bugs.webkit.org/show_bug.cgi?id=42434> and here: <https://bugs.webkit.org/show_bug.cgi?id=135467>

>> Source/WebInspectorUI/UserInterface/Protocol/MessageDispatcher.js:36
>> +    for (i = 0; i < this.messagesToDispatch.length; ++i) {
> 
> I would put var i = 0; on the first line and leave the for loop initialize statement empty. Saves on var changes and will make the JIT happier.

ok

>> Source/WebInspectorUI/UserInterface/Protocol/MessageDispatcher.js:50
>> +        this._dispatchTimeout = setTimeout(this.dispatchNextQueuedMessageFromBackend.bind(this), 0);
> 
> We should avoid this.dispatchNextQueuedMessageFromBackend.bind every time this happens and have a pre-bound version of this that we can call. Or eliminate all uses of "this." and convert to "WebInspector." and we won't need a bound function.

ok
Comment 7 Radar WebKit Bug Importer 2014-07-31 12:47:08 PDT
<rdar://problem/17874168>
Comment 8 Brian Burg 2014-07-31 12:47:56 PDT
Committed r171881: <http://trac.webkit.org/changeset/171881>