Summary: | Web Inspector: MessageDispatcher should not synchronously dispatch all backend messages | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||||
Component: | Web Inspector | Assignee: | 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
Brian Burg
2014-07-30 14:08:36 PDT
Created attachment 235761 [details]
Patch
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. Created attachment 235763 [details]
example output 1
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 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 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 Committed r171881: <http://trac.webkit.org/changeset/171881> |