1485084 - Moving cursor in CodeMirror JsTerm triggers slow reflow

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Brad Werth [:bradwerth]]

**Steps to reproduce**
1. Open the console
2. Evaluate "\n".repeat(9000)
3. In the input, Hit Shift + Enter to enter new lines
4. Hit Arrow up to move the cursor one line up

This triggers a reflow. The fix for Bug 1482798 will make it a less-costly reflow, but there's no reason for it to trigger a reflow at all. A stack trace shows that during restyle, a div is inserted into the frame which then triggers reflow. Let's find the part of CodeMirror that is adding that div and see if we can route around it.

Comment 1

[Brad Werth [:bradwerth]]

Okay, after a conversation with Daniel Holbert, I have an understanding of why this is occurring, and some ideas for what we can do about it.

The CodeMirror code is doing some div remove/add to manage the cursor state. This is happening in the console input area. That remove/add of content is invalidating the size of the input area. That's fine and there's nothing we should try to change about that. The problem is that the the console input and output area are both children of a display: flex element, and when one child has its intrinsic size dirtied, all of the children have to be reflowed. For now, both display: flex and display: grid elements have this undesirable behavior. display: block elements do not.

So, potential fixes I can see:

1) Optimize nsFlexContainerFrame to avoid reflow of siblings. This is tough and there's Bug 1376536 open to address this.
2) Use CSS Containment on the input and output areas to keep reflows from spilling over. We don't have support for this yet. Bug 1150081 is tracking this feature.
3) Change the common parent of the input and output areas to be display: block. This might be possible with some alternative layout tricks. I'll explore that further in this bug.

Comment 2

[Brad Werth [:bradwerth]]

I haven't found a solution to this with a display: block containing element. The desired layout of the console is one that is only expressible in CSS as a flexbox layout (fixed height elements at top and bottom, expandable height element in center). As I explore this further, it's clear that the right solution is to solve the underlying problem in our implementation of the flexbox algorithm. There are already two bugs filed that are substantially the same: Bug 1376536 (WhatsApp), and Bug 1377253 (Twitter). This algorithm issue needs to be fixed. I'm going to close this bug as WONTFIX for now and see if I can assist in the flexbox algorithm changes.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Something that might be interesting: on my old laptop (~9 yo macbook pro - non-retina), STR from Comment 0 _does not_ trigger such slow reflow (it is not visible in the profile).
Typing is slow, but okayish (reflows seems to be half as costly as on my work machine). 
I'll post a profile later.

Comment 4

[Brad Werth [:bradwerth]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Something that might be interesting: on my old laptop (~9 yo macbook pro -
> non-retina), STR from Comment 0 _does not_ trigger such slow reflow (it is
> not visible in the profile).
> Typing is slow, but okayish (reflows seems to be half as costly as on my
> work machine). 
> I'll post a profile later.

This is addressed in Bug 1486996 where we determined that the difference between the two profiles was a OS-level setting for the visibility of scrollbars.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: wip patch for bug 1377253, which might help

Nicolas, would you mind testing this patch & seeing if it helps for you?

For me (in a local opt build), I can run the | "\n".repeat(9000) | step 5-10 times, and then when I type...
 document.write("ohai")
...I see the characters show up (and get colorized) ~instantly as I type them.  And if I capture a profile during this typing, I can see that the reflow time is negligible during the keypress handling time.

Comment 6

[Daniel Holbert [:dholbert]]

here's a profile of me, in a build with the patch in comment 5, typing "document.write" after I've done that "\n".repeat(9000) step a handful of times: https://perfht.ml/2CNl9u5

There's only 47ms of *total time* spent in nsFlexContainerFrame::Reflow over that whole 16-second profile, per this filtered view: https://perfht.ml/2x3NarK , which is pretty good. (And in most cases it's 1 sample (or no samples) for a single .offsetTop call while handling a event for the text-input.)

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Created attachment 9008233 [details] [diff] [review]
> wip patch for bug 1377253, which might help
> 
> Nicolas, would you mind testing this patch & seeing if it helps for you?
> 
> For me (in a local opt build), I can run the | "\n".repeat(9000) | step 5-10
> times, and then when I type...
>  document.write("ohai")
> ...I see the characters show up (and get colorized) ~instantly as I type
> them.  And if I capture a profile during this typing, I can see that the
> reflow time is negligible during the keypress handling time.

So, I applied the patch and run damp tests to get some numbers: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b0f8dda6a450c7eb81a4fcf3d556a64f61ffadb9&newProject=try&newRevision=8f3968f65310d999939109f157da47a7169da183&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=12

This is huge. I can't express how excited I am about this patch !

The biggest change is of course for console.typing, which is a test where there's 500 messages in the output, and we're typing 5 keystrokes, waiting between each of them for the autocomplete popup to update. This is going from 870ms to 130ms ! I strongly feel like typing in the console should not be an issue anymore, which is really awesome as it will enable us to ship CodeMirror JsTerm.

But that's not the only win, there are smaller (3 to 6%) improvements in the debugger as well, which is really nice.

I'm still waiting for my local build to be done so I can play with it locally, but looking at both damp results and the profile you linked to, this seems to be a very significant improvement.

Thanks a lot Daniel !

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

After some manual testing, all I said in Comment 7 still holds up. This is really fantastic 🎉

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #8)
> After some manual testing, all I said in Comment 7 still holds up. This is
> really fantastic 🎉

Great to hear! If we're seeing numbers like this on the console I'm sure that web content and other parts of the chrome will see it as well.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> (In reply to Daniel Holbert [:dholbert] from comment #5)
> > Created attachment 9008233 [details] [diff] [review]
> > wip patch for bug 1377253, which might help
> > 
> > Nicolas, would you mind testing this patch & seeing if it helps for you?
> > 
> > For me (in a local opt build), I can run the | "\n".repeat(9000) | step 5-10
> > times, and then when I type...
> >  document.write("ohai")
> > ...I see the characters show up (and get colorized) ~instantly as I type
> > them.  And if I capture a profile during this typing, I can see that the
> > reflow time is negligible during the keypress handling time.
> 
> So, I applied the patch and run damp tests to get some numbers:
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=try&originalRevision=b0f8dda6a450c7eb81a4fcf3d
> 556a64f61ffadb9&newProject=try&newRevision=8f3968f65310d999939109f157da47a716
> 9da183&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignatur
> e=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=12

By the way, comparing talos across an artifact and non-artifact build is unreliable. When you are testing a patch that requires --no-artifact you should also use that on the base.

Comment 11

[:Harald Kirschner :digitarald]

Adding [qf] as this might help out for general web as well.

Comment 12

[Daniel Holbert [:dholbert]]

Thanks for the testing, Nicolas!

BTW, removing [qf] -- the WIP patch that I posted here will ultimately end up on bug 1377253 (or a helper), which is already [qf:p1].

Comment 13

[Daniel Holbert [:dholbert]]

I'm adjusting the title slightly (to focus on the fact that it triggers a potentially *slow* reflow, which was really the problem here), and I'm calling this "fixed" by bug 1490890.

(Please reopen if that ends up not being the case, though.)

Comment 14

[Daniel Holbert [:dholbert]]

(and, stealing assignee field. :))