1348587 - stylo: Consider AtomicRefCell instead of RwLock inside SharedRwLock

Status: RESOLVED FIXED

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

Description

[Simon Sapin (:SimonSapin)]

https://github.com/servo/servo/pull/16014 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.)

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Simon Sapin (:SimonSapin)]

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).

Comment 3

[Simon Sapin (:SimonSapin)]

Attachment: atomic.patch

> 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.)

Comment 4

[Bobby Holley (:bholley)]

I'll take over landing this.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Use AtomicRefCell instead of RwLock inside SharedRwLock for stylo. v1

MozReview-Commit-ID: 8M6BUqe4pil

Comment 6

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88f0c5d7d7da1fde9f3848f2faaeea9055236ce

Comment 7

[Bobby Holley (:bholley)]

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