1362538 - stylo: Parallelize stylist::update

Status: NEW

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

Description

[Bobby Holley (:bholley)]

In my measurements, we spend about the same amount of time inserting elements into the rulehashes as we do into the dependency set. The SelectorMaps are disjoint, and so we could do this work in parallel.

We can even iterate the rules in opposite order, to potentially reduce cache contention on the refcount bumps (not sure if this is an issue or not, but doesn't hurt).

We could also potentially parallelize across the different rule hashes we use for the different cascade levels.

This touches all the stylist::update stuff, so I'm going to wait until bz finishes touching it first.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Note that @-moz-document support adding in bug 1355408 doesn't have thread-safe evaluation in its current form. Patches there have got r+ but haven't landed.

We can make it thread-safe via making the rule evaluated during parsing, but I thought that could be a low-priority optimization followup. If parallelizing stylist::update is high priority, we probably need to fix that first, either in that bug before its landing, or in another bug depending on it.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> Note that @-moz-document support adding in bug 1355408 doesn't have
> thread-safe evaluation in its current form. Patches there have got r+ but
> haven't landed.
> 
> We can make it thread-safe via making the rule evaluated during parsing, but
> I thought that could be a low-priority optimization followup. If
> parallelizing stylist::update is high priority, we probably need to fix that
> first, either in that bug before its landing, or in another bug depending on
> it.

Does this happen at the same time as media query evaluation? In general it would be good to avoid doing the media query evaluation on each thread, which would mean doing it beforehand on the main thread somehow. If all that's the case, then we wouldn't need to worry about -moz-document OMT.

Doing the above would probably require collect()ing into a vec though, which might be a bit of overhead. That said, bz and emilio were discussion media query optimizations and caching, which might mean caching the resulting rules anyway? NI emilio on the planned setup there.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Yeah, media-query evaluation is not thread-safe either AFAIK (probably can be done without too much effort). I'm not sure you can't do without evaluating the media queries on update(), what you'd do is avoiding to call update() itself when media query results haven't changed AFAIK.

That said, note that rule lists, both for stylesheets, media queries, and -moz-document are Arc'd, so collecting lists of rules that apply may be as simple as collecting a Vec<Arc<RuleList>>, which is probably ok if the wins from parallelizing it are enough.

Comment 4

[Bobby Holley (:bholley)]

Does that mean that each stylesheet will have N+1 effective rule lists, where N is the nu,ber of media queries + number of moz-document rules? If so that's great news. The refcount traffic for that would be fine (whereas the refcount traffic for Arc<Rule> might not be).

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Yes, I think so, at most N + 1 effective rulelists.

Comment 6

[Bobby Holley (:bholley)]

Ok great - then I think we should be able to just .collect() here, and don't need any thread-safety from @-moz-document handling.

Comment 7

[Bobby Holley (:bholley)]

See also bug 1368240 comment 82 as something to think about when we do this. We may not be able to coalesce the selectors walks because of the parallelism, but we should think of how to balance the jobs as best we can.

Comment 8

[Bobby Holley (:bholley)]

Here's a testcase where we spend a fair amount of time in flush_stylesheets, which should be helped by this bug: http://tests.themasta.com/twitter-compact/twitter-main.html

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I think there are better improvements we should try here, like just tracking the kind of stylesheets to rebuild, see bug 1382927.

Comment 10

[Bobby Holley (:bholley)]

I noticed that stylist updates are still hurting us on youtube. I wrote up a prototype to see how much of a win there is to be had here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9dfc58b593c485e2b154f2a9c5d5b5d998554b0

Comment 11

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57f30376cc111fa1bfdc99b67908146854ecd14

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I can also take a look at what youtube is doing. If a page load is rebuilding the same stylesheet fully twice, it may fall under the umbrella of remaining optimizations for the incremental rebuild stuff, I doubt they're removing sheets.

Bobby, is this just the YT front-page?

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Setting ni? for :emilio's question in comment 12.

Comment 14

[Bobby Holley (:bholley)]

Oh yeah, to clarify - this is on the youtube page from [1].

I have patches here that work and measurably reduce the overhead. However, given that we're near the end of the cycle, adding a new source of parallelism here is probably not advisable. I'm going to get my patches organized and then we can pick them back up in 58.

[1] http://github.com/rwood-moz/talos-pagesets/tree/master/tp6

Comment 15

[Bobby Holley (:bholley)]

I don't have time to unbitrot the patches, but they're here:

https://github.com/bholley/gecko/tree/parallize_stylist_rebuild

Comment 16

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57