1753177 - Use a virtualized list for the webconsole

Status: RESOLVED FIXED

(DevTools :: Console, task)

Description

[Nicolas Chevobbe [:nchevobbe]]

This is probably the most efficient way we could make the console faster.

Let's see how we could do that, and what the challenges are

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Impact

• We already have an initial phase where we only display the messages that fit the console (devtools/client/webconsole/components/Output/ConsoleOutput.js#230-239 ). We could probably get rid of it entirely.
• "Pinned to bottom" behavior could have to be re-written? We're using an IntersectionObserver for it at the moment (devtools/client/webconsole/components/Output/ConsoleOutput.js#93-102 )
• Export visible messages relies on DOM content. Would need to be updated

Possible Issues

• Variable message element height
• Layout changes caused by user action:
  • Expanding/Collapsing objects
  • Expanding/Collapsing stacks
  • Expanding/Collapsing groups
  • Filtering
  • Showing sidebar
  • Toggling Editor Mode
• Accessibility ?

Comment 2

[Alex Thayer [:alexical] (she/her)]

So, here's where this is currently at. I have a virtualized list up and running. It supports variable sized elements, and it pins the scrolling to the bottom if we were already there. The performance is night and day compared to the existing console - this is a profile of it handling an influx of 100000 messages: https://share.firefox.dev/3uorXY1. For context, the same amount of messages on today's firefox will make the browser completely unresponsive for several minutes.

So I'm fairly pleased with the result. However, there's a few things that still need to be sorted out:

1. Filtering of messages. I just need to dig into this a bit more. Filtering works when we're scrolled up to the top of the output list, but it really doesn't like it when we're already at the bottom. I'm not entirely sure what that's about but it looks like it will probably just involve a simple tweak.
2. Holding onto the expanded/collapsed state of messages after they're scrolled out of existence. This one is a bit more complicated - basically once we remove a list entry, we lose all the UI state associated with it. I'm not sure of the right path forward here, and I need to dig a little bit more into how that all fits together.

Once all of these are resolved, though, there's still more that we'll need to sort out to get the performance of this where I would like it to be (AKA, faster than Chrome's console.) We spend a lot of time in functions like getRepeatId and limitTopLevelMessageCount. There's a lot of complexity in the reducer side of things, and miscellaneous things show up all over there. I don't really know that Redux is a good fit for such a heavy workload. But I think that's out of the scope of this bug.

So, my current plan is just to fix points 1 and 2 above and put up a patch for this, and then start doing more exploration work on the broader performance picture.

Comment 3

[Alex Thayer [:alexical] (she/her)]

I've fixed the filtering of messages - it did turn out to just be a minor bug. I haven't started working on persisting the expand/collapse state of messages yet, because the performance of this was still not what I wanted it to be. The big problem was synchronous style/layout flushes from all of the getBoundingClientRect calls and friends. So I burned those away by using the chrome-only promiseDocumentFlushed (though I'll need to sort out permissions on that, as it's stricter in its requirements than I think it needs to be?) - but then I was left still needing synchronous flushes in order to get it to correctly pin the scrolling to the bottom. I found this post by Ryan Hunt from a while back, which actually mentions nchevobbe, but I wasn't able to get the technique to work with the virtualized list. I'm not quite sure why it wouldn't work. I was able to get something somewhat working using Element.mozScrollSnap() though, and the result was much more performant (profile). I think I'm going to ask around among layout people if there's a better way to pin the scrolling to the bottom, because the performance wins of not having to manage this in JS seem pretty clear.

Comment 4

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1753177 - Clean up unnecessary componentMap in MessageContainer.js r?nchevobbe

This is just some clean up - looks like this was once used to more dynamically
map strings to types, but now all of the mappings are explicitly accessed via
string literals.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1753177 - Use a virtualized list for webconsole output r?nchevobbe

Depends on D138030

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1753177 - Persist object expanded state when scrolling r?nchevobbe

Depends on D138031

Comment 7

[Alex Thayer [:alexical] (she/her)]

Attachment: WIP: Bug 1753177 - Big test fixup patch

Depends on D138610

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1753177 - [devtools] Make copy messages action compatible with virtualized console. r=dthayer.

This is a naive implementation where we re-create a ConsoleOutput
element, reusing the existing store, and passing a specific prop
so all the messages would be rendered.

Depends on D138031.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/012bf7c9090b
Clean up unnecessary componentMap in MessageContainer.js r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/fd0a28873bd1
Use a virtualized list for webconsole output r=nchevobbe

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/012bf7c9090b
https://hg.mozilla.org/mozilla-central/rev/fd0a28873bd1

Comment 13

[Phabricator Automation]

Comment on attachment 9269623 [details]
Bug 1753177 - [devtools] Make copy messages action compatible with virtualized console. r=dthayer.

Revision D142189 was moved to bug 1764236. Setting attachment 9269623 [details] to obsolete.