1386443 - stylo: Consider using FxHash instead of FnvHash for the stylist and invalidation stuff.

Status: RESOLVED INCOMPLETE

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Emilio Cobos Álvarez (:emilio)]

According to Glenn this made a big difference in WebRender, and probably the hottest hashmaps in servo are in the Stylist, so we should at least profile with it instead.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

IRC conversation with links, fwiw (since I need to go to sleep now):

00:23 <@gw> If anyone is using Sip/Fnv hash in hot paths of Servo - you may like to try FxHash - see https://github.com/servo/webrender/pull/1538 for some numbers showing the performance wins in WR.
00:23 <crowbot> PR #1538: Switch hash table and maps to use FxHash instead of Fnv. - https://github.com/servo/webrender/pull/1538
00:26 <emilio> gw: that may also be nice for the slow rebuild stuff that bholley and bz are investigating, thanks!
00:27 <@gw> emilio: no worries, it definitely makes a big difference in WR!

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I just tested it with the testcase from bug 1382927, but with 4k iterations, and I get a 4x+ speedup (8s vs. 1.7s), so I'll work on land it.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/17946

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I just tested it with the testcase from bug 1382927, but with 4k iterations, and I get a 4x+ speedup 

I just tried applying this patch and testing on that testcase, and I see numbers pretty much the same as without this patch, fwiw.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> > I just tested it with the testcase from bug 1382927, but with 4k iterations, and I get a 4x+ speedup 
> 
> I just tried applying this patch and testing on that testcase, and I see
> numbers pretty much the same as without this patch, fwiw.

So I can't repro my numbers in my personal laptop, so I'm not sure what was happening to the nightly I was comparing against (for which I got ~19/~24s consistently...)

So I'd disregard that measuring. I'll try to get a profile of the nightly once I'm back at that computer.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Here's a profile of a run of the testcase that took over 22s on my usual profiling profile (with no addons other than the Gecko profiler):

  https://perfht.ml/2vvtTk0

However rm -rf'ing that directory makes it non reproducible again, so I'm not sure what was up with that profile at all.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Here's a profile of a run of the testcase that took over 22s on my usual
> profiling profile (with no addons other than the Gecko profiler):
> 
>   https://perfht.ml/2vvtTk0
> 
> However rm -rf'ing that directory makes it non reproducible again, so I'm
> not sure what was up with that profile at all.

Huh, so I found the reason for this, and it's that I had the |layers.acceleration.force-enabled| pref set to true from profiling bug 1372665... Why that pref makes things 4 to 5x slower, I don't know...

Comment 8

[Boris Zbarsky [:bzbarsky]]

That's pretty bizarre.  I would not expect that pref to affect this testcase at all... :(

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #8)
> That's pretty bizarre.  I would not expect that pref to affect this testcase
> at all... :(

FWIW, since I pasted it on IRC but not here: https://crisal.io/tmp/layers-accel-force-enabled.ogv

I changed the hasher to use the precomputed hash anyway. The only FnvHashMaps remaining are the stylesheet invalidation stuff, the namespaces hashmaps, and the media query invalidation stuff.

Since none of them are that hot, and we don't have that much strong evidence to justify this, I'm closing this as incomplete. We can measure it if we think it's worth again later.

Comment 10

[Darkspirit]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> FWIW, since I pasted it on IRC but not here:
> https://crisal.io/tmp/layers-accel-force-enabled.ogv

My results of
layers.acceleration.force-enabled with "true", "true with webrender+webrendest+layersfree+blob-image" and "false"
are all between 2370 and 2440.
Nightly 57 x64 20170824100243 @ Debian Testing (KDE Plasma/Xorg)

(Unrelated, but amazing: bholley's benchmark in https://news.ycombinator.com/item?id=15076968
Macbook Pro: Stylo_parallel=48.35ms, Stylo_sequential=156.72ms, Gecko=575.29ms, Chrome_Canary=196.53ms, Safari_11_Beta=223.50ms
vs.
My Octacore Debian: Stylo_parallel=74.50ms, Stylo_sequential=254.17ms, Gecko=293.27ms, Chrome_Dev=257.73ms)