1502524 - Style .CodeMirror-scroll with "contain:layout size"

Status: RESOLVED WONTFIX

(DevTools :: Console, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Once we have bug 1497414 and we've preffed on CSS containment for frontend code (bug 1501492), one good candidate is the CodeMirror stuff that's caused layout issues in the past.

Ideally, we want to contain the area where the user is typing, so that reflows from typing won't waste time reflowing/traversing subtrees & ancestors that aren't affected by that typing.

In particular, we want to add "contain: layout size" styling to some element that is not dependent on its content for sizing & that is as close to the user's typed characters as possible.

From looking at the console in Browser Toolbox, it looks like one good candidate is the ".CodeMirror-scroll" element. Its size does not seem to be influenced by its content (it has "width:auto; height:100%; overflow: scroll"), so we should be OK to give it "contain:layout size" without breaking its behavior at all.

I am guessing that style would go somewhere in here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/codemirror/mozilla.css

Comment 1

[Daniel Holbert [:dholbert]]

After applying the patches for the dependent bugs, in a build with "ac_add_options --enable-debug-symbols --enable-optimize", I tried out the following experiment, before & after adding the "contain:layout size" rule that I'm suggesting in comment 0 here:

1. Open a new tab, and visit about:blank
2. Ctrl+shift+K
3. Execute the following statement:
 "\n".repeat(9000)
4. Start profiling.
5. Hold down the "a" key on your keyboard, and once a single line of text is filled up, let go of the key. (If you're doing it right, you should end up with a few characters on the second line, no more than ~6.)
6. Capture profile.


Results:
(URL provided is filtered for "reflow", and the reported measurement is milliseconds spent with "reflow" in the stack):

* No "contain" styling:
http://bit.ly/2Sjop3Y 590.8
http://bit.ly/2SmF5aU 568.2
http://bit.ly/2Ss2hoe 579.4
Average: 579.5ms

* With "contain: layout size" on .CodeMirror-scroll:
http://bit.ly/2SkXJzO 253.6
http://bit.ly/2SnYiJ3 254.8
http://bit.ly/2SjpGYO 244.0
Average: 250.8ms

Comparing the averages, this gets us a 56.7% reduction in time spent in reflow.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1502524: Style devtools' "CodeMirror-scroll" element with "contain: layout size", to hint that it can be a reflow root. r?nchevobbe

This should improve typing resposiveness in the web console, by allowing
reflows to be initiated from an ancestor that's closer to the changed content
(the typed characters).

Comment 3

[Daniel Holbert [:dholbert]]

Here's a comparison between 2 runs to see if this gets us any wins in the "DAMP" typing-in-console test (from bug 1487359), too:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c17a3206712d815cc06c96df0fa0d71b9f2f67b3&newProject=try&newRevision=6bb4d342ba2d51908333ee1706c9a12642ebead3

(originalRevision has dependent bugs' patches; newRevision has that same patch stack plus this bug's patch.)

Comment 4

[Daniel Holbert [:dholbert]]

(Also: just for fun, I tried reverting bug 1490890's gecko flexbox patch that helped us with CodeMirror perf, to see how much "contain" helps us in the absence of that optimization.  (Specifically: I added back the loop of DeleteProperty() calls that were removed in that bug's patch.)

In that build (with that optimization & without any contain styling), I performed Comment 2's experiment, and we are just terrible - after ~4 "a" characters appear, we hang for ~5 seconds, and then paint some more a's, and then we hang for tens of seconds when the first line is about half filled.  Here's a profile of me trying to perform the STR, in a build like that:
  http://bit.ly/2Speb1Q
  (This ended up with about a line and a half of "a" characters, because I couldn't tell when I was approaching the end of the first line because we were hanging.)

If I add the "contain:layout" styling, my Comment 2 experiment goes back to normal -- it ends up at an intermediate performance level between the values reported in comment 2:
http://bit.ly/2z5jF9i 438.4
http://bit.ly/2zaHA7c 435.0

Notably, these measurements are lower than the "baseline" measurements in comment 2 (which were in the 568-590ms range).  So in other words, the targeted "contain:layout style" application in this bug's patch is *slightly more effective* than bug 1490890 was, at making us perform well on the experiment in Comment 2!  But of course, we're even better with both combined, as shown by the final results in comment 2. :)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> In that build (with that optimization & without any contain styling), I
> performed Comment 2's experiment, and we are just terrible

Sorry, typo -- I left out a word here. I meant to say "(with that optimization _removed_ [...]"

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Thanks Daniel for doing this, the results from DAMP are quite interesting! https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=c17a3206712d815cc06c96df0fa0d71b9f2f67b3&newProject=try&newRevision=6bb4d342ba2d51908333ee1706c9a12642ebead3&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=12 

It shows a performance regression on the console typing test, but that's not relevant given the profile you recorded.

The most important gains seem to be on the debugger (which use codeMirror to display the js files). 

Could you push your WIP patch somewhere where it can be pulled so we can check everything react as expected in the console, debugger and style editor?

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Ignore my previous request, I see now that the patch is on phabricator (I'll have another cup of coffee)

Comment 8

[Daniel Holbert [:dholbert]]

(Thanks for the better perfherder link -- mine from comment 3 wasn't to the correct area.)

> The most important gains seem to be on the debugger (which use codeMirror to display the js files). 

Nice! That makes sense -- the difference between "before" vs. "after" there is a single codeMirror-specific declaration.

RE the patches if you want to try them locally -- yeah, you'd want to use the patches from phabricator on this bug, plus the two dependent bugs (bug 1497414 and bug 1501492).  Or you can "hg import" them from the try push, for a little while at least - you should be able to "hg import" each of these URLs:

patch 1 (bug 1497414): https://hg.mozilla.org/try/raw-rev/4c6bc81301f7
patch 2 (bug 1501492): https://hg.mozilla.org/try/raw-rev/adffbc6378c7
patch 3 (this bug):    https://hg.mozilla.org/try/raw-rev/e291ce2fb82d

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/713eea9bd41b
Style devtools' "CodeMirror-scroll" element with "contain: layout size", to hint that it can be a reflow root. r=nchevobbe

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/713eea9bd41b

Comment 11

[Daniel Holbert [:dholbert]]

Backed out:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/b0d5f304e740f0dcd1516d78270f5d731cf4a054
...for causing bug 1514763.

In particular, I realized that the element in question here actually *does* want to depend on its contents' size (which is why the text entry area gets larger when you press "shift+enter" in the console).  And size-containment should break that that dependency, in a way that breaks the intended rendering.  It wasn't entirely breaking the dependency, due to a containment bug that I've now field as bug 1514843.

So, as-filed, this bug is probably "wontfix", and I don't think we can fix it without breaking the graceful "text input box gets taller as you enter more lines of input" behavior that the web console has.