Bug 136984 – Web Inspector: console view takes a long time to render if there are many messages

• Status: NEW
• Component: Web Inspector

[Brian Burg]

It seems like we try to load up all the messages before showing the full-size console view. This is a problem if there are 100's of message in the console. (A good test case is using 2nd level inspector while dumping protocol traffic via the switch in InspectorBackend.)

Since old output scrolls off the top, we should probably be showing the view immediately then incrementally rendering messages from newest to oldest.

[Radar WebKit Bug Importer]

<rdar://problem/18407035>

[Nikita Vasilyev]

> A good test case is using 2nd level inspector while dumping protocol traffic via the switch in InspectorBackend.

Could you explain how to do that? I’m not familiar with that switch.

I’m in the process of rewriting console views (LogContentView.js, JavaScriptLogView.js, ConsoleGroup.js, etc.) and I’d like to fix all issues related to console’s slugishness.

[Joseph Pecoraro]

(In reply to comment #2)
> > A good test case is using 2nd level inspector while dumping protocol traffic via the switch in InspectorBackend.
> 
> Could you explain how to do that? I’m not familiar with that switch.

He is referring to InspectorBackend.dumpInspectorProtocolMessages.

Set that to true, and the 2nd level inspector will dump to console all frontend <-> backend traffic.

---

Just a quick glance at the code:

  - InspectorConsoleAgent stores the last 1000 messages while no inspector is open
  - InspectorConsoleAgent::enable sends all of them to the frontend in order (old -> new)

[Nikita Vasilyev]

I’ll do progressive rendering, batching by something between 10 and 32 messages. 32 is number of one-line messages fitting in the console on my 15 inch MacBook Pro: 
https://cldup.com/vzs8ntl1ew.png

[Timothy Hatcher]

(In reply to comment #4)
> I’ll do progressive rendering, batching by something between 10 and 32
> messages. 32 is number of one-line messages fitting in the console on my 15
> inch MacBook Pro: 
> https://cldup.com/vzs8ntl1ew.png

Sounds good.

[Nikita Vasilyev]

Created attachment 247503 [details]
WIP

This patch does batch updates only 3 messages at the time because it’s easier to debug this way.

Before looking at the patch, please note that I was in the middle of ConsoleGroup refactoring, which was intersected with this patch.

Brian, let me know if it solves the original problem. Even with InspectorBackend.dumpInspectorProtocolMessages turned on, it was fairly fast for me, so I couldn’t quite reproduce it.

[Nikita Vasilyev]

s/was intersected/was intersecting/

(I’ll learn English grammar one day)

[Timothy Hatcher]

Comment on attachment 247503 [details]
WIP

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

So this batch updates on new messages but it does not cap the max messages we should in the DOM. I think it would still be good to cap the max we put in the DOM in the first place so switching to the console is fast and won't eat memory and slow down the UI with too many DOM nodes in the tree.

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:282
> +        this._batchUpdateId = requestAnimationFrame(function() {

I would break out the inline function into a named function since it is large. It can still be a closure, just named. See NavigationSidebarPanel.js _checkForOldResources for an example (that uses setTimeout when we likely should consider using rAF more.)

At first I wondered what _batchUpdateId was, I thought it was some id you used but it is the rAF id. I would put "TimeoutIdentifier" or "AnimationIdentifier" as a suffix.

Also we spell out Identifier. https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#Namingthings

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:315
> +            //@nvasilyev: extra console.groupEnd() call can make console group go negative margin, not good.

We should just ignore the extra then. Would be good to file a separate bug for that.

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:331
>          if (this.delegate && typeof this.delegate.didAppendConsoleMessage === "function")
>              this.delegate.didAppendConsoleMessage(msg);

I wonder if this should fire only after the batch update?

[Nikita Vasilyev]

With bug 155629 landed, now it's very straightforward to implement progressive rendering.