Closed Bug 1420741 Opened 7 years ago Closed 7 years ago

stylo: page takes 5 min to load

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
57 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 + fixed
firefox59 - fixed

People

(Reporter: lud.janvier, Assigned: emilio)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: perf, regression)

Attachments

(4 files)

Problematic change that causes the issue.
(deleted), text/plain
Details
Bug 1420741: Require an nth-index-cache for invalidation.
(deleted), text/x-review-board-request
xidorn
: review+
Details
Bug 1420741: Log a bit more information about invalidation collection.
(deleted), text/x-review-board-request
xidorn
: review+
Details
Bug 1420741: Bail out from invalidation if we're the root and got tons of descendant invalidations.
(deleted), text/x-review-board-request
heycam
: review+
gchang
: approval-mozilla-beta+
Details
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20171115095126 Steps to reproduce: go to this page: https://puppet.com/docs/puppet/latest/type.html try to scroll down try to switch to other tabs Actual results: cannot scroll down in this page the "load" event may take between 2 and 5min during this time, firefox is using 100% of CPU time not a network issue other tabs become unusable until firefox's restart tested with empty profiles (no extensions, clean DBs), Linux (Ubuntu) and Windows (8) Expected results: this page is loading in less than 5 sec in previous versions of firefox (<=45) and other browsers.
Mac OS build is affected too
Component: Untriaged → General
Keywords: perf
Product: Firefox → Core
6:12.86 INFO: No more inbound revisions, bisection finished. 6:12.86 INFO: Last good revision: 5f721a664bf64fed99184a866b60c24a6afcb3a0 6:12.87 INFO: First bad revision: 62cebea1d1578461a423a9ca7848706455db9ea5 6:12.87 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5
Blocks: stylo-nightly
Severity: normal → major
Status: UNCONFIRMED → NEW
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox59: --- → affected
status-firefox-esr52: --- → unaffected
Component: General → CSS Parsing and Computation
Ever confirmed: true
Keywords: regression
Blocks: stylo-site-issues
URL: https://puppet.com/docs/puppet/latest...
Has Regression Range: --- → yes
Has STR: --- → yes
Summary: quantum regression page takes 5 min to load → stylo: page takes 5 min to load
Flags: needinfo?(emilio)
This page is making the following change in the <html> element: Collecting changes for: <html class="no-touchevents details csscalc no-is-touch"> (0x7fca06af9430) > state: (empty) > id changed: false -> +None -None > class changed: true -> +[Gecko Atom(0x7fc9f77e6600, csscalc), Gecko Atom(0x7fc9f1cffda0, no-is-touch)] -[] (That is, adding the classes "csscalc" and "no-is-touch"). That causes all of those selectors to change in the page. We actually try to go down and find the exact elements that require the change to update them, and we take forever... Not sure if that's because of all the nth-child selectors, which are super-slow because in this path we don't have an nth-index cache, or just because of the number of selectors. Both should be fixable (first -> add the cache, second -> bound the number of invalidations somehow). Will try the first for now.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Hey all, do we feel this is something we might uplift to 57?
(In reply to Jim Mathies [:jimm] from comment #9) > Hey all, do we feel this is something we might uplift to 57? That'd be nice I'd say... Given cam is on PTO I'll see if Xidorn can review this to get it landed soon.
Attachment #8932422 - Flags: review?(cam) → review?(xidorn+moz)
Attachment #8932423 - Flags: review?(cam) → review?(xidorn+moz)
Attachment #8932424 - Flags: review?(cam) → review?(xidorn+moz)
Comment on attachment 8932422 [details] Bug 1420741: Require an nth-index-cache for invalidation. https://reviewboard.mozilla.org/r/203466/#review209466 ::: servo/ports/geckolib/glue.rs:4469 (Diff revision 1) > - // FIXME(emilio): an nth-index cache could be worth here, even > - // if temporary? > + // FIXME(emilio): Ideally we could share the nth-index-cache across all > + // the elements? Yeah that may be useful too, but probably wouldn't be as significant as this one.
Attachment #8932422 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8932423 [details] Bug 1420741: Log a bit more information about invalidation collection. https://reviewboard.mozilla.org/r/203468/#review209518
Attachment #8932423 - Flags: review?(xidorn+moz) → review+
Attachment #8932424 - Flags: review?(xidorn+moz) → review?(cam)
Comment on attachment 8932424 [details] Bug 1420741: Bail out from invalidation if we're the root and got tons of descendant invalidations. https://reviewboard.mozilla.org/r/203470/#review209520 ::: servo/components/style/invalidation/element/collector.rs:238 (Diff revision 1) > + // This number is completely made-up, but the page that made us add this > + // code generated 1960+ invalidations (bug 1420741). > + if element.is_root() && descendant_invalidations.len() > 150 { I... don't really buy this heuristic... Why it's only for root? Should we do something if some non-root element generates tons of descendant invalidation as well? Would this perform better if we use some different heuristic, e.g. ratio of number of invalidations to number of elements? Actually, why do we need this at all? Why doesn't Gecko has the same issue? Maybe it just doesn't invalidate but instead simply restyles everything?
Comment on attachment 8932424 [details] Bug 1420741: Bail out from invalidation if we're the root and got tons of descendant invalidations. https://reviewboard.mozilla.org/r/203470/#review209520 > I... don't really buy this heuristic... > > Why it's only for root? Should we do something if some non-root element generates tons of descendant invalidation as well? > > Would this perform better if we use some different heuristic, e.g. ratio of number of invalidations to number of elements? > > Actually, why do we need this at all? Why doesn't Gecko has the same issue? Maybe it just doesn't invalidate but instead simply restyles everything? Right, gecko just restyles the world all the time, which is slower 99% of the time but... :) Doing it with the number of invalidations and elements down the tree would be nice, but you'd need to actually traverse the tree before-hand, so the root element is just an easy way of picking an element with tons of descendants... I could remove the is_root requirement I guess.
Comment on attachment 8932424 [details] Bug 1420741: Bail out from invalidation if we're the root and got tons of descendant invalidations. https://reviewboard.mozilla.org/r/203470/#review209520 > Right, gecko just restyles the world all the time, which is slower 99% of the time but... :) > > Doing it with the number of invalidations and elements down the tree would be nice, but you'd need to actually traverse the tree before-hand, so the root element is just an easy way of picking an element with tons of descendants... I could remove the is_root requirement I guess. Yes, I think we should remove that is_root check since I can totally believe that we can run into the same situation except the page is twiddling classes on the <body>. I think it's hard to come up with a good heuristic for the number invalidations to check for, since we don't actually track the number of elements in the document. 150 sounds fine to start with.
Also, did we plan to process invalidations using rayon at some point?
Comment on attachment 8932424 [details] Bug 1420741: Bail out from invalidation if we're the root and got tons of descendant invalidations. https://reviewboard.mozilla.org/r/203470/#review209656 r=me with that, and the comment updated.
Attachment #8932424 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #15) > Yes, I think we should remove that is_root check since I can totally believe > that we can run into the same situation except the page is twiddling classes > on the <body>. sorry for interruption. I saw some pages in the wild which do those feature toggles on the <html> element instead of the <body>. maybe this is a case which helps to prove your thesis.
I think it is fine to support limiting the number of invalidations we process whether it's the <html>, the <body>, or any other element.
status-firefox57: affected → wontfix
status-firefox59: affected → fixed
tracking-firefox58: --- → ?
tracking-firefox59: --- → ?
Target Milestone: --- → mozilla59
I understand if you want to let this bake for a bit, but I think this is bad enough that we'll want a Beta uplift request whenever you're comfortable doing so.
Flags: needinfo?(emilio)
Comment on attachment 8932424 [details] Bug 1420741: Bail out from invalidation if we're the root and got tons of descendant invalidations. (In reply to Ryan VanderMeulen [:RyanVM] from comment #21) > I understand if you want to let this bake for a bit, but I think this is bad > enough that we'll want a Beta uplift request whenever you're comfortable > doing so. I don't think this patch is risky at all. Note that the patch landed addresses the review comments and is https://hg.mozilla.org/mozilla-central/rev/6f509e2dd704908cfcb5e06514d01130e0722145 Approval Request Comment [Feature/Bug causing the regression]: stylo [User impact if declined]: long page load time in the bug URL website. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: not risky [Why is the change risky/not risky?]: simple patch, just bails out from a fine-grained invalidation invalidating all children. [String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8932424 - Flags: approval-mozilla-beta?
Track 58+ as this is a severe perf regression.
tracking-firefox58: ? → +
tracking-firefox59: ? → -
Comment on attachment 8932424 [details] Bug 1420741: Bail out from invalidation if we're the root and got tons of descendant invalidations. Take this to fix perf issue. Beta58+.
Attachment #8932424 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: