This is basically the gist of a discussion that I and xidorn were
having, pertaining to CSSOM in both Servo and Stylo. We mostly wrapped
it up, but there are loose ends and I'm not sure if the choices made
are correct.

In CSSOM, you basically have a StyleSheetList holding many StyleSheets
holding a CSSRuleList holding many CSSRules. You can
index/insert/delete on the lists and CSSRule can be manipulated in a
bunch of ways.

One invariant that's not *exactly* specified by the spec (but is sort
of hinted at, and implemented by browsers) is that these lists should
yield the same underlying JS object when you access the same
conceptual rule/stylesheet in a list (even if the rest of the list was
mutated). So the DOM indexing methods cannot just create
rule/stylesheet JS objects on the fly, they need to cache them somehow
and update them when the list is updated.

Servo currently has a style tree (stylesheets->rulelist->rules->other
rules and rule lists) in style::Stylesheet[1], and this is shared with
the DOM via Arcs. As I understand it, Gecko, on the other hand, just
uses the CSSOM DOM tree and has no "style tree" otherwise. Reading
rules in Gecko requires traversing through CSSOM objects. This is a
straightforward solution to the same-object restriction; if your style
tree is made up of these objects in the first place there's nothing to
worry about.

Servo can't use this solution. Trying to thread style through the DOM
will mean merging the style and script crates, and it will be quite
complicated to make Servo's style system generic over CSSOM
implementations for stylo to work.

A possible solution is to have a opaquish DOM backpointer to
corresponding rule objects in the style tree and have it call some
update function to update the DOM when mutating. This isn't a great
solution IMO.

The one that we seem to prefer is maintaining the style tree and the
CSSOM tree in parallel, with references from the CSSOM tree to the
style tree, but not vice versa. Only the DOM mutates the style tree.
This could possible be enforced by wrapping all interior mutability in
the style tree with a type that requires a zero-sized token to be
passed in to the borrow_mut/write method; a token which can't be
created normally but can be obtained via a method on dom::Reflectable
(within arms reach for most DOM code). Stylo code can FFI call into
functions using a similar mechanism.

So far this seems good (though very open to other ideas or suggestions
for improvement).

However, in this model the DOM still needs to hold Arcs to the
corresponding style system objects. In some cases, this is
straightforward. dom::StyleSheet holds an arc to a style::Stylesheet.
dom::CSSRule contains a style::CSSRule (which is a cheaply cloneable
enum of Arcs).

In the case of CSSRuleList, we have a bit of an issue. There's no
corresponding arc'd style:: type for CSSRuleList. There's just
`Vec<style::CSSRule>`, which is used in Stylesheet[1], MediaRule[2],
and @supports rules (not supported in servo yet).

Now, we could just put the Vec in an Arc. I'm not sure if we want to
with the extra indirection, but it's a simple solution. An alternative
is to recognize that the `Vec<CSSRule>` is always going to be stored
within larger Arc'd objects, and just use those, by storing an

enum RuleListOwner {
    Sheet(Arc<Stylesheet>),
    Media(Arc<MediaRule>),
    // ..
}

This is great for the Rust code -- easy to abstract over and no extra perf cost.

However, this can't be easily passed over to C++ for stylo. There's no
way to write a corresponding C++ type that takes up the same
space/alignment as a Rust enum. We could hardcode the size and have
sizeof assertions in the tests, but that may not solve alignment
issues, especially with the unspecified Rust ABI. We could also store
an explicit tag/pointer pair which we have unsafe wrappers for on the
Rust side. Ick.

Personally I think just sticking the vec into an Arc is fine and won't
impact us much. But I don't know.

Overall, I think we need to map out the ownership graph of CSSOM in
both Servo and Stylo before starting work on it.


[1]: https://doc.servo.org/style/stylesheets/struct.Stylesheet.html
[2]: https://doc.servo.org/style/stylesheets/struct.MediaRule.html

Thanks,
-Manish Goregaokar

On Fri, Nov 11, 2016 at 03:22:16AM -0800, Manish Goregaokar wrote:
> A possible solution is to have a opaquish DOM backpointer to
> corresponding rule objects in the style tree and have it call some
> update function to update the DOM when mutating. This isn't a great
> solution IMO.

Note that this is already what is done for a Node's computed style (and
the rest of the style and layout data) in Servo, and it's already
causing some problems, like unnecessary extra reflows and overhead to
check a node's computed value for a given property using
GetComputedStyle.

As a side note, given we're thinking in how to structure these crates,
it would be nice to find a solution for this too, since the extra
reflows aren't cheap, and a layout thread query in general seems a lot
of overhead for something that should be really cheap.

That's probably a slightly harder problem though, because it involves
knowing layout data structures (as in: Servo's layout crate). I've
thought about enforcing the first member of the heap-allocated node data
to be a ComputedStyle, but I sincerely would prefer to avoid as many
transmutes as possible, one of them is too many...

> The one that we seem to prefer is maintaining the style tree and the
> CSSOM tree in parallel, with references from the CSSOM tree to the
> style tree, but not vice versa. Only the DOM mutates the style tree.

This seems like the most reasonable approach to me too. Merging style
and DOM will not only be probably a blocker for Stylo, but also make the
style system much more unsafe (having lots more of JS reflectors in
random threads).

> This could possible be enforced by wrapping all interior mutability in
> the style tree with a type that requires a zero-sized token to be
> passed in to the borrow_mut/write method; a token which can't be
> created normally but can be obtained via a method on dom::Reflectable
> (within arms reach for most DOM code). Stylo code can FFI call into
> functions using a similar mechanism.

Or probably asserting that we're not in a Servo Layout thread when we
`borrow_mut`? Not perfect, but...

Also, note that the layout thread right now is the one that constructs
and manages the Stylist, and that stylesheet web fonts are processed
right now async with script (see the AddStylesheet layout message), and
it reads the font face rules, so potentially you want a stronger
synchronization there.

> Now, we could just put the Vec in an Arc. I'm not sure if we want to
> with the extra indirection, but it's a simple solution. An alternative
> is to recognize that the `Vec<CSSRule>` is always going to be stored
> within larger Arc'd objects, and just use those, by storing an
>
> enum RuleListOwner {
>     Sheet(Arc<Stylesheet>),
>     Media(Arc<MediaRule>),
>     // ..
> }
>
> This is great for the Rust code -- easy to abstract over and no extra perf cost.

Can you move the same CSSRuleList to a different owner? If you can, this
wouldn't work, right?

> Personally I think just sticking the vec into an Arc is fine and won't
> impact us much. But I don't know.

Agreed, the extra Arc, over all given where it's used doesn't seem such
a show-stopper to me. We have Arc<RwLocks<>> all over the place, and I
don't think this two places are hotter than the ones on top of, let's
say, property declaration blocks.

I assume this will also need it's own RwLock in order to mutate the
list, right? If that's the case, probably something like
Arc<RwLock<CSSRuleList>>, where CSSRuleList(Vec<CSSRule>) is probably
nicer.

 -- Emilio

> Or probably asserting that we're not in a Servo Layout thread when we
> `borrow_mut`? Not perfect, but...

Extra runtime check and I'm not fond of this solution in general,
since it's runtime. The token thing is a pure compile time option.
Making it !Send+!Sync means that it's hard to make it accidentally end
up in the layout thread.

> Can you move the same CSSRuleList to a different owner? If you can, this
> wouldn't work, right?

I don't think you can.

e this will also need it's own RwLock in order to mutate the
> list, right? If that's the case, probably something like
> Arc<RwLock<CSSRuleList>>, where CSSRuleList(Vec<CSSRule>) is probably
> nicer.

Yep. I'm implementing an immutable cssom first, and will revisit the
exact positions of the rwlocks later.

On 11/11/16 12:22, Manish Goregaokar wrote:
> Servo currently has a style tree (stylesheets->rulelist->rules->other
> rules and rule lists) in style::Stylesheet[1], and this is shared with
> the DOM via Arcs. As I understand it, Gecko, on the other hand, just
> uses the CSSOM DOM tree and has no "style tree" otherwise. Reading
> rules in Gecko requires traversing through CSSOM objects. This is a
> straightforward solution to the same-object restriction; if your style
> tree is made up of these objects in the first place there's nothing to
> worry about.
>
> Servo can't use this solution. Trying to thread style through the DOM
> will mean merging the style and script crates, and it will be quite
> complicated to make Servo's style system generic over CSSOM
> implementations for stylo to work.

I’ve proposed this approach before, and I don’t think it would be that
complicated. The only parts of components/style that access this tree
(other than the parsing part creating it) is Stylist::update and
Stylist::set_device taking &[Arc<Stylesheet>] and traversing the tree.

I’m sure we can reasonably define a trait to abstract this traversal.
The "leaves" of this tree (selector list, declaration block, …) would
stay in style crate, but the tree structure and the ownership/mutability
story doesn’t need to be handled there.

Bobby argued that we should keep that tree in the style crate because it
can implement some operations that won’t have to be re-implemented in
both Servo and Gecko/Stylo.

But I don’t know if there is much to share anyway, and the alternative
(maintaining parallel trees) also has its complexity.

> A possible solution is to have a opaquish DOM backpointer to
> corresponding rule objects in the style tree and have it call some
> update function to update the DOM when mutating. This isn't a great
> solution IMO.
>

> The one that we seem to prefer is maintaining the style tree and the
> CSSOM tree in parallel, with references from the CSSOM tree to the
> style tree, but not vice versa. Only the DOM mutates the style tree.

Yes, in my mind that was always the plan, as discussed in
https://bugzilla.mozilla.org/show_bug.cgi?id=1281962. Have a tree in
style without parent pointers or pointers back to DOM, and a parallel
CSSOM tree where each JS object holds an Arc to the corresponding style
object (and a parent pointer as needed), with DOM operations taking care
of keeping them in sync.

> This could possible be enforced by wrapping all interior mutability in
> the style tree with a type that requires a zero-sized token to be
> passed in to the borrow_mut/write method; a token which can't be
> created normally but can be obtained via a method on dom::Reflectable
> (within arms reach for most DOM code). Stylo code can FFI call into
> functions using a similar mechanism.

I don’t think enforcing this is important. As mentioned above the part
of the style crate accessing this tree is small, so I don’t think it
likely that it will accidentally mutate it. And I, as far as I know, the
worst that can happen if they do get out of sync is incorrect styling or
results from CSSOM, but nothing like a memory safety issue.

> In the case of CSSRuleList, we have a bit of an issue. There's no
> corresponding arc'd style:: type for CSSRuleList. There's just
> `Vec<style::CSSRule>`, which is used in Stylesheet[1], MediaRule[2],
> and @supports rules (not supported in servo yet).

There isn’t yet (in master) but as you did in #14190 the idea is to
introduce Arc’s (and RwLock’s) as needed. (That is, unless we get rid of
this style tree.)

--
Simon Sapin

On Tue, Nov 15, 2016, at 04:03 AM, Simon Sapin wrote:
> On 11/11/16 12:22, Manish Goregaokar wrote:
> > Servo currently has a style tree (stylesheets->rulelist->rules->other
> > rules and rule lists) in style::Stylesheet[1], and this is shared with
> > the DOM via Arcs. As I understand it, Gecko, on the other hand, just
> > uses the CSSOM DOM tree and has no "style tree" otherwise. Reading
> > rules in Gecko requires traversing through CSSOM objects. This is a
> > straightforward solution to the same-object restriction; if your style
> > tree is made up of these objects in the first place there's nothing to
> > worry about.
> >
> > Servo can't use this solution. Trying to thread style through the DOM
> > will mean merging the style and script crates, and it will be quite
> > complicated to make Servo's style system generic over CSSOM
> > implementations for stylo to work.
>
> I’ve proposed this approach before, and I don’t think it would be that
> complicated. The only parts of components/style that access this tree
> (other than the parsing part creating it) is Stylist::update and
> Stylist::set_device taking &[Arc<Stylesheet>] and traversing the tree.
>
> I’m sure we can reasonably define a trait to abstract this traversal.
> The "leaves" of this tree (selector list, declaration block, …) would
> stay in style crate, but the tree structure and the ownership/mutability
> story doesn’t need to be handled there.

I suppose that means Gecko would maintain the tree directly as well for
stylo, rather than creating a parallel DOM tree on demand like what I'm
currently doing.

I guess it is probably doable, but that means parsing and traversing the
tree would involve lots of additional FFI calls to build and access the
tree in Gecko side. Not sure whether that is something we want.

> Bobby argued that we should keep that tree in the style crate because it
> can implement some operations that won’t have to be re-implemented in
> both Servo and Gecko/Stylo.
>
> But I don’t know if there is much to share anyway, and the alternative
> (maintaining parallel trees) also has its complexity.

There are some, like serialization. It doesn't sound like a good idea to
have a serialization algorithm involves both Gecko code and Servo code.

- Xidorn

On Mon, Nov 14, 2016 at 5:11 PM, Xidorn Quan <m...@upsuper.org> wrote:
> I guess it is probably doable, but that means parsing and traversing the
> tree would involve lots of additional FFI calls to build and access the
> tree in Gecko side.

It also can't be done lazily if it's a single tree. My current
Servo-side implementation maintains a parallel tree and is lazy, which
works well.

> But I don’t know if there is much to share anyway

Parsing and serialization basically. A lot of the serialization code
hasn't been implemented yet but we can do that easily. Parsing may
need to be wrapped in a cleaner API (haven't had a chance to properly
look through that yet, and most of the parsing is just parsing of
PropertyDeclarationBlocks).


-Manish Goregaokar