Make Stylist no longer an unsafe pointer once we have fast RWArcs #2604

pcwalton · 2014-06-06T18:54:51Z

Currently RWArcs use Sems and that is very slow. Once that is no longer the case, put Stylist in an RWArc.

SimonSapin · 2014-08-12T16:05:11Z

RWArc<T> is gone, replaced by Arc<RWLock<T>>. Does RWLock still have the same perf problems?

pcwalton · 2015-03-02T18:24:44Z

@SimonSapin I believe RWLock is fast now, looking at Rust. We'll need to benchmark to make sure.

mt2d2 · 2015-07-03T08:42:43Z

Took a look at this with help from @Ms2ger on IRC—an implementation moving the Stylist from LayoutTaskData to LayoutTask as an Arc<RwLock<Stylist>> panics with a deadlock. Similar deadlock seen when simply using a Mutex (albeit no panic). No luck, open to suggestions!

Ms2ger · 2015-07-06T18:26:43Z

That was master...mt2d2:stylist_arc_rwlock

pcwalton · 2015-07-06T18:27:36Z

Probably needs finer-grained locking inside the Stylist then?


        Make SharedLayoutContext threadsafe.

Not sure if this is the best way to structure the code; there are a lot of mutexes involved. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.


        Make SharedLayoutContext threadsafe.

Not sure if this is the best way to structure the code; there are a lot of mutexes involved. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.


        Make SharedLayoutContext threadsafe.

Not sure if this is the best way to structure the code; there are a lot of mutexes involved. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.


        Make SharedLayoutContext threadsafe.

Making the Stylist usage threadsafe without unsafe code or extra locking required some changes to WorkQueue; the changes are mostly straightforward. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.


        Make SharedLayoutContext threadsafe.

Making the Stylist usage threadsafe without unsafe code or extra locking required some changes to WorkQueue; the changes are mostly straightforward. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.


        Make SharedLayoutContext threadsafe.

Making the Stylist usage threadsafe without unsafe code or extra locking required some changes to WorkQueue; the changes are mostly straightforward. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.


        Make SharedLayoutContext threadsafe.

Making the Stylist usage threadsafe without unsafe code or extra locking required some changes to WorkQueue; the changes are mostly straightforward. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.


        Make SharedLayoutContext threadsafe.

Making the Stylist usage threadsafe without unsafe code or extra locking required some changes to WorkQueue; the changes are mostly straightforward. Some refactoring of LayoutTaskData was also required: this is basically just splitting the structure in two so the borrow checker can understand that building a SharedLayoutContext only borrows part of LayoutTaskData. The "frozen" part is the part that can't be safely modified while a parallel work-queue is running. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.

Yoric · 2015-09-29T15:07:01Z

I can try to work on this. How can I test this codepath?

Edit: Ah, I see that @eefriedman is on it. Nevermind.

jdm · 2015-09-29T15:24:38Z

I believe #7524 is taking care of this.


        Make SharedLayoutContext threadsafe.

Making the Stylist usage threadsafe without unsafe code or extra locking required some changes to WorkQueue; the changes are mostly straightforward. Some refactoring of LayoutTaskData was also required: this is basically just splitting the structure in two so the borrow checker can understand that building a SharedLayoutContext only borrows part of LayoutTaskData. The "frozen" part is the part that can't be safely modified while a parallel work-queue is running. It might be worth considering switching to some sort of threadsafe channel to avoid the mutexes... see Rust RFC PR servo#1299. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.


        Make SharedLayoutContext threadsafe.

Making the Stylist usage threadsafe without unsafe code or extra locking required some changes to WorkQueue; the changes are mostly straightforward. Some refactoring of LayoutTaskData was also required: this is basically just splitting the structure in two so the borrow checker can understand that building a SharedLayoutContext only borrows part of LayoutTaskData. The "frozen" part is the part that can't be safely modified while a parallel work-queue is running. It might be worth considering switching to some sort of threadsafe channel to avoid the mutexes... see Rust RFC PR servo#1299. Fixes servo#2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.

Ms2ger · 2015-11-07T11:56:14Z

I tried to just wrap it into Arc/Mutex on top of #8378; to my surprise it managed to load github: servo:master...Ms2ger:arc-stylist.

eefriedman · 2015-11-07T18:59:03Z

