Closed
Bug 1381270
Opened 7 years ago
Closed 7 years ago
stylo: Parse performance regression from CSS error reporting on apple.com
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bholley, Assigned: jdm)
References
(Blocks 1 open bug)
Details
Bug 1381045 handled the lion's share of the parse performance regression: https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=%5Bmozilla-central,dd55da63ebce86ee3867aa3b39975c2a90869ce2,1,1%5D&selected=%5Bmozilla-central,dd55da63ebce86ee3867aa3b39975c2a90869ce2,227622,298192965,1%5D However, if you look at the test breakdown, apple.com is still significantly slower: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&originalRevision=0e41d07a703f19224f60b01577b2cbb5708046c9&newProject=mozilla-central&newRevision=33d3192ed9ebe9e04d327e85c367f3e93cf9fb4d&originalSignature=dd55da63ebce86ee3867aa3b39975c2a90869ce2&newSignature=dd55da63ebce86ee3867aa3b39975c2a90869ce2&showOnlyConfident=1&framework=1 Loading the talos testcase for apple.com, I get the following profiles: Gecko: https://perfht.ml/2uvvjKL Stylo: https://perfht.ml/2uvsMjB So parsing is definitely slower, and Stylo definitely spends more time reporting CSS errors than Servo does. It's not clear to me whether that accounts for the entirety of the regression, but it's probably a good place to start.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(josh)
Comment 1•7 years ago
|
||
Are those profiles done at 0.5ms sampling interval? I ask because all the times seem to be multiples of 0.5. It sort of matters because I see 0.5ms spent under GetStringFromName in the Gecko profile and 2.0ms under FormatStringFromName in the servo profile instead, but if that's 1 vs 4 samples it could just be random... For the rest, we should certainly fix bug 1381195 and see how that affects things: right now stylo simply reports way more errors than Gecko does, esp. on sites using lots of -webkit- properties, and I bet apple.com does that.
Depends on: 1381195
Updated•7 years ago
|
Flags: needinfo?(bobbyholley)
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #1) > Are those profiles done at 0.5ms sampling interval? I ask because all the > times seem to be multiples of 0.5. Yes.
Flags: needinfo?(bobbyholley)
Comment 3•7 years ago
|
||
I filed bug 1381282 on the NS_NewURI calls during error reporting in those profiles.
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 4•7 years ago
|
||
In my local testing after bug 1381195, I see Gecko taking 2-3ms for reporting errors when loading the store.apple.com page, and Stylo dropped from 7ms to 3ms.
Flags: needinfo?(josh)
Reporter | ||
Comment 5•7 years ago
|
||
I still see a ~10 ms regression with the fix in bug 1381195: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&originalRevision=5d794bf4c4653153e602631f1b8818acd559d8f5&newProject=mozilla-central&newRevision=efc0b1525edbd357818dc7195537364e76f709e7&originalSignature=dd55da63ebce86ee3867aa3b39975c2a90869ce2&newSignature=dd55da63ebce86ee3867aa3b39975c2a90869ce2&framework=1 Can you have a look and see what accounts for it?
Flags: needinfo?(josh)
Assignee | ||
Comment 6•7 years ago
|
||
I made profiles of two builds: one with all of geckolib::error_reporter::ErrorReporter::report_error commented out one with with that code enabled. This means that I can see the impact of the actual integration with the console service, and infer that the remaining difference is due to the extra error data that is now being passed around the CSS parser. Unfortunately, I don't have a lot of confidence in the numbers I'm getting from the profiler - the difference between the two builds is ~10ms, but the amount of time spent under ErrorReporter::report_error only amounts to ~5ms of that. I'm finding it really difficult to figure out the next step here, but I've submitted https://github.com/servo/servo/pull/17787 which did improve the numbers I was seeing; let's take another look at talos after it merges.
Assignee | ||
Comment 7•7 years ago
|
||
Oh good, the more runs I do, the worse the results get: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&originalRevision=5d794bf4c4653153e602631f1b8818acd559d8f5&newProject=mozilla-central&newRevision=a8885191775ad1f655d2826f8f8e943a9a985f43&originalSignature=dd55da63ebce86ee3867aa3b39975c2a90869ce2&newSignature=dd55da63ebce86ee3867aa3b39975c2a90869ce2&framework=1 I'll try digging into myspace, since I'm out of ideas for store.apple.com.
Reporter | ||
Updated•7 years ago
|
Priority: P2 → --
Reporter | ||
Comment 8•7 years ago
|
||
Josh, where are we on this?
Comment 9•7 years ago
|
||
Assigning this bug to Josh because he has been investigating.
Assignee: nobody → josh
Priority: -- → P1
Assignee | ||
Comment 10•7 years ago
|
||
According to perfherder, stylo-sequential is now consistently performing better than non-stylo since a few days ago: https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=mozilla-central,1496972,0,1&series=mozilla-central,1324797,0,1&series=mozilla-central,1416655,1,1&series=mozilla-central,1481823,0,1&series=mozilla-central,1497044,1,1 . It's possible that this is a result of my changes in https://hg.mozilla.org/mozilla-central/rev/4c4093ead1b2; I'm going to go ahead and close this since we're achieved the result we want.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(josh)
Resolution: --- → FIXED
Updated•7 years ago
|
status-firefox57:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•