1400078 - stylo: measure the user-agent cascade data cache.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/18509 (bug 1362532) is adding a global cache to store UA stylesheet data.

As such, we can't measure it from the stylist, so I'm removing that code and adding a FIXME where it used to be.

Instead, we'd need to measure the cascade data once per processes.

The cache instance is called UA_CASCADE_DATA_CACHE:

   https://github.com/emilio/servo/blob/897681ffe52b675039c7e76bc79dbe7db7e4dc0d/components/style/stylist.rs#L67

It is guarded by a Mutex, though unlike in Servo, it's uncontended in Gecko (it's only accessed from the main thread).

The type definition is a few lines below, but TL;DR, it's a Vec of UserAgentCascadeDatas that it "owns", and other stylists may reference.

All the infra for measuring UserAgentCascadeDatas should be there, since we measured CascadeData and PrecomputedPseudoElementDecls.

So in practice I don't think this will be too terrible, but let me know if I can help out somehow.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Measure the UA cache

ServoStyleSetSizes now has two uses, one for the Stylist, and one for the UA
cache, and so the patch removes 'Stylist' from the field names.

Example output from about:memory:

> +----1,359,608 B (00.55%) -- layout
> |    +----756,488 B (00.31%) -- style-sheet-cache [2]
> |    +----393,968 B (00.16%) -- servo-ua-cache
> |    |    +--234,496 B (00.10%) -- element-and-pseudos-maps
> |    |    +---59,648 B (00.02%) -- revalidation-selectors
> |    |    +---58,320 B (00.02%) -- invalidation-map
> |    |    +---30,752 B (00.01%) -- other
> |    |    +---10,752 B (00.00%) -- precomputed-pseudos

MozReview-Commit-ID: 8oxuJO0ojp

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8909684 [details] [diff] [review]
Measure the UA cache

Review of attachment 8909684 [details] [diff] [review]:
-----------------------------------------------------------------

The servo bits look ok to me, however I'm on the onboarding week and I'm not going to be able to dig into the Gecko bits today, so redirecting to cam to review those, sorry :/.

If Cam is super busy or something I can try to take a look after this week is over, or maybe when I find some free time tomorrow.

::: servo/components/style/stylist.rs
@@ +139,5 @@
> +    #[cfg(feature = "gecko")]
> +    pub fn add_size_of(&self, ops: &mut MallocSizeOfOps, sizes: &mut ServoStyleSetSizes) {
> +        sizes.mOther += self.entries.shallow_size_of(ops);
> +        for arc in self.entries.iter() {
> +            // njn: are these primary references? i.e. ok to measure unconditionally?

Yes

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8909684 [details] [diff] [review]
Measure the UA cache

Review of attachment 8909684 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I was able to take a closer look on the bus. This looks good to me, thanks!

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1400078 - Measure the UA cache. .

ServoStyleSetSizes now has two uses, one for the Stylist, and one for the UA
cache, and so the patch removes 'Stylist' from the field names.

Example output from about:memory:

> +----1,359,608 B (00.55%) -- layout
> |    +----756,488 B (00.31%) -- style-sheet-cache [2]
> |    +----393,968 B (00.16%) -- servo-ua-cache
> |    |    +--234,496 B (00.10%) -- element-and-pseudos-maps
> |    |    +---59,648 B (00.02%) -- revalidation-selectors
> |    |    +---58,320 B (00.02%) -- invalidation-map
> |    |    +---30,752 B (00.01%) -- other
> |    |    +---10,752 B (00.00%) -- precomputed-pseudos

Review commit: https://reviewboard.mozilla.org/r/181540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181540/

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8910063 [details]
Bug 1400078 - Measure the UA cache. .

https://reviewboard.mozilla.org/r/181540/#review186814

Carrying over r+ from emilio.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b9dcbd0701a
Measure the UA cache. r=njn.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2b9dcbd0701a