Sure, you can wrap it in a Mutex... but what does that do to the performance? (That's what sent me down the path of borrowing it.)

Ms2ger · 2015-11-08T12:57:20Z

Alternatively, we could try to move the Stylist out of the LayoutTask entirely; then we could borrow it while there's a mutable borrow of the LayoutTask.

SimonSapin · 2015-11-08T13:45:50Z

Move it tot the document node, similar to #8039 ?

Ms2ger · 2015-11-08T14:44:03Z

I don't follow.

SimonSapin · 2015-11-08T20:38:57Z

You mention moving Stylist out of LayoutTask. To where? One option could be to have it owned by the document node. There is sort of a precedent in that PR, which moves ownership of stylesheets from Stylist to respective DOM nodes.

eefriedman · 2015-11-08T20:52:09Z

If we intentionally want to dynamically check the borrowing of Stylist, I think we can just stuff it into an Arc, and use get_mut().unwrap() when we want to mutate it. Granted, it's stylistically terrible, but it's probably better than a raw pointer.

I don't see how changing the ownership of the Stylist helps.

SimonSapin · 2015-11-08T20:57:03Z

I don’t think we need dynamic checks at all if Stylist is in the DOM. Like the rest of DOM data, access is exclusive from the script task, and shared but read-only in the "early" phase (until flow tree construction) of layout.

eefriedman · 2015-11-08T21:10:06Z

Oh, right... if Stylist is in the DOM, we don't need to mutate it from layout. Makes sense.

jdm · 2016-03-10T19:36:20Z

Latest attempt at #7524.

emilio · 2016-04-23T00:21:52Z

#10815 Should take care of this.

It uses an Arc plus Arc::get_mut, and seems to work just fine. It should not have almost any performance overhead.

SimonSapin · 2016-04-23T09:06:34Z

@emilio Arc::get_mut clones the contents if the refcount is above one. Is this really desired here? This issue originally mentions RWLock.

emilio · 2016-04-23T09:55:13Z

@SimonSapin: Stylist is not cloneable, Arc::get_mut returns None if the refcount is not one (unlike Arc::make_mut).

I'm using Arc::get_mut(&mut rw_data.stylist).unwrap() and I haven't ever got a crash.

SimonSapin · 2016-04-23T10:00:53Z

Oh yeah, sorry, I got confused with make_mut. But if the refcount really is always one, could this be a Box?

emilio · 2016-04-23T10:19:11Z

I think not since it has to be handed out to layout traversal (I mean to SharedLayoutContext).

The refcount is always one when we update the stylist in the layout thread, but not always.

emilio · 2016-05-02T03:24:47Z

This is closed by #10815

pcwalton added I-refactor labels Jun 6, 2014

kmcallister added the E-easy label May 12, 2015

jdm added E-less easy and removed E-easy labels Jul 3, 2015

eefriedman mentioned this issue Sep 3, 2015

Make SharedLayoutContext threadsafe. #7524

Closed

KiChjang added the C-assigned label Mar 2, 2016

KiChjang assigned eefriedman Mar 2, 2016

jdm removed C-assigned E-less easy labels Mar 10, 2016

emilio closed this May 2, 2016

servo / servo

Make Stylist no longer an unsafe pointer once we have fast RWArcs #2604

Make Stylist no longer an unsafe pointer once we have fast RWArcs #2604

pcwalton commented Jun 6, 2014

SimonSapin commented Aug 12, 2014

pcwalton commented Mar 2, 2015

mt2d2 commented Jul 3, 2015

Ms2ger commented Jul 6, 2015

pcwalton commented Jul 6, 2015

Yoric commented Sep 29, 2015

jdm commented Sep 29, 2015

Ms2ger commented Nov 7, 2015

eefriedman commented Nov 7, 2015

Ms2ger commented Nov 8, 2015

SimonSapin commented Nov 8, 2015

Ms2ger commented Nov 8, 2015

SimonSapin commented Nov 8, 2015

eefriedman commented Nov 8, 2015

SimonSapin commented Nov 8, 2015

eefriedman commented Nov 8, 2015

jdm commented Mar 10, 2016

emilio commented Apr 23, 2016

SimonSapin commented Apr 23, 2016

emilio commented Apr 23, 2016

SimonSapin commented Apr 23, 2016

emilio commented Apr 23, 2016

emilio commented May 2, 2016

servo / servo

Join GitHub today

Make Stylist no longer an unsafe pointer once we have fast RWArcs #2604

Make Stylist no longer an unsafe pointer once we have fast RWArcs #2604

Comments

pcwalton commented Jun 6, 2014

SimonSapin commented Aug 12, 2014

pcwalton commented Mar 2, 2015

mt2d2 commented Jul 3, 2015

Ms2ger commented Jul 6, 2015

pcwalton commented Jul 6, 2015

Yoric commented Sep 29, 2015

jdm commented Sep 29, 2015

Ms2ger commented Nov 7, 2015

eefriedman commented Nov 7, 2015

Ms2ger commented Nov 8, 2015

SimonSapin commented Nov 8, 2015

Ms2ger commented Nov 8, 2015

SimonSapin commented Nov 8, 2015

eefriedman commented Nov 8, 2015

SimonSapin commented Nov 8, 2015

eefriedman commented Nov 8, 2015

jdm commented Mar 10, 2016

emilio commented Apr 23, 2016

SimonSapin commented Apr 23, 2016

emilio commented Apr 23, 2016

SimonSapin commented Apr 23, 2016

emilio commented Apr 23, 2016

emilio commented May 2, 2016