Closed Bug 1348587 Opened 7 years ago Closed 7 years ago

stylo: Consider AtomicRefCell instead of RwLock inside SharedRwLock


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




Tracking Status
firefox55 --- affected


(Reporter: SimonSapin, Unassigned)




(2 files) adds for style objects a SharedRwLock abstraction that uses RwLock<()> internally. Bobby proposed replacing that with AtomicRefCell<()>, which is cheaper and panics instead of blocking in case the lock/cell is already help in a conflicting way. A panic is easier to debug than a deadlock.

For Stylo, it seems that such conflicts are always a bug that would cause a deadlock.

For Servo, at least in the current design, I don’t think that is the case since the TickAnimation message is not synchronized with the script thread and causes cascading which accesses style objects.

So we may want to conditionally compile RwLock<()> for non-Stylo Servo, and AtomicRefCell<()> for Stylo. (And maybe rename SharedRwLock and related things since they don’t necessarily have the semantics of RwLock anymore.)

The new abstraction keeps this change localized to one file.

Implementing this requires either:

* Making SharedRwLock{Read,Write}Guard store both *const AtomicRefCell<()> and AtomicRef{,Mut}, making them three words instead of one. Maybe that’s OK, these guards stay on the stack and don’t usually move (references to them are what’s passed around).

* Extending the AtomicRefCell API to support the equivalent of these parking_lot::RwLock methods: raw_read, raw_write, raw_unlock_read, raw_unlock_write. (Though the first two can be built on top of read/borrow and write/borrow_mut by calling std::mem::forget on the return value.)
I'm going to call this P1 because I'm worried that the deadlocks will bite us.

I'm fine with either of the two options. We might as well try the second I guess, assuming the API is easy enough to add to AtomicRefCell.
Priority: P4 → P1
raw_read/raw_write are the same as read/write but without a return value. (Or, same as calling read/write then leaking the return value.) raw_unlock_read/raw_unlock_write are the same guards’ destructors (but as `unsafe` methods).
Attached patch atomic.patchSplinter Review
> Making SharedRwLock{Read,Write}Guard store both *const AtomicRefCell<()> and AtomicRef{,Mut}, making them three words instead of one.

Ah, turns out we don’t need that raw pointer. Instead we can compare AtomicRef{,Mut}::deref() with AtomicRefCell::as_ptr(). Untested patch attached. (Conditional compilation left as an exercise.)
I'll take over landing this.
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.