style: Remove the Mutex from new_animations_sender by moving it to the local StyleContext. #11946

emilio · 2016-06-30T04:07:13Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors

These changes do not require tests because refactoring.

As a follow-up, we could move all the data living under a mutex in the
SharedLayoutContext only in order to create the local context to the same place.

This should increase animation performance when there are multiple animations in
one page that happen to be on different threads.

r? @SimonSapin/@mbrubeck for the style/layout, @bholley for the geckolib changes

This change is

highfive · 2016-06-30T04:07:16Z

Heads up! This PR modifies the following files:

@bholley: components/style/parallel.rs, components/style/animation.rs, components/style/context.rs, components/style/matching.rs

highfive · 2016-06-30T04:07:17Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style and layout code, but no tests are modified. Please consider adding a test!

bholley · 2016-06-30T16:47:44Z

components/layout/context.rs

@@ -91,6 +95,9 @@ pub struct SharedLayoutContext {
    /// Interface to the font cache thread.
    pub font_cache_thread: Mutex<FontCacheThread>,

+    /// A channel used to send the new animations back to layout.
+    pub new_animations_sender: Mutex<Sender<Animation>>,


Wait, what's this about? Isn't the whole point to move this to the local context?

emilio · 2016-06-30T17:55:23Z

Review status: 0 of 12 files reviewed at latest revision, 4 unresolved discussions.

components/layout/context.rs, line 99 [r1] (raw file):

Previously, bholley (Bobby Holley) wrote…

Wait, what's this about? Isn't the whole point to move this to the local context?

Yep, so the problem here is that servo relies on being able to create a `LayoutContext` (that includes all the local contexts) from a `SharedLayoutContext`. And since `SharedLayoutContext` needs to be thread-safe, the only way of having an unmutexed copy in the local style context (which conceptually is the right thing to do), is having a thread-safe version in the `SharedLayoutContext`.

This is pretty unfortunate, but since the layout contexts are created lazily there isn't too much that we can do about it.

ports/geckolib/context.rs, line 3 [r1] (raw file):

Previously, bholley (Bobby Holley) wrote…

For reviewability, it would be much better to do the file-hoisting and code-refactoring in separate commits.

Fair enough.

ports/geckolib/context.rs, line 39 [r1] (raw file):

Previously, bholley (Bobby Holley) wrote…

We've generally used the term 'context' to refer to ambient data used by the style system algorithms, and 'data' to refer to computed style data that we hang off the nodes (see the context.rs/data.rs split). I'd rather not confuse that here.

Yep, I'm not happy with the name here, any idea?

ports/geckolib/context.rs, line 45 [r1] (raw file):

Previously, bholley (Bobby Holley) wrote…

More generally though, I'm not super convinced of the usefulness here. Wouldn't it be better to just leave the Mutexed Sender on the SharedStyleData, and make a local un-Mutexed clone for the local style context? That would avoid the need to introduce a new data structure here with someone-ambiguous meaning, and also avoid the need to add it to SharedLayoutContext above.

That's an option, certainly, but I'd rather not keep both copies on the style context. Conceptually, the style context should only know about the unmutexed version, and use that. How the local context is created is up to layout / stylo.

Comments from Reviewable

bholley · 2016-07-01T00:58:00Z

Review status: 0 of 12 files reviewed at latest revision, 4 unresolved discussions.

ports/geckolib/context.rs, line 45 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

That's an option, certainly, but I'd rather not keep both copies on the style context. Conceptually, the style context should only know about the unmutexed version, and use that. How the local context is created is up to layout / stylo.

But this change puts the Mutexed version directly on the StyleContext, right?

It seems like StandaloneStyleContext::new should take an additional struct called LocalStyleContextCreationInfo, which it uses to create the LocalStyleContext as necessary. If we actually need this on the StandaloneStyleContext after initialization, that seems like an indicator that it belongs on the SharedStyleContext.

Comments from Reviewable

emilio · 2016-07-01T17:50:31Z

Review status: 0 of 13 files reviewed at latest revision, 4 unresolved discussions.

ports/geckolib/context.rs, line 45 [r1] (raw file):

Previously, bholley (Bobby Holley) wrote…

But this change puts the Mutexed version directly on the StyleContext, right?

It seems like StandaloneStyleContext::new should take an additional struct called LocalStyleContextCreationInfo, which it uses to create the LocalStyleContext as necessary. If we actually need this on the StandaloneStyleContext after initialization, that seems like an indicator that it belongs on the SharedStyleContext.

Yep, after thinking a bit more about it, you're right, so I've just done that.

Comments from Reviewable

bholley · 2016-07-01T20:17:33Z

Reviewed 5 of 12 files at r1, 7 of 8 files at r2.
Review status: 12 of 13 files reviewed at latest revision, 2 unresolved discussions.

components/style/context.rs, line 21 [r2] (raw file):

/// This structure is used to create a local style context from a shared one.
pub struct LocalStyleContextCreationData<Impl: SelectorImplExt> {

I think I'd rather call this LocalStyleContextCreationInfo instead, given the aforementioned overloading of the word 'data', but I won't insist if it's a pain.

components/style/matching.rs, line 376 [r2] (raw file):

    fn cascade_node_pseudo_element(&self,
                                   context: &SharedStyleContext<<Self::ConcreteElement as Element>::Impl>,
                                   local_context: &LocalStyleContext<<Self::ConcreteElement as Element>::Impl>,

Instead of passing both contexts everywhere, why not just pass an implementation of StyleContext, from which both the local and shared contexts can be fetched?

Comments from Reviewable

bholley · 2016-07-01T20:18:12Z

r=me on the whole patch with those fixes. @bors-servo delegate+

bors-servo · 2016-07-01T20:18:14Z

✌️ @emilio can now approve this pull request

…e local StyleContext. As a follow-up, we could move all the data living under a mutex in the SharedLayoutContext only in order to create the local context to the same place. This should increase animation performance when there are multiple animations in one page that happen to be on different threads.

emilio · 2016-07-01T20:48:43Z

@bors-servo: r=bholley

bors-servo · 2016-07-01T20:48:44Z

📌 Commit 203d2a6 has been approved by bholley

bors-servo · 2016-07-01T21:12:55Z

⌛ Testing commit 203d2a6 with merge ec0d3e0...

@mbrubeck

style: Remove the Mutex from new_animations_sender by moving it to the local StyleContext.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because refactoring.  As a follow-up, we could move all the data living under a mutex in the SharedLayoutContext only in order to create the local context to the same place. This should increase animation performance when there are multiple animations in one page that happen to be on different threads. r? @SimonSapin/@mbrubeck for the style/layout, @bholley for the geckolib changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11946)

bors-servo · 2016-07-01T22:06:29Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned SimonSapin Jun 30, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 30, 2016

bholley reviewed Jun 30, 2016
View reviewed changes

emilio force-pushed the animations-sender branch from be27e4c to dd33d6d Compare July 1, 2016 17:49

bholley assigned bholley and unassigned SimonSapin Jul 1, 2016

emilio force-pushed the animations-sender branch from dd33d6d to 203d2a6 Compare July 1, 2016 20:48

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 1, 2016

bors-servo merged commit 203d2a6 into servo:master Jul 1, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: Remove the Mutex from new_animations_sender by moving it to the local StyleContext. #11946

style: Remove the Mutex from new_animations_sender by moving it to the local StyleContext. #11946

emilio commented Jun 30, 2016 •

edited by larsbergstrom

Loading

highfive commented Jun 30, 2016

highfive commented Jun 30, 2016

bholley Jun 30, 2016

emilio commented Jun 30, 2016

bholley commented Jul 1, 2016

emilio commented Jul 1, 2016

bholley commented Jul 1, 2016

bholley commented Jul 1, 2016

bors-servo commented Jul 1, 2016

emilio commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 1, 2016

style: Remove the Mutex from new_animations_sender by moving it to the local StyleContext. #11946

style: Remove the Mutex from new_animations_sender by moving it to the local StyleContext. #11946

Conversation

emilio commented Jun 30, 2016 • edited by larsbergstrom Loading

highfive commented Jun 30, 2016

highfive commented Jun 30, 2016

bholley Jun 30, 2016

Choose a reason for hiding this comment

emilio commented Jun 30, 2016

bholley commented Jul 1, 2016

emilio commented Jul 1, 2016

bholley commented Jul 1, 2016

bholley commented Jul 1, 2016

bors-servo commented Jul 1, 2016

emilio commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 1, 2016

emilio commented Jun 30, 2016 •

edited by larsbergstrom

Loading