Make SharedLayoutContext threadsafe. #7524

eefriedman · 2015-09-03T02:28:32Z

Not sure if this is the best way to structure the code; there are a lot
of mutexes involved.

Fixes #2604. Pretty sure the issue with the other version mentioned
there was just that it held on to the lock too long.

highfive · 2015-09-03T02:28:37Z

Warning

These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

Ms2ger · 2015-09-03T09:57:47Z

r? @pcwalton

pcwalton · 2015-09-03T16:43:56Z

components/layout/traversal.rs

@@ -201,7 +201,7 @@ impl<'a> PreorderDomTraversal for RecalcStyleForNode<'a> {
                                          parent_opt,
                                          &applicable_declarations,
                                          &mut self.layout_context.applicable_declarations_cache(),
-                                          &self.layout_context.shared.new_animations_sender);
+                                          &self.layout_context.shared.new_animations_sender.lock().unwrap());


Please pass the mutex along here. It'll be too slow otherwise.

pcwalton · 2015-09-03T16:44:09Z

components/layout/traversal.rs

@@ -185,8 +185,8 @@ impl<'a> PreorderDomTraversal for RecalcStyleForNode<'a> {

                    if node.as_element().is_some() {
                        // Perform the CSS selector matching.
-                        let stylist = unsafe { &*self.layout_context.shared.stylist };
-                        node.match_node(stylist,
+                        let stylist = self.layout_context.shared.stylist.read().unwrap();


Can you make the stylist itself thread-safe? I worry this is going to be too slow.

I don't see how that's possible...

Actually, I'm pretty sure the current usage of Stylist on master is actually threadsafe without any additional locking. It's just impossible to prove that to the compiler at the moment because we can't attach a lifetime to SharedLayoutContext with the current WorkQueue API.

It would be nice if we could do that. I'm fine with changing the WorkQueue API.

I think taking a lock for every node here will probably erase most parallelism in Servo, so we can't land this.

pcwalton · 2015-09-03T16:45:12Z

components/layout/construct.rs

@@ -303,6 +303,7 @@ impl<'a> FlowConstructor<'a> {
            layout_data.remove_compositor_layers(self.layout_context
                                                     .shared
                                                     .constellation_chan
+                                                     .lock().unwrap()


This will probably be too slow. Can you pass the mutex to remove_compositor_layers and take the lock only if necessary?

eefriedman · 2015-09-04T02:22:47Z

Figured out how to avoid the Stylist locking. Current version is WIP; but I'd like to double-check that the approach is sane.

eefriedman · 2015-09-10T02:32:02Z

Updated. Not sure why I thought the other stuff needed mutexes; I was probably just confused by the comment claiming they do.

bors-servo · 2015-09-12T15:10:41Z

☔ The latest upstream changes (presumably #7613) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2015-09-17T12:35:30Z

☔ The latest upstream changes (presumably #7644) made this pull request unmergeable. Please resolve the merge conflicts.

eefriedman · 2015-09-20T23:42:35Z

Updated version. Rebased, and fixed so it adds zero new mutexes.

bors-servo · 2015-09-24T00:01:41Z

☔ The latest upstream changes (presumably #7710) made this pull request unmergeable. Please resolve the merge conflicts.

eefriedman · 2015-09-24T04:21:13Z

(I'm not planning to rebase this again until it gets reviewed.)

pcwalton · 2015-09-25T01:00:36Z

components/layout/layout_task.rs

+    /// The position and size of the visible rect for each layer. We do not build display lists
+    /// for any areas more than `DISPLAY_PORT_SIZE_FACTOR` screens away from this area.
+    pub visible_rects: Arc<HashMap<LayerId, Rect<Au>, DefaultState<FnvHasher>>>,
+}


What does "Frozen"/"Unfrozen" mean? I'm a bit confused. Is "Frozen" immutable? If so, that would be called "Sync" in Rust.

Frozen probably isn't the best terminology, but I was having trouble thinking of something better.

When we do layout, we have to build a SharedLayoutContext. To avoid extraneous locking/copying, it needs to have a pointer into the current LayoutTaskData. (There's probably more unnecessary copying that could be avoided as a followup, but I wanted to keep this from getting too complicated.) However, some bits need to be mutated while the SharedLayoutContext exists. So this patch splits LayoutTaskData into the bits which are immutably borrowed by SharedLayoutContext and the bits which need to be mutated. The "frozen" section is the one which is borrowed, and therefore can't be mutated.

eefriedman · 2015-09-29T23:15:34Z

Updated. Forgot a Sync bound in WorkQueue, so I managed to confuse myself about Sender (which in practice is Sync, but doesn't expose a Sync bound; see rust-lang/rfcs#1299 .) Renamed the usage of Frozen to Sync, which is essentially appropriate here given that SharedLayoutContext is Sync.


        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 #1299. Fixes #2604. Pretty sure the issue with the other version mentioned there was just that it held on to the lock too long.

bors-servo · 2015-09-30T01:58:59Z

☔ The latest upstream changes (presumably #7423) made this pull request unmergeable. Please resolve the merge conflicts.

pcwalton · 2015-11-02T18:11:44Z

This seems fine to me in general. I'm a little worried about performance of the font cache task mutex, but if it's measured to be not a problem then I'm fine with it.

eefriedman · 2015-11-03T03:00:36Z

I'm a little worried about performance of the font cache task mutex

I'm not sure how this could be an issue... each layout thread takes the lock exactly once in its entire lifetime.

pcwalton · 2015-11-03T05:07:16Z

Oh, OK then. In that case looks good after a rebase.

Ms2ger · 2015-11-06T20:43:06Z

Fwiw, I'd like to have this wait a bit; I'm looking into an alternative approach.

eefriedman · 2015-11-06T21:17:21Z

Okay.

I might try to split out the WorkQueue-related changes into a separate PR, since they're probably useful anyway.

frewsxcv · 2016-01-14T23:46:05Z

Any updates?

eefriedman · 2016-01-15T21:45:41Z

I'll revisit this once #9050 is resolved, I guess.

jdm · 2016-03-10T19:35:52Z

I don't think it's worth keeping this open - #9050 doesn't appear to be going anywhere without anybody to champion it, and this PR is linked from #2604 already.

highfive added the S-awaiting-review label Sep 3, 2015

eefriedman force-pushed the eefriedman:shared-layout-context branch from 1c8114f to 1d17d86 Sep 3, 2015

highfive assigned pcwalton Sep 3, 2015

pcwalton reviewed Sep 3, 2015
View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 3, 2015

eefriedman force-pushed the eefriedman:shared-layout-context branch from 1d17d86 to 8982fe4 Sep 4, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 4, 2015

eefriedman force-pushed the eefriedman:shared-layout-context branch 3 times, most recently from 6d5244a to a293c79 Sep 10, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 12, 2015

eefriedman force-pushed the eefriedman:shared-layout-context branch from a293c79 to c900824 Sep 15, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 17, 2015

eefriedman force-pushed the eefriedman:shared-layout-context branch from c900824 to b385969 Sep 20, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 24, 2015

pcwalton reviewed Sep 25, 2015
View changes

jdm mentioned this pull request Sep 29, 2015

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

Closed

eefriedman force-pushed the eefriedman:shared-layout-context branch from b385969 to 1086c59 Sep 29, 2015

eefriedman force-pushed the eefriedman:shared-layout-context branch from 1086c59 to e11c00a Sep 29, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 30, 2015

eefriedman mentioned this pull request Oct 12, 2015

Unnecessary indirection for 'unsafe' boxed vecs in layout #7986

Open

jdm removed the S-awaiting-review label Nov 2, 2015

eefriedman mentioned this pull request Nov 6, 2015

Reduce LayoutTaskData to the fields used for RPC. #8378

Merged

Ms2ger mentioned this pull request Dec 22, 2015

Remove type parameters from WorkQueue. #9050

Closed

frewsxcv added the S-blocked-on-external label Jan 16, 2016

jdm closed this Mar 10, 2016

servo / servo

Make SharedLayoutContext threadsafe. #7524

Make SharedLayoutContext threadsafe. #7524

eefriedman commented Sep 3, 2015

highfive commented Sep 3, 2015

Ms2ger commented Sep 3, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eefriedman commented Sep 4, 2015

eefriedman commented Sep 10, 2015

bors-servo commented Sep 12, 2015

bors-servo commented Sep 17, 2015

eefriedman commented Sep 20, 2015

bors-servo commented Sep 24, 2015

eefriedman commented Sep 24, 2015

This comment has been minimized.

This comment has been minimized.

eefriedman commented Sep 29, 2015

bors-servo commented Sep 30, 2015

pcwalton commented Nov 2, 2015

eefriedman commented Nov 3, 2015

pcwalton commented Nov 3, 2015

Ms2ger commented Nov 6, 2015

eefriedman commented Nov 6, 2015

frewsxcv commented Jan 14, 2016

eefriedman commented Jan 15, 2016

jdm commented Mar 10, 2016

servo / servo

Join GitHub today

Make SharedLayoutContext threadsafe. #7524

Make SharedLayoutContext threadsafe. #7524

Conversation

eefriedman commented Sep 3, 2015

highfive commented Sep 3, 2015

Ms2ger commented Sep 3, 2015

This comment has been minimized.

pcwalton Sep 3, 2015

This comment has been minimized.

pcwalton Sep 3, 2015

This comment has been minimized.

eefriedman Sep 3, 2015

This comment has been minimized.

pcwalton Sep 3, 2015

This comment has been minimized.

pcwalton Sep 3, 2015

eefriedman commented Sep 4, 2015

eefriedman commented Sep 10, 2015

bors-servo commented Sep 12, 2015

bors-servo commented Sep 17, 2015

eefriedman commented Sep 20, 2015

bors-servo commented Sep 24, 2015

eefriedman commented Sep 24, 2015

This comment has been minimized.

pcwalton Sep 25, 2015

This comment has been minimized.

eefriedman Sep 25, 2015

eefriedman commented Sep 29, 2015

bors-servo commented Sep 30, 2015

pcwalton commented Nov 2, 2015

eefriedman commented Nov 3, 2015

pcwalton commented Nov 3, 2015

Ms2ger commented Nov 6, 2015

eefriedman commented Nov 6, 2015

frewsxcv commented Jan 14, 2016

eefriedman commented Jan 15, 2016

jdm commented Mar 10, 2016