Closed
Bug 1485084
Opened 5 years ago
Closed 5 years ago
Moving cursor in CodeMirror JsTerm triggers slow reflow
Categories
(DevTools :: Console, defect, P3)
DevTools
Console
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bradwerth, Assigned: dholbert)
References
Details
Attachments
(1 file)
8.57 KB,
patch
|
Details | Diff | Splinter Review |
**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.
Reporter | ||
Comment 1•5 years ago
|
||
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.
Reporter | ||
Comment 2•5 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Comment 3•5 years ago
|
||
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.
Reporter | ||
Comment 4•5 years ago
|
||
(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.
Assignee | ||
Comment 5•5 years ago
|
||
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.
Flags: needinfo?(nchevobbe)
Assignee | ||
Comment 6•5 years ago
|
||
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•5 years ago
|
||
(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 !
Flags: needinfo?(nchevobbe)
Comment 8•5 years ago
|
||
After some manual testing, all I said in Comment 7 still holds up. This is really fantastic 🎉
Comment 9•5 years ago
|
||
(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•5 years ago
|
||
(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•5 years ago
|
||
Adding [qf] as this might help out for general web as well.
Whiteboard: [qf]
Assignee | ||
Comment 12•5 years ago
|
||
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].
Whiteboard: [qf]
Assignee | ||
Comment 13•5 years ago
|
||
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.)
Resolution: WONTFIX → FIXED
Summary: Moving cursor in CodeMirror JsTerm triggers reflow → Moving cursor in CodeMirror JsTerm triggers slow reflow
You need to log in
before you can comment on or make changes to this bug.
Description
•