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)

defect

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.
Flags: needinfo?(josh)
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
Flags: needinfo?(bobbyholley)
(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)
I filed bug 1381282 on the NS_NewURI calls during error reporting in those profiles.
Priority: -- → P2
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)
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.
Priority: P2 → --
Josh, where are we on this?
Assigning this bug to Josh because he has been investigating.
Assignee: nobody → josh
Priority: -- → P1
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
You need to log in before you can comment on or make changes to this bug.