1516780 - Loading rust iterator docs feels much slower than in Chrome

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

It feels really weird that we're constantly restyling 16k elements when loading this page.

https://perfht.ml/2AorYOZ

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This might be related to uBlock origin, disabling it does make it better. And it makes sense because uBlock injects a lot of CSS on pages. Need to verify it though.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Profile without uBlock: https://perfht.ml/2Aoswo1

It is much better (the time spent in revalidation selectors drops for 44% to 4%). We're still re-cascading a whole lot of elements. I _suspect_ it's related to the web font loading and us recomputing it due to ex / ch units.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

A profile of https://doc.rust-lang.org/std/ascii/index.html (which is much smaller and thus style isn't invalidated just because of content insertions, screwing up the profiler's "invalidated at" popup) confirms that theory:

https://perfht.ml/2An099G

We have a PostRestyleEvent coming from nsPresContext::UserFontSetUpdated, which causes us to recalc the whole document.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

So the offending rule is:

.collapse-toggle > .inner {
  display: inline-block;
  width: 1.2ch;
  text-align: center;
}

And there's really something weird going on, since when I toggle that `width` declaration with the inspector, then we reload the font-faces over and over.

That may be an inspector bug actually. Patrick, do you know how the inspector toggles declarations in CSS rules? Do you remove all the rules in the stylesheet somehow?

STR is going to https://doc.rust-lang.org/std/ascii/index.html, inspecting the [+] / [-] button, and toggling the width: 1.2ch declaration, which is what triggers all this havoc.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

It'd be nice to load the @font-face rules earlier somehow, see https://github.com/webcompat/web-bugs/issues/22574.

Comment 6

[Jonathan Watt [:jwatt]]

You didn't set a priority so I'm just setting one to get this off the triage list. If you disagree with the priority feel free to change it of course.

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> That may be an inspector bug actually. Patrick, do you know how the
> inspector toggles declarations in CSS rules? Do you remove all the rules in
> the stylesheet somehow?
Yes the inspector does something like this:

- when a property is toggled, it rewrites the entire rule that the property belongs to:
  https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/devtools/shared/css/parsing-utils.js#880-918

- it then replaces this rule within the original stylesheet it belongs to:
  https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/devtools/server/actors/styles.js#1442-1444

- it then re-injects the entire stylesheet in the page: 
  https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/devtools/server/actors/stylesheets.js#529
  -> Using InspectorUtils's ParseStyleSheet method: https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/layout/inspector/InspectorUtils.cpp#643-648

Comment 8

[Sean Voisen (:svoisen)]

@Emilio: Didn't some of the webfont loading code change recently? Is it worth re-testing this now?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

It looks at least a bit better, yeah, but I'm pretty sure we're still restyling the whole document, nothing in that regard has changed...

Not quite sure how easy it is to do better, I'll give it some thought. Should be doable to store whether we have font-metrics-relative sizes and restyle just that rather than the whole page. We're walking the whole frame tree anyway to reflow just what we need so we may as well do that.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Let me think about this tomorrow.

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

STR is going to https://doc.rust-lang.org/std/ascii/index.html, inspecting
the [+] / [-] button, and toggling the width: 1.2ch declaration, which is
what triggers all this havoc.

So while it'd be great to improve our behavior here (along the lines of comment 9), the other point to mention is that width: 1.2ch feels like a poor approach to the styling here. It seems to be making the assumption that 1.2ch will result in a somewhat greater width than the "-" or "+" character by itself, so as to provide a bit of extra space around it (inside the brackets), but surely a more natural way to express that would be to use something like padding: 0 0.1em on the .inner span. If the CSS were modified along those lines, it would avoid the use of the potentially-expensive ch unit here.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Yeah, I agree. While looking into this I found that this is already pretty messy, see bug 1529537.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1516780 - Keep track of whether a style is affected by font metrics. r=#style,jfkthame

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1516780 - Remove ComputedStyle::mBits. r=#style,jfkthame

Use the Servo flags instead.

Depends on D20728

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1516780 - Optimize restyles when a font is loaded. r=#style,jfkthame

Depends on D20729

Comment 16

[Emilio Cobos Álvarez (:emilio)]

I wrote a thing. I think the last patch may need a bit of rework to handle display: contents, which I can do tomorrow...

Would need to switch to a separate DOM walk for that instead of reusing the frame tree walk. But the rest should be ok. This is all green of course:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91b32c78d8cdb23a75e4e027ba5fdff45bedebdc

Comment 17

[Daniel Holbert [:dholbert]]

RE the 1.2ch usage being inherently expensive/problematic (per comment 11 and 12) -- it looks like the problematic line of CSS lives here in the rust source tree:
https://github.com/rust-lang/rust/blob/f66e4697ae286985ddefc53c3a047614568458bb/src/librustdoc/html/static/rustdoc.css#L898

Perhaps worth submitting a pull request to suggest a trivial replacement?

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Well, sure, but then I run out of a test-case to profile ;)

Also, it looks like they now lazily insert the problematic element, by a time most of the fonts have already loaded, so the website is much faster now...

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69c8acfbec73
Keep track of whether a style is affected by font metrics. r=heycam
https://hg.mozilla.org/integration/autoland/rev/8238c8aeb851
Remove ComputedStyle::mBits. r=jfkthame,heycam
https://hg.mozilla.org/integration/autoland/rev/9499e5e85519
Optimize restyles when a font is loaded. r=heycam

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7e589bb49e3
fix rusttests by removing a now-unused declaration.

Comment 21

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/69c8acfbec73
https://hg.mozilla.org/mozilla-central/rev/8238c8aeb851
https://hg.mozilla.org/mozilla-central/rev/9499e5e85519
https://hg.mozilla.org/mozilla-central/rev/b7e589bb49e3

Comment 22

[Chris Peterson [:cpeterson]]

67=wontfix because there's no need to uplift to 67 Beta