Closed Bug 1305141 Opened 8 years ago Closed 8 years ago

stylo: Use parking_lot::RwLock for PrivateStyleData

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Unassigned)

References

Details

Style data currently hangs off of Nodes in a boxed RefCell. One currently-unenforced invariant of the servo style system is that we only access node data either (a) when we have exclusive access to the node (processing self), or (b) immutably (processing children). The RefCell borrow flags aren't threadsafe, which means we have to use an unsafe borrow here:

https://github.com/servo/servo/blob/e6bbff110a017019ac31cf0700b1e722d8a1c61b/components/style/matching.rs#L868

A better solution would be to use a RefCell with thread-safe borrow flags. This is basically an RwLock with try_read().expect() and try_write.expect() replacing borrow() and borrow_mut().

RwLocks have been kinda frowned upon in Servo because they were slow (using OS-level primitives under the hood). But eddyb just pointed me to parking_lot [1], which is a pure-rust reimplementation of std::sync. In particular, I've verified that try_write and try_read both boil down to an atomic CAS and relax load + atomic CAS, respectively.

I think we should use that, and get rid of the unsafety here. 

[1] https://github.com/Amanieu/parking_lot/
[1] https://github.com/Amanieu/parking_lot/blob/137d45335a1e7bff916d7d27c918790a592686a5/src/raw_rwlock.rs#L54
[2] https://github.com/Amanieu/parking_lot/blob/137d45335a1e7bff916d7d27c918790a592686a5/src/raw_rwlock.rs#L104
Do we really want to use try_read().expect() and try_write().expect() (which can panic) rather than read() and write() (which can block)? Blocking would solve the case of animations which are currently not synchronized with script. (The blocking methods also use the same kind of atomic access in the uncontended case.
(In reply to Simon Sapin (:SimonSapin) from comment #1)
> Do we really want to use try_read().expect() and try_write().expect() (which
> can panic) rather than read() and write() (which can block)? Blocking would
> solve the case of animations which are currently not synchronized with
> script.

How often are we expecting contention? If we're likely to have a lot of contention that would seem to suggest that we'd be doing it wrong. But I don't have a great sense of what these animation synchronization issues are.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> How often are we expecting contention? If we're likely to have a lot of
> contention that would seem to suggest that we'd be doing it wrong. But I
> don't have a great sense of what these animation synchronization issues are.

That should only happen when CSSOM is accessing a keyframe rule for writing, and the layout thread is reading from it already. In general the locking should be always zero, except on that case, so I'd block in that case, then use try_{read, write}().expect() in all the other cases that are not expected to block to ensure we're not doing something wrong.
Oh ok - as long as we only use potentially-blocking operations at keyframe-specific callsites, that sounds fine.

In my Arena PR I made something call AtomicRefCell which newtypes RwLock and provides a nicer interface. Using that or something like it, which names like read_for_keyframes, or somesuch, would ensure we don't accidentally introduce blocking in more places than we should.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> I'd block in that case, then
> use try_{read, write}().expect() in all the other cases that are not
> expected to block to ensure we're not doing something wrong.

The same code (e.g. the cascade() function) doing that access is used in both cases (animation and restyle), but knowing which case a given call is is much higher in the call stack.

In the PR I’m currently working on I always use blocking .read() and .write().
(In reply to Simon Sapin (:SimonSapin) from comment #5)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> > I'd block in that case, then
> > use try_{read, write}().expect() in all the other cases that are not
> > expected to block to ensure we're not doing something wrong.
> 
> The same code (e.g. the cascade() function) doing that access is used in
> both cases (animation and restyle), but knowing which case a given call is
> is much higher in the call stack.
> 
> In the PR I’m currently working on I always use blocking .read() and
> .write().

Yeah, though the caller knows it directly (at least in the keyframes case it's not much higher on the call stack).

I was thinking on passing that information down. In the rule tree version of the PR I use an iterator for the keyframe rules and a different one for the general case, so in that case we control how the declaration is accessed. Maybe that change can be ported into your PR, so we can differentiate that? There are other ways to do it too, like just passing an enum that represents that information to cascade().
I misread the original message in this bug and assumed it was about PropertyDeclarationBlock rather than PrivateStyleData. I’ve been working on https://github.com/servo/servo/pull/13459. But presumably the analysis of atomics in the uncontended case being fast enough still holds?

(Contention only happening when CSSOM is used is about PropertyDeclarationBlock though, I’m not sure about PrivateStyleData.)

Corresponding PR: https://github.com/servo/servo/pull/13459
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> Corresponding PR: https://github.com/servo/servo/pull/13520

This is now fixed.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.