incremental restyle: Introduce StylingMode and deprecate explicit dirtiness #13913

bholley · 2016-10-25T01:52:22Z

This is another chunk of work to move us toward the new incremental restyle architecture.

Eventually, we'll make a fine-grained decision at each node about what style to recompute based on the RestyleHint on the node data (along with other things). For now, we use the existence of RestyleData as a coarse-grained approximation of this.

This change is

highfive · 2016-10-25T01:52:27Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/layout_wrapper.rs
@fitzgen: components/script/layout_wrapper.rs
@emilio: components/layout/traversal.rs, ports/geckolib/glue.rs, components/style/parallel.rs, components/style/data.rs, components/style/sequential.rs, components/style/dom.rs, components/style/gecko/traversal.rs, components/style/gecko/wrapper.rs, components/style/traversal.rs

highfive · 2016-10-25T01:52:28Z

Warning

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

bholley · 2016-10-25T01:52:31Z

r? @emilio

bholley · 2016-10-25T01:52:36Z

@bors-servo try

bors-servo · 2016-10-25T01:52:39Z

⌛ Trying commit 87a6ed0 with merge bc16c39...


        Auto merge of #13913 - bholley:styling_mode, r=<try>

incremental restyle: Introduce StylingMode and deprecate explicit dirtiness This is another chunk of work to move us toward the new incremental restyle architecture. Eventually, we'll make a fine-grained decision at each node about what style to recompute based on the RestyleHint on the node data (along with other things). For now, we use the existence of RestyleData as a coarse-grained approximation of this.

bors-servo · 2016-10-25T02:40:12Z

💔 Test failed - mac-rel-css

jdm · 2016-10-25T03:48:52Z

  ▶ FAIL [expected PASS] /css21_dev/html4/cascade-import-dynamic-002.htm
  └   → /css21_dev/html4/cascade-import-dynamic-002.htm f56352da93639a2cc2b3d31b071a73c459b8b396
/css21_dev/html4/reference/ref-this-text-should-be-green.htm 28d8045f7834b63ec9aa27d6aee5f8c94c5b86e7
Testing f56352da93639a2cc2b3d31b071a73c459b8b396 == 28d8045f7834b63ec9aa27d6aee5f8c94c5b86e7

  ▶ FAIL [expected PASS] /css21_dev/html4/cascade-import-dynamic-004.htm
  └   → /css21_dev/html4/cascade-import-dynamic-004.htm f56352da93639a2cc2b3d31b071a73c459b8b396
/css21_dev/html4/reference/ref-this-text-should-be-green.htm 28d8045f7834b63ec9aa27d6aee5f8c94c5b86e7
Testing f56352da93639a2cc2b3d31b071a73c459b8b396 == 28d8045f7834b63ec9aa27d6aee5f8c94c5b86e7

bholley · 2016-10-25T04:31:47Z

@bors-servo try

bors-servo · 2016-10-25T04:31:54Z

⌛ Trying commit 9d8e321 with merge 61d3604...


        Auto merge of #13913 - bholley:styling_mode, r=<try>

incremental restyle: Introduce StylingMode and deprecate explicit dirtiness This is another chunk of work to move us toward the new incremental restyle architecture. Eventually, we'll make a fine-grained decision at each node about what style to recompute based on the RestyleHint on the node data (along with other things). For now, we use the existence of RestyleData as a coarse-grained approximation of this.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13913)

bors-servo · 2016-10-25T05:15:06Z

💔 Test failed - mac-rel-css

bholley · 2016-10-25T07:13:17Z

@bors-servo retry

bors-servo · 2016-10-25T07:13:19Z

⌛ Trying commit 9d8e321 with merge cc6ed99...


        Auto merge of #13913 - bholley:styling_mode, r=<try>

incremental restyle: Introduce StylingMode and deprecate explicit dirtiness This is another chunk of work to move us toward the new incremental restyle architecture. Eventually, we'll make a fine-grained decision at each node about what style to recompute based on the RestyleHint on the node data (along with other things). For now, we use the existence of RestyleData as a coarse-grained approximation of this.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13913)

emilio · 2016-10-25T07:38:33Z

Super excited to review this!

