Loading rust iterator docs feels much slower than in Chrome

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Depends on 1 bug, {perf})

unspecified
mozilla68
Points:
---

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

Details

(Whiteboard: [qf:p1:pageload], URL)

Attachments

(3 attachments)

(Assignee)

Description

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

https://perfht.ml/2AorYOZ
(Assignee)

Comment 1

5 months ago
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.
(Assignee)

Comment 2

5 months ago
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.
(Assignee)

Comment 3

5 months ago
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.
(Assignee)

Comment 4

5 months ago
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.
Flags: needinfo?(pbrosset)
(Assignee)

Comment 5

5 months ago
It'd be nice to load the @font-face rules earlier somehow, see https://github.com/webcompat/web-bugs/issues/22574.
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.
Priority: -- → P3
Whiteboard: [qf] → [qf:p1:pageload]
(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
Flags: needinfo?(pbrosset)

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

Flags: needinfo?(emilio)
(Assignee)

Comment 9

3 months ago

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.

Flags: needinfo?(emilio)
(Assignee)

Comment 10

3 months ago

Let me think about this tomorrow.

Flags: needinfo?(emilio)

(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.

(Assignee)

Updated

3 months ago
See Also: → 1529537
(Assignee)

Comment 12

3 months ago

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

(Assignee)

Comment 14

3 months ago

Use the Servo flags instead.

Depends on D20728

(Assignee)

Comment 16

3 months ago

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

Assignee: nobody → emilio
Flags: needinfo?(emilio)
(Assignee)

Updated

3 months ago
See Also: → 1530212

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?

Flags: needinfo?(emilio)
(Assignee)

Comment 18

2 months ago

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...

Flags: needinfo?(emilio)

Comment 19

2 months ago
Pushed by ealvarez@mozilla.com:
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

2 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/b7e589bb49e3
fix rusttests by removing a now-unused declaration.

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

You need to log in before you can comment on or make changes to this bug.