Bug 135427

Summary: Web Inspector: MessageDispatcher should not synchronously dispatch all backend messages
Product: WebKit Reporter: Brian Burg <burg>
Component: Web InspectorAssignee: Brian Burg <burg>
Status: RESOLVED FIXED    
Severity: Normal CC: graouts, joepeck, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
timothy: review+, timothy: commit-queue-
example output 1
none
example output 2 none

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>