The last test failures are #13887 and a new pass (!):

  ▶ PASS [expected FAIL] /css21_dev/html4/c5525-fltblck-000.htm

bors-servo · 2016-10-25T07:49:14Z

💔 Test failed - linux-dev

emilio · 2016-10-25T07:49:42Z

./components/layout/traversal.rs:113: Line is longer than 120 characters

bholley · 2016-10-25T16:35:01Z

@bors-servo try

bors-servo · 2016-10-25T20:43:12Z

💔 Test failed - linux-dev

bholley · 2016-10-25T20:50:18Z

@bors-servo retry

error: linking with cc failed: exit code: 1

bors-servo · 2016-10-25T20:50:20Z

⌛ Trying commit 97a9741 with merge e20b584...


        Auto merge of #13913 - bholley:styling_mode, r=<try>

incremental restyle: Introduce StylingMode and deprecate explicit dirtiness This is another chunk of work to move us toward the new incremental restyle architecture. Eventually, we'll make a fine-grained decision at each node about what style to recompute based on the RestyleHint on the node data (along with other things). For now, we use the existence of RestyleData as a coarse-grained approximation of this.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13913)

bors-servo · 2016-10-25T22:06:11Z

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

emilio · 2016-10-26T07:09:12Z

components/style/dom.rs

@@ -212,7 +274,7 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
    fn attr_equals(&self, namespace: &Namespace, attr: &Atom, value: &Atom) -> bool;

    /// Properly marks nodes as dirty in response to restyle hints.
