Closed Bug 1362982 Opened 8 years ago Closed 7 years ago

stylo: Stylo 5%-10% slower than Gecko for setting innerHTML

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jseward, Unassigned)

References

Details

Using the testcase at https://bugzilla.mozilla.org/show_bug.cgi?id=1347525#c13 (letting it run through and reporting the best value (milliseconds)) for ("gecko") m-c, opt, non-debug, non-Stylo, -O2 vs ("stylo") m-c, opt, non-debug, Stylo, -O2, STYLO_THREADS=1 on Linux x86_64, Fedora 25, Intel(R) Xeon(R) CPU E3-1505M v5, 2.6 GHz. (4 cores, 8 threads, 8 MB L3) Results: gecko: 55 53 stylo: 57 59 It looks as if Stylo is 5%-10% slower than Gecko for this test. Mozconfigs and command line are below. It's unclear to me what the effect on Rust optimisation is of setting --enable-optimize="-g -O2" in the Stylo case. For Stylo, the Clang version was 3.9. gcc is 6.3.1 and rustc is 1.16.0. -- command line ---------------------------------- STYLO_THREADS=1 DISPLAY=:2.0 ./gcc-O2-nondebug-stylo/dist/bin/firefox-bin -P dev -no-remote file:///home/sewardj/MOZ/STYLO/bug1347525testcase_looping2.html -- mozconfig gecko ------------------------------- . $topsrcdir/browser/config/mozconfig mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/gcc-O2-nondebug-plain ac_add_options --enable-tests ac_add_options --enable-optimize="-g -O2" ac_add_options --enable-debug-symbols ac_add_options --enable-profiling ac_add_options --disable-crashreporter mk_add_options MOZ_MAKE_FLAGS="-j8" mk_add_options AUTOCLOBBER=1 -- mozconfig stylo ------------------------------- . $topsrcdir/browser/config/mozconfig mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/gcc-O2-nondebug-stylo ac_add_options --enable-tests ac_add_options --enable-optimize="-g -O2" ac_add_options --enable-debug-symbols ac_add_options --enable-stylo ac_add_options --enable-valgrind ac_add_options --enable-profiling ac_add_options --enable-elf-hack ac_add_options --disable-crashreporter mk_add_options MOZ_MAKE_FLAGS="-j8" mk_add_options AUTOCLOBBER=1
I'm not getting as high regression, but there is some. In stylo enabled build (layout.css.servo.enabled == true), nsNodeUtils::ContentAppended becomes a bit slower, mostly because nsPresShell::ContentAppended calls nsCSSFrameConstructor::ContentAppended which then calls nsCSSFrameConstructor::LazilyStyleNewChildRange. LazilyStyleNewChildRange calls are done only when isNewlyAddedContentForServo or some such is true. This stuff just isn't there at all when layout.css.servo.enabled == false or non-gecko build. Still looking these profiles a bit more.
On average the numbers seem to be higher (slower) with just stylo build, even with layout.css.servo.enabled == false. Still investigating.
I think this should be tested on non-linux platforms too. I don't right now have access to such machines.
(In reply to Olli Pettay [:smaug] from comment #2) > On average the numbers seem to be higher (slower) with just stylo build, > even with layout.css.servo.enabled == false. Still investigating. If you find a perf regression that reproduces with stylo built but disabled, that's much more urgent - please file separately and I'll get someone to jump on it.
Summary: Stylo 5%-10% slower than Gecko for setting innerHTML → stylo: Stylo 5%-10% slower than Gecko for setting innerHTML
So far I haven't found anything concrete in the profiles. It may hint that use of memory caches is worse because of larger Element objects, or it may not.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4) > (In reply to Olli Pettay [:smaug] from comment #2) > > On average the numbers seem to be higher (slower) with just stylo build, > > even with layout.css.servo.enabled == false. Still investigating. > > If you find a perf regression that reproduces with stylo built but disabled, > that's much more urgent - please file separately and I'll get someone to > jump on it. That seems like something that the Stylo team should be investigating actively as part of investigating the performance implications of enabling Stylo. We'll file bugs if we notice something, but please don't assume someone else is doing this investigation. :-0
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #6) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4) > > (In reply to Olli Pettay [:smaug] from comment #2) > > > On average the numbers seem to be higher (slower) with just stylo build, > > > even with layout.css.servo.enabled == false. Still investigating. > > > > If you find a perf regression that reproduces with stylo built but disabled, > > that's much more urgent - please file separately and I'll get someone to > > jump on it. > > That seems like something that the Stylo team should be investigating > actively as part of investigating the performance implications of enabling > Stylo. We'll file bugs if we notice something, but please don't assume > someone else is doing this investigation. :-0 The point here is that smaug was reporting results that I would not expect, and don't plan to spend much time chasing unless we have STR (or at least a definite claim that there is actually a slowdown) or a regression on Talos. Building stylo by default (but preffing it off) is no different than any run-of-the-mill platform patch - it adds some fields and branches here and there, but nothing that I would expect to alter our basic performance characteristics. So while I'd certainly be very interested to hear about issues and would follow up, the risk profile of this change is just the same as landing any other platform feature preffed off.
I wouldn't be at all surprised if just adding 8 bytes to sizeof Element would regress performance. That is a major change to the core of DOM after all.
(In reply to Olli Pettay [:smaug] from comment #8) > I wouldn't be at all surprised if just adding 8 bytes to sizeof Element > would regress performance. > That is a major change to the core of DOM after all. If we're concerned about this, I can rearrange some things to avoid growing Element at least on 64-bit. I'll file a bug.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9) > (In reply to Olli Pettay [:smaug] from comment #8) > > I wouldn't be at all surprised if just adding 8 bytes to sizeof Element > > would regress performance. > > That is a major change to the core of DOM after all. > > If we're concerned about this, I can rearrange some things to avoid growing > Element at least on 64-bit. I'll file a bug. Filed bug 1363375 about this.
smaug, now that bug 1363375 landed, are you still seeing a regression from simply building stylo?
Flags: needinfo?(bugs)
I assume https://bugzilla.mozilla.org/show_bug.cgi?id=1362982#c1 hasn't disappeared, but let me re-profile
Times seem to vary quite a lot, but https://bug934607.bmoattachments.org/attachment.cgi?id=8872948 definitely does shows a regression, and comment 1 is still very valid.
Flags: needinfo?(bugs)
Ok - but the build-time regression has gone away?
The noise is quite bad, but at least servo build with the pref disable is never faster than non-servo build. I'd say in general it seems to be a bit slower. But worth to try on non-linux too.
I modified the testcase in bug 1347525 to track averages, and measured a vanilla local m-c build against a stylo build (with stylo enabled). This is on new OSX MBP. Measurements are: With stylo: * avg: 54.4, best: 40.95 * avg: 53.8, best: 42.8 * avg: 54.2, best: 46.6 Without stylo: * avg: 54.3, best: 46.8 * avg: 53.3, best: 42.3 * avg: 52.7, best: 46 * avg: 54.1, best: 48.1 That does not look to me like an indicator that we should be spending time on this.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.