1362532 - stylo: Share the rulehashes for UA sheets across documents

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

Gecko does this, seems like we might as well do it too to avoid re-inserting the UA sheets into the rule hash for every page.

We can store it in the GlobalStyleData, and blow it away when the UA sheets change. We'll also need to disable this optimization if there are ever media queries in the UA sheets.

Comment 1

[Cameron McCormack (:heycam)]

FWIW there already are:

http://searchfox.org/mozilla-central/search?q=%40media&case=false&regexp=false&path=layout%2Fstyle%2Fres%2F*.css

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Per Boris' measurements, this may be the bigger cause of the AWSY regression. I can do the plumbing and implement this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Specifically, Emilio and I suspect that this is the primary reason that servo-style-sets is so much bigger than gecko-style-sets on awsy (about 2x, or about 10MB regression on an 8MB baseline, in the three TabsOpen checkpoints).

The data in support of that hypothesis is as follows:

1)  The smallest gecko-style-sets number on awsy is 608 bytes.  There are 9 of those.  There are 4 more that
    have numbers that are about 4KB or less.

2)  Some of those 608-byte things are about:blank iframes (e.g. on myspace albumart.html).

3)  The element-and-pseudos-map value for stylo on those about:blank iframes on albumart.html is 83840 bytes.

4)  The invalidation-map value for stylo on those about:blank iframes on albumart.html is 19536 bytes.

5)  The revalidation-selectors value for stylo on those about:blank iframes on albumart.html is 23424 bytes.

5)  Spot-checking some of the other memory report paths that have a low gecko-style-sets number, all of them
    have element-and-pseudos-map + invalidation-map in the 126KB range.

6)  If I count things correctly, there are about 70 live documents in content processes on AWSY in the TabsOpen condition.

Items 1-5 suggest that the ua-level stylist takes at least 126KB.  Item 6 suggests that corresponds to 8.8MB of memory.

Gecko's rule-processor-cache, on the awsy about:memory dumps, is about 200KB per content process and ~360KB in the main process.  Ignoring the main process, that means about 800KB of memory, vs 7MB.  Not only that, but it looks like Gecko's rule-processor-cache is somewhat populated in at least some content processes even with stylo (256 bytes in one process, 100KB in another, 0 in the remaining two)...

So figure about 8MB of our 10MB regression is accounted for by the non-sharing of UA bits of the stylist across documents.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Items 1-5 suggest that the ua-level stylist takes at least 126KB.

I just realized that I forgot to renumber my items when I added revalidation-selectors.  This would be items 1-5, including both item 5s.  ;)  Item 6 remains item 6 for purposes of determining overall regression.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Cam, I plan to do this tomorrow / over the week. IIRC you tried to hack this too a while ago, so I'd want your feedback.

My plan to handle media query stuff and document rules is:

 * Renaming EffectiveMediaQueryResults into EffectiveMediaQueryAndDocumentResults, or something like that, to (ab)use it for @-moz-document rules too.

 * Make a global cache from:
   * (Vec<StylistSheet>, EffectiveMediaQueryAndDocumentResults)
 * To:
   * Arc<(CascadeData, PrecomputedPseudoElementDeclarations)>.

This means that to do the lookup we need to iterate over the effective rules, collecting into the EffectiveMediaQueryAndDocumentResults.

That's slightly unfortunate, specially in the case we don't get a cache hit, but seems acceptable to me in the sense that it should be fast anyway, and we'd need to do the same either while rebuilding or afterwards to build the cache key and insert into the cache.

This may require splitting StyleSheetSet per origin, which shouldn't be a huge deal anyway, I would think.

How would you feel about something like that? Does it look reasonable? Anything I've missed?

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Cam, I plan to do this tomorrow / over the week. IIRC you tried to hack this
> too a while ago, so I'd want your feedback.

Yes, although now I can't seem to find my experiment patch... :(

> My plan to handle media query stuff and document rules is:
> 
>  * Renaming EffectiveMediaQueryResults into
> EffectiveMediaQueryAndDocumentResults, or something like that, to (ab)use it
> for @-moz-document rules too.

Sounds OK, but I wonder if we need to bother, since AFAIK ABP no longer uses @-moz-document rules, and we kind of want to remove them at some point anyway.  If it's easy enough to fault out of CascadeData sharing if there are @-moz-document rules (and you verify that ABP indeed doesn't use them any more) then I think it should be fine to do that.

>  * Make a global cache from:
>    * (Vec<StylistSheet>, EffectiveMediaQueryAndDocumentResults)
>  * To:
>    * Arc<(CascadeData, PrecomputedPseudoElementDeclarations)>.
> 
> This means that to do the lookup we need to iterate over the effective
> rules, collecting into the EffectiveMediaQueryAndDocumentResults.

I can't see a way around iterating over the style sheets' rules to find the effective @media rules.

> This may require splitting StyleSheetSet per origin, which shouldn't be a
> huge deal anyway, I would think.

That would be a nice change to make anyway, yeah.

> Anything I've missed?

In Gecko, we use an expiration tracker to clear out cached CascadeData that is no longer needed.  Can/should we do something similar here?

What situations do we need to clear out entries from this cache?  For example when a rule in a sheet is modified, we need to remove any entry from the cache that has that sheet in it.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: WIP

I found the WIP patch I had.  Feel free to re-use bits of it if it's useful.

Comment 8

[Bobby Holley (:bholley)]

(Marking this P3 just so that we only have one blocker on file for improving memory usage)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Today I opened (and about-to-land):

  https://github.com/servo/servo/pull/18484
  https://github.com/servo/servo/pull/18486