-    fn note_restyle_hint(&self, hint: RestyleHint) {
+    fn note_restyle_hint<C: DomTraversalContext<Self::ConcreteNode>>(&self, hint: RestyleHint) {


hm... This seems clunky, but I guess it's ok.

Yes, it will go away soon anyway.

emilio · 2016-10-26T07:09:12Z

components/style/gecko/traversal.rs

@@ -29,7 +31,7 @@ impl<'lc, 'ln> DomTraversalContext<GeckoNode<'ln>> for RecalcStyleOnly<'lc> {
    }

    fn process_preorder(&self, node: GeckoNode<'ln>) -> RestyleResult {
-        recalc_style_at(&self.context, self.root, node)
+        recalc_style_at::<GeckoNode<'ln>, StandaloneStyleContext, Self>(&self.context, self.root, node)


Probably the two first parameters can be inferred, right? (using _).

Oh nice, I didn't know that worked.

emilio · 2016-10-26T07:09:12Z

components/style/traversal.rs

+
+        for kid in parent.children() {
+            if kid.styling_mode() != StylingMode::Stop {
+                if !marked_dirty_descendants {


seems like set_dirty_descendants should be cheap enough we shouldn't need this?

In Gecko it's an FFI call, so seems like we might as well avoid it when we can.

Seems like we can check that flag before the FFI call only in the gecko side.

emilio · 2016-10-26T07:09:12Z

components/style/data.rs

@@ -175,25 +175,24 @@ impl NodeData {
        self.styles = match mem::replace(&mut self.styles, Uninitialized) {
            Uninitialized => Previous(f()),
            Current(x) => Previous(Some(x)),
-            _ => panic!("Already have previous styles"),


I guess this change is because we call gather_previous_styles unconditionally on all the children even if we end up not styling them, is that right?

Yeah, at least for now it's more convenient for gather_previous_styles to be idempotent.

emilio · 2016-10-26T07:09:12Z

components/style/dom.rs

+                if d.restyle_data.is_some() || self.deprecated_dirty_bit_is_set() {
+                    Restyle
+                } else {
+                    debug_assert!(!self.frame_has_style()); // display:none etc


Seems like mode_for_descendants could be stop in the display: none case for example. I see we're still using RestyleResult, but it'd be nice to leave a single mechanism in place.

Refactoring the code to remove the usage of RestyleResult is on my list, but I'm trying to keep the patches small-ish.

emilio · 2016-10-26T07:09:12Z

components/style/traversal.rs

-                parent.set_dirty_descendants();
+    /// Helper for the traversal implementations to select the children that
+    /// should be enqueued for processing.
+    fn traverse_children<F: FnMut(N)>(parent: N, mut f: F)


Maybe rename this to traverse_children_needing_restyle or something like that that implies that it's more than a for loop?

Per IRC discussion, this name doesn't work, because the nodes might be StylingMode::Traverse (in which case they don't need a restyle) or StylingMode::Initial (In which case it's not a restyle). The eventual terminology I'm trying to move to is that 'traverse' means visiting in some capacity, and 'process' means doing actual work.


        Introduce StylingMode and deprecate explicit dirtiness.

MozReview-Commit-ID: 5tF075EJKBa

emilio · 2016-10-26T11:36:03Z

@bors-servo: r+

bors-servo · 2016-10-26T11:36:04Z

📌 Commit 05c1f1e has been approved by emilio

bors-servo · 2016-10-26T11:36:07Z

⌛ Testing commit 05c1f1e with merge c8b6ece...


        Auto merge of #13913 - bholley:styling_mode, r=emilio

incremental restyle: Introduce StylingMode and deprecate explicit dirtiness This is another chunk of work to move us toward the new incremental restyle architecture. Eventually, we'll make a fine-grained decision at each node about what style to recompute based on the RestyleHint on the node data (along with other things). For now, we use the existence of RestyleData as a coarse-grained approximation of this.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13913)

bors-servo · 2016-10-26T12:09:21Z

💔 Test failed - linux-dev

bholley · 2016-10-26T16:58:37Z

@bors-servo retry p=1

LLVM ERROR: IO failure on output stream

bors-servo · 2016-10-26T16:58:41Z

⚡ Previous build results for arm32, arm64, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-dev...

bors-servo · 2016-10-26T17:06:26Z

💔 Test failed - linux-dev

bholley · 2016-10-26T17:07:21Z

@bors-servo retry

note: collect2: error: ld returned 1 exit status

bors-servo · 2016-10-26T17:07:22Z

⚡ Previous build results for arm32, arm64, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-dev...

bors-servo · 2016-10-26T17:26:36Z

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

highfive assigned mbrubeck Oct 25, 2016

highfive added the S-awaiting-review label Oct 25, 2016

highfive assigned emilio and unassigned mbrubeck Oct 25, 2016

highfive added the S-tests-failed label Oct 25, 2016

bholley force-pushed the bholley:styling_mode branch from 87a6ed0 to 9d8e321 Oct 25, 2016

highfive removed the S-tests-failed label Oct 25, 2016

highfive added the S-tests-failed label Oct 25, 2016

bholley force-pushed the bholley:styling_mode branch from 9d8e321 to 97a9741 Oct 25, 2016

highfive removed the S-tests-failed label Oct 25, 2016

highfive added the S-tests-failed label Oct 25, 2016

emilio reviewed Oct 26, 2016

View changes

Introduce StylingMode and deprecate explicit dirtiness.

Loading status checks…

05c1f1e

MozReview-Commit-ID: 5tF075EJKBa

bholley force-pushed the bholley:styling_mode branch from 97a9741 to 05c1f1e Oct 26, 2016

highfive removed the S-tests-failed label Oct 26, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 26, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 26, 2016

highfive added the S-tests-failed label Oct 26, 2016

bors-servo merged commit 05c1f1e into servo:master Oct 26, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Oct 26, 2016

Use rayon to drive parallel layout and styling. #13641

Merged

Rule tree, v1 #13202

Merged

bholley deleted the bholley:styling_mode branch Oct 30, 2016

servo / servo

Join GitHub today

incremental restyle: Introduce StylingMode and deprecate explicit dirtiness #13913

incremental restyle: Introduce StylingMode and deprecate explicit dirtiness #13913

Conversation

bholley commented Oct 25, 2016 • edited by larsbergstrom

highfive commented Oct 25, 2016

highfive commented Oct 25, 2016

bholley commented Oct 25, 2016

bholley commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 25, 2016

jdm commented Oct 25, 2016

bholley commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bholley commented Oct 25, 2016

bors-servo commented Oct 25, 2016

emilio commented Oct 25, 2016

bors-servo commented Oct 25, 2016

emilio commented Oct 25, 2016

bholley commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bholley commented Oct 25, 2016

bors-servo commented Oct 25, 2016

bors-servo commented Oct 25, 2016

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.

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bholley commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bholley commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bholley commented Oct 25, 2016 •

edited by larsbergstrom