1368399 - stylo: We have no style sharing for inline styles

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

In Gecko, these are explicitly coalesced on the DOM level just so we'll share the style side see bug 760331 for why we did that.  In particular, note bug 760331 comment 11, which notes that the change dropped made style context memory usage from 200+MB to 11KB (not a typo!), on a large hg.mozilla.org changeset page.

Anyway, with stylo we share the _declaration_, but inline styles immediately disable ComputedValues sharing.

I don't see why we need to do that.  As long as two elements have the same exact declaration pointer for their inline styles, why can't we do style sharing for them?

(If this is sounding a lot like bug 1368229, that's because it is.)

Comment 1

[Boris Zbarsky [:bzbarsky]]

> change dropped made style context memory usage

Just "change dropped style context memory usage", of course.

Comment 2

[Bobby Holley (:bholley)]

Agreed, we should do this once bug 1368229 lands.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Reduce the number of places where we need to enumerate ValidationData members. v1

This is what I was talking about in that other bug. :-)

MozReview-Commit-ID: 9m2ebknBFSE

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Compare style attributes rather than rejecting them from the cache. v1

MozReview-Commit-ID: Jmu7Pie2mBP

Comment 5

[Boris Zbarsky [:bzbarsky]]

AFFECTED_BY_PRESENTATIONAL_HINTS is dead too and can also be removed, I think.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Longer terms we should try to think of a way to avoid the FFI call here to get the style attribute, btw, but it seems a bit complicated.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8872841 [details] [diff] [review]
Part 1 - Reduce the number of places where we need to enumerate ValidationData members. v1

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

Not a fan of Default, but fine :)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8872842 [details] [diff] [review]
Part 2 - Compare style attributes rather than rejecting them from the cache. v1

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

r=me, thanks for doing this :)

::: servo/components/style/sharing/checks.rs
@@ +52,5 @@
> +{
> +    match (target.style_attribute(), candidate.style_attribute()) {
> +        (None, None) => true,
> +        (Some(_), None) => false,
> +        (None, Some(_)) => false,

nit: You can merge the branches here if you want.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #5)
> AFFECTED_BY_PRESENTATIONAL_HINTS is dead too and can also be removed, I
> think.

It can't, because it's still used to communicate between stylist and sharing/mod.rs. See https://hg.mozilla.org/mozilla-central/file/7b8937970f9c/servo/components/style/sharing/mod.rs#l372

However, as soon as your id stuff lands, this information will no longer need to round-trip through rust-selectors, which will be awesome.

Comment 10

[Bobby Holley (:bholley)]

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

Green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb00cc7efb0defe23bd1ed55f1e89488ae7a042f