style: Avoid selector-matching when only the style attribute is changed. #15317

emilio · 2017-01-31T17:52:22Z

r? @bholley

This change is

highfive · 2017-01-31T17:52:26Z

Heads up! This PR modifies the following files:

@bholley: components/style/data.rs, components/style/restyle_hints.rs, components/style/rule_tree/mod.rs, components/style/traversal.rs, components/style/matching.rs

highfive · 2017-01-31T17:52:28Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

emilio · 2017-01-31T17:55:04Z

@bors-servo try

bors-servo · 2017-01-31T17:55:09Z

⌛ Trying commit 9830752 with merge 2b7124f...


        Auto merge of #15317 - emilio:style-attr-restyle, r=<try>

@bholley

style: Avoid selector-matching when only the style attribute is changed. r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15317)

emilio · 2017-01-31T18:04:41Z

cc @SimonSapin @heycam

bors-servo · 2017-01-31T19:04:16Z

💔 Test failed - linux-rel-css

emilio · 2017-01-31T19:55:05Z

So there are two tests failing there, and I believe none of them is my fault.

The first seems like a timing-related incremental table layout bug (/css21_dev/html4/border-collapse-dynamic-table-002.htm). I can reproduce it sort-of reliably with my patch, but the reflow logs are identical for both, and I've verified that the styles applying are correct (thus my conclusion).

The second I can't reproduce it at all, but seems similarly a reflow issue, not a styling issue:

emilio · 2017-01-31T19:55:16Z

@bors-servo retry

bors-servo · 2017-01-31T19:55:20Z

⌛ Trying commit 9830752 with merge 75a5eeb...


        Auto merge of #15317 - emilio:style-attr-restyle, r=<try>

@bholley

style: Avoid selector-matching when only the style attribute is changed. r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15317)

bholley

lgtm with those changes. Thanks for doing this!

bholley · 2017-01-31T20:51:31Z

components/style/data.rs

+/// The rule nodes for each of the pseudo-elements of an element.
+///
+/// TODO(emilio): Probably shouldn't be a `HashMap` by default, but a smaller
+/// array.


Can you file a bug against stylo-perf for this?

bholley · 2017-01-31T20:51:31Z

components/style/data.rs

+    ///
+    /// FIXME(emilio): We could in theory avoid creating these when we have
+    /// support for just re-cascading an element. Then the input to
+    /// `cascade_node` could be `MatchResults` or just `UseExistingStyle`.


bholley · 2017-01-31T20:51:31Z

components/style/data.rs

+    }
+
+    /// Whether the element holding the hint needs to be restyled on some way.
+    pub fn needs_restyle_self(&self) -> bool {


Let's call this has_self_invalidations.

bholley · 2017-01-31T20:51:31Z

components/style/data.rs

+    ///
+    /// This hint is stripped down, and only contains hints that are a subset of
+    /// RestyleHint::for_single_element().
+    pub for_self: RestyleHint,


bholley · 2017-01-31T20:51:31Z

components/style/traversal.rs

+
+                // TODO(emilio): Here we'll need to support animation-only hints
+                // and similar.
+                match_results = if data.needs_only_style_attribute_update() {


Given that it seems like we'll eventually want to do this the other way around, I think we want the opposite predicate here:

if data.restyle_self() {
// do full selector matching
} else {
if data.style_attribute_changed() {
// Handle style attributes
}
if data.tick_animation() {
// Handle animations
}
// etc
}

So let's get rid of needs_only_style_attribute_update() (since it doesn't scale), and replace it with restyle_self() and style_attribute_changed().

bholley · 2017-01-31T20:51:31Z

components/style/restyle_hints.rs

+impl RestyleHint {
+    /// The subset hints that affect the styling of a single element during the
+    /// traversal.
+    pub fn for_single_element() -> Self {


I'd call this for_self.

bholley · 2017-01-31T20:51:31Z

components/style/matching.rs

@@ -635,6 +627,40 @@ pub trait MatchMethods : TElement {
        }
    }

+    /// Updates the style attribute rule nodes without re-running selector
+    /// matching, using just the rule tree.
+    fn update_style_attribute(&self,


Given that we're going to be stacking this up with similar calls (for animations and transitions, etc), it seems like this should just accept the old rule node as an argument and return a rule node, rather than operating on ElementData and returning MatchResults. We can handle grabbing the old pseudo rule nodes at the callsite, once all the rule tree fixups have been performed.

bholley · 2017-01-31T20:51:31Z

components/style/matching.rs

+                              -> MatchResults {
+        let style_attribute = self.style_attribute();
+
+        let mut new_rule_node = data.styles().primary.rules.clone();


Nit: I'd just call this |rule_node|, because it's not new at this point, and it's confusing when we pass the old rule node to replace_rule_level_if_applicable as |new_rule_node|.

bholley · 2017-01-31T20:51:31Z

components/style/rule_tree/mod.rs

+        // transitions, but could not be so for SMIL animations, which we'd need
+        // to special-case (isn't hard, it's just about removing the `if` and
+        // special cases, and replacing them for a `while` loop, avoiding the
+        // optimizations).


Can you file this as a dependency to https://bugzilla.mozilla.org/show_bug.cgi?id=1302948 ?

bholley · 2017-01-31T20:51:31Z

components/style/rule_tree/mod.rs

+    /// Returns the resulting node that represents the new path.
+    ///
+    /// TODO(emilio): Maybe this should be in `StrongRuleNode`?
+    pub fn replace_rule_at_level_if_applicable(&self,


So, the if_applicable part of the name here deals with the fact that we might be either inserting or removing, instead of strictly replacing?

How about fn update_rule_at_level(...)?

bholley · 2017-01-31T20:51:47Z

@bors-servo delegate+

bors-servo · 2017-01-31T20:51:48Z

✌️ @emilio can now approve this pull request

bors-servo · 2017-01-31T21:09:52Z

💔 Test failed - linux-rel-css

emilio · 2017-02-01T13:31:53Z

@bors-servo try

bors-servo · 2017-02-01T13:31:57Z

⌛ Trying commit bc7c4f0 with merge 5980d6a...


        Auto merge of #15317 - emilio:style-attr-restyle, r=<try>

@bholley

style: Avoid selector-matching when only the style attribute is changed. r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15317)

emilio · 2017-02-01T13:32:21Z

Bobby, I added another large-ish commit refactoring stuff and adding support for the cascade-only thing, I'd appreciate if you can take another look at it.

bors-servo · 2017-02-01T14:52:20Z

💔 Test failed - mac-rel-css

emilio · 2017-02-01T17:38:39Z

@bors-servo retry

#14378

bors-servo · 2017-02-01T17:38:44Z

⌛ Trying commit bc7c4f0 with merge abc0548...


        Auto merge of #15317 - emilio:style-attr-restyle, r=<try>

@bholley

style: Avoid selector-matching when only the style attribute is changed. r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15317)

bors-servo · 2017-02-01T18:20:37Z

💔 Test failed - linux-rel-css

bholley · 2017-02-02T06:03:16Z

components/style/data.rs

+    /// Whether this element should be restyled during the traversal, and how.
+    ///
+    /// This hint is stripped down, and only contains hints that are a subset of
+    /// RestyleHint::for_single_element().


Nit: This comment needs updating.

bholley · 2017-02-02T06:23:00Z

components/style/traversal.rs

-                                                 shared_context, &mut data) }
-    };
+    // TODO(emilio): Make cascade_input less expensive to compute in the cases
+    // we don't need to run selector matching.


Yeah. I'm not super wild about the structure here, but I'm ok with fixing it up later.

bholley · 2017-02-02T06:23:09Z

@bors-servo delegate+

bors-servo · 2017-02-02T06:23:10Z

✌️ @emilio can now approve this pull request


        style: Avoid selector-matching when only the style attribute is changed.


        style: Refactor the traversal so it's more easy to read and straight-…

…forward.

emilio · 2017-02-02T11:00:19Z

@bors-servo r=bholley

I backed out the first inherited_style_changed patch. It's due to a bug in servo's style damage computation, but I'll submit the fix separately.

bors-servo · 2017-02-02T11:00:19Z

📌 Commit 2594cb9 has been approved by bholley

bors-servo · 2017-02-02T11:00:24Z

⌛ Testing commit 2594cb9 with merge fb7f65f...


        Auto merge of #15317 - emilio:style-attr-restyle, r=bholley

@bholley

style: Avoid selector-matching when only the style attribute is changed. r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15317)

bors-servo · 2017-02-02T11:54:38Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: bholley
Pushing fb7f65f to master...

highfive assigned bholley Jan 31, 2017

highfive added the S-awaiting-review label Jan 31, 2017

emilio mentioned this pull request Jan 31, 2017

MotionMark: improve performance #15163

Open

highfive added the S-tests-failed label Jan 31, 2017

bholley reviewed Jan 31, 2017

View changes

highfive added the S-tests-failed label Jan 31, 2017

emilio force-pushed the emilio:style-attr-restyle branch from 9830752 to 758c965 Feb 1, 2017

highfive removed the S-tests-failed label Feb 1, 2017

emilio force-pushed the emilio:style-attr-restyle branch from 758c965 to bc7c4f0 Feb 1, 2017

highfive added the S-tests-failed label Feb 1, 2017

bholley reviewed Feb 2, 2017

View changes

emilio added 2 commits Jan 31, 2017

style: Avoid selector-matching when only the style attribute is changed.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

8859aa0

style: Refactor the traversal so it's more easy to read and straight-…

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

2594cb9

…forward.

emilio force-pushed the emilio:style-attr-restyle branch from bc7c4f0 to 2594cb9 Feb 2, 2017

highfive removed the S-tests-failed label Feb 2, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 2, 2017

bors-servo merged commit 2594cb9 into servo:master Feb 2, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Feb 2, 2017

emilio deleted the emilio:style-attr-restyle branch Feb 2, 2017

servo / servo

Join GitHub today

style: Avoid selector-matching when only the style attribute is changed. #15317

style: Avoid selector-matching when only the style attribute is changed. #15317

Conversation

emilio commented Jan 31, 2017 • edited by larsbergstrom

highfive commented Jan 31, 2017

highfive commented Jan 31, 2017

emilio commented Jan 31, 2017

bors-servo commented Jan 31, 2017

emilio commented Jan 31, 2017

bors-servo commented Jan 31, 2017

emilio commented Jan 31, 2017 • edited

emilio commented Jan 31, 2017

bors-servo commented Jan 31, 2017

bholley left a comment

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.

bholley commented Jan 31, 2017

bors-servo commented Jan 31, 2017

bors-servo commented Jan 31, 2017

emilio commented Feb 1, 2017

bors-servo commented Feb 1, 2017

emilio commented Feb 1, 2017

bors-servo commented Feb 1, 2017

emilio commented Feb 1, 2017

bors-servo commented Feb 1, 2017

bors-servo commented Feb 1, 2017

This comment has been minimized.

This comment has been minimized.

bholley commented Feb 2, 2017

bors-servo commented Feb 2, 2017

emilio commented Feb 2, 2017

bors-servo commented Feb 2, 2017

bors-servo commented Feb 2, 2017

bors-servo commented Feb 2, 2017

emilio commented Jan 31, 2017 •

edited by larsbergstrom

emilio commented Jan 31, 2017 •

edited