Still I have to think what's the best place for the cache to live and that sort of stuff.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

A bit more plumbing on the way:

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

I have a clear idea now about how to do it, and also about how to expire unused stuff in there.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP

Need to integrate this into rebuild(), but this is basically what I'm thinking of.

Key observation here is that the stylesheets are already encoded in the stuff that matches media queries.

This doesn't handle document rules, but doing so is trivial.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch

Two things that this doesn't handle, but that Cam said we may not need to handle anymore, due to ABP not using @-moz-document rules.

 * @-moz-document rules: Kinda trivial to treat them as media-dependent.
 * Dynamic changes to UA sheets. It'd be somewhat nasty to handle. Not totally terrible, but...

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch

Err, bugzilla doesn't handle github PR links to comments, and also that was the wrong link. Sorry for the bugspam everybody.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

So, numbers:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=05602575c687dc0a1ed7d6fbc4ea85fbf06f92ea&newProject=try&newRevision=eec9cb4a5088691f18da4a09ec99f6a50720c99b&framework=4&showOnlyImportant=0

This improves AWSY (not a whole lot though, I may go and log when do we share it and when do we not).

This is also green on try[1] (yay!), mod leak logging in release build which I've fixed in the PR.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8486098d912cf748d2c488428d5a2c07610d875&selectedJob=131098412

Comment 15

[Boris Zbarsky [:bzbarsky]]

> This improves AWSY (not a whole lot though, I may go and log when do we share it and when do we not).

Copying from IRC, if I compare Gecko to emilio's baseline push on the three "tabs open" checkpoints I see:

  Gecko style-sets: 8.164610, stylo style-sets: 17.859047, regression: 9.694437
  Gecko style-sets: 8.164610, stylo style-sets: 17.852476, regression: 9.687866
  Gecko style-sets: 8.164610, stylo style-sets: 17.862614, regression: 9.698004

If I compare it to his push with this patch, I see:

  Gecko style-sets: 8.164610, stylo style-sets: 9.537336, regression: 1.372726
  Gecko style-sets: 8.164610, stylo style-sets: 9.531691, regression: 1.367081
  Gecko style-sets: 8.164610, stylo style-sets: 9.540131, regression: 1.375521

which is a reduction of about 8.3MB in the style-sets regression.  That matches our numbers from comment 3 quite well.  ;)

The pre-emilio-patch breakdown of stylo-style-sets is:

    invalidation map: 4.720837
    revalidation selectors: 1.972954
    element-and-pseudos maps: 8.273788
    ruletree: 1.420975
    precomputed-pseudos: 0.217896
    other: 1.175827

and the post-patch breakdown is:

    invalidation map: 3.429352
    revalidation selectors: 0.768005
    element-and-pseudos maps: 3.245811
    ruletree: 1.421852
    precomputed-pseudos: 0.00000
    other: 0.594772

Note that precomputed-pseudos, which is UA-sheet-only, is gone completely, and big reductions in some other items.  Also, ruletree is unaffected, which is good, because it should be unaffected!

Comment 16

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Two things that this doesn't handle, but that Cam said we may not need to
> handle anymore, due to ABP not using @-moz-document rules.
> 
>  * @-moz-document rules: Kinda trivial to treat them as media-dependent.

On IRC we found one use of @-moz-document in UA sheets: bug 1400092.

>  * Dynamic changes to UA sheets. It'd be somewhat nasty to handle. Not
> totally terrible, but...

It is certainly possible to use some say inDOMUtils APIs to get at the sheets that apply to a document, including UA sheets, and then they could be messed with.  aswan tells me that WebExtensions cannot add sheets to the UA level, and shouldn't have the ability to modify UA sheets arbitrarily.  Of course, system addons could.

Brad, do you know whether devtools exposes any interfaces for modifying UA sheets?

Comment 17

[Cameron McCormack (:heycam)]

Also I think it's worth investigating exactly what the ABP WebExtension (and maybe uBlock too?) inserts its style sheets.  Even if for example it adds a non-site-specific sheet along with a site-specific one to the User level, but that could get re-used across different pages of the same site, it might be worth adding support for caching at the User level.  (That might cause the Vec of cached CascadeData to get a bit long, though, with many tabs open on different sites.  So if we do that it might be worth having a different storage mechanism.)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #17)
> Also I think it's worth investigating exactly what the ABP WebExtension (and
> maybe uBlock too?) inserts its style sheets.

Yeah, this doesn't play too well with incremental rebuilds though... It's not hard to do of course.

Comment 19

[Brad Werth [:bradwerth]]

(In reply to Cameron McCormack (:heycam) from comment #16)
> Brad, do you know whether devtools exposes any interfaces for modifying UA
> sheets?

There is one way I know about: the Browser Toolbox. https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox. With that, you can inspect and modify the UA sheets in the browser process. I would be surprised if there are other ways, since this tool is setup specifically to provide this type of editing (and requires special preferences, etc.).

Comment 20

[Bobby Holley (:bholley)]

Emilio, where are we at with this? Joe requested during today's Quantum meeting that we delay any further memory optimizations to 58 to reduce risk and focus on stability / correctness. Does that seem reasonable to you?

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://github.com/servo/servo/pull/18509 was merged 3 days ago, as https://hg.mozilla.org/mozilla-central/rev/1885fa2ee0a8 no?  So I think this bug just needs to be marked fixed...  Will let Emilio confirm.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Yup, bz is right.

Comment 23

[Cameron McCormack (:heycam)]

I filed bug 1401074 for handling modifications to UA sheets.  I don't think it's critical to get to this for 57.