1360883 - stylo: avoid unnecessary Arc traffic during the cascade

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

Our current implementation of the copy-on-write style structs is suboptimal. We should be able to eliminate all the Arc::make_mut (i.e. CAS) calls, as well as some refcount bumps (i.e. RMU) by filling in our style structs lazily rather than eagerly.

Right now we start out by doing a bunch of refcount bumps (once for each style struct, cloning the arc from either the parent or the initial values). Then, when we want to modify them, we do Arc::make_mut(). The first one of these calls for a given style struct always triggers a clone, and subsequent ones always just return the owned struct (since nobody else is using it).

Instead, we should create a ComputedValuesBuilder struct that stores a borrow of the parent and default ComputedValues, and a bunch of Option<>s for the style struct pointers. When we try to set a property, we check whether we have Some or None. If we have None, we clone the parent and cache it as Some. If we have Some, we know we have exclusive access to it, so we just mutate it.

The annoying bit is that the Arc API doesn't give us a way to do this "I know I have exclusive access, just give me a mutable pointer" part. We could do some unsafe pointer tricks and rely on the layout of ArcInner. Or we could just fork our own version of Arc, which has its advantages anyway [1].

[1] It would allow us to get rid of the weak count, which we don't use, and it would be more amenable to arena-allocating style structs if we go that route.

Comment 1

[Simon Sapin (:SimonSapin)]

> the Arc API doesn't give us a way to do this "I know I have exclusive access, just give me a mutable pointer" part.

It does: Arc::get_mut(&mut arc).expect("expected should have exclusive access")


> get rid of the weak count,

https://github.com/rust-lang/rust/pull/41005 changes the counts from usize to u32, saving a word per Arc on 64-bit platforms. With this, still on 64-bit, removing the weak count would not change the size of ArcInner<T> if T is word-aligned.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #1)
> > the Arc API doesn't give us a way to do this "I know I have exclusive access, just give me a mutable pointer" part.
> 
> It does: Arc::get_mut(&mut arc).expect("expected should have exclusive
> access")

That does the CAS though, and the whole point here is to avoid that overhead.

> 
> 
> > get rid of the weak count,
> 
> https://github.com/rust-lang/rust/pull/41005 changes the counts from usize
> to u32, saving a word per Arc on 64-bit platforms. With this, still on
> 64-bit, removing the weak count would not change the size of ArcInner<T> if
> T is word-aligned.

That's good to hear - though that doesn't help on 32-bit, and Arc actually does quite a bit of extra atomic RMU work to handle weak references. I think that's worth eliminating.

I've got a partial patch for this, will file a dependent bug.

Comment 3

[Simon Sapin (:SimonSapin)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> (In reply to Simon Sapin (:SimonSapin) from comment #1)
> > Arc::get_mut(&mut arc).expect("expected should have exclusive access")
> 
> That does the CAS though, and the whole point here is to avoid that overhead.

Rust 1.17 also added:

Arc::into_raw(this: Arc<T>) -> *const T 
Arc::from_raw(ptr: *const T) -> Arc<T>

(The latter unsafe, of course. Only valid with a pointer from Arc::into_raw.)

If we’re very confident that the Arc passed to into_raw was the only reference (e.g. it was just created with Arc::new) then I think *const T can be cast to *mut T which can be defered to &mut T, effectively building unchecked_get_mut. (The downside is that into_raw consumes an Arc instead of taking &mut Arc<T>, so it’s less flexible. It’s designed for FFI.)

Comment 4

[Bobby Holley (:bholley)]

Emilio basically did a modified version of comment 0 in [1], which fixed the Arc traffic, but increased memmovs. I'm going to fix this with custom Arcs in bug 1360889.

[1] https://github.com/servo/servo/pull/16663