The default bug view has changed. See this FAQ.

stylo: Consider AtomicRefCell instead of RwLock inside SharedRwLock

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
4 days ago
2 days ago

People

(Reporter: SimonSapin, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox55 affected)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 days ago
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.)
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
(Reporter)

Comment 2

4 days ago
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).
(Reporter)

Comment 3

4 days ago
Created attachment 8848846 [details] [diff] [review]
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.)
I'll take over landing this.
Created attachment 8849336 [details] [diff] [review]
Use AtomicRefCell instead of RwLock inside SharedRwLock for stylo. v1

MozReview-Commit-ID: 8M6BUqe4pil
Attachment #8849336 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88f0c5d7d7da1fde9f3848f2faaeea9055236ce
https://github.com/servo/servo/pull/16053
Status: NEW → RESOLVED
Last Resolved: 2 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.