Rewrite the style sharing candidate cache #12668

emilio · 2016-07-31T06:53:22Z

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Style sharing candidate cache code is probably not working as intended. #12534

There are tests for these changes OR

This change is

highfive · 2016-07-31T06:53:25Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs, components/style/sequential.rs, components/style/traversal.rs, components/style/matching.rs, components/style/Cargo.toml, components/style/restyle_hints.rs, components/style/dom.rs, components/style/context.rs, components/style/parallel.rs
@KiChjang: components/script/Cargo.toml, components/script/layout_wrapper.rs, components/script_layout_interface/Cargo.toml

emilio · 2016-07-31T06:53:33Z

@bors-servo: try

bors-servo · 2016-07-31T06:53:35Z

⌛ Trying commit f33cbb0 with merge a93bb08...

[wip] Rewrite the style sharing candidate cache  See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12534  - [x] There are tests for these changes OR

bors-servo · 2016-07-31T07:55:39Z

💔 Test failed - linux-rel

highfive · 2016-07-31T07:55:41Z

  ▶ FAIL [expected PASS] /_mozilla/css/restyle_hints_state.html
  └   → /_mozilla/css/restyle_hints_state.html e0244585b680e30247daff27d091c5acb570cb70
/_mozilla/css/restyle_hints_state_ref.html 4b148bc27c3ec95f7eba64524b0ce61ea150d26a
Testing e0244585b680e30247daff27d091c5acb570cb70 == 4b148bc27c3ec95f7eba64524b0ce61ea150d26a

emilio · 2016-07-31T23:38:14Z

Hah! Funnily enough, that test fails because of (the lack of) emilio/rust-selectors@4c43337, bug that is in current rust selectors.

@bors-servo: try

bors-servo · 2016-07-31T23:38:16Z

⌛ Trying commit b1a4217 with merge 729ed50...

[wip] Rewrite the style sharing candidate cache  See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12534  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12668)

samlh · 2016-08-01T00:13:03Z

components/style/traversal.rs

                let shareable_element = match node.as_element() {
                    Some(element) => {
+                        if opts::get().style_sharing_stats {


It may be faster to just do the add unconditionally than to take a lock to check the option.

opts is not the same as prefs, and opts::get() is just getting a lazy-static, not a RWLock as with the preferences.

bors-servo · 2016-08-01T10:23:58Z

💥 Test timed out

emilio · 2016-08-01T16:24:45Z

@bors-servo: retry

bors-servo · 2016-08-01T16:25:00Z

⌛ Trying commit b1a4217 with merge 477db79...

[wip] Rewrite the style sharing candidate cache  See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12534  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12668)

bors-servo · 2016-08-01T17:42:03Z

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

emilio · 2016-08-01T17:43:42Z

Yay!

r? @pcwalton (still some cleanup needed, but should be good enough to get it reviewed).

pcwalton · 2016-08-02T21:16:31Z

components/style/selector_matching.rs

+        debug!("Stylist stats:");
+        debug!(" - Got {} sibling-affecting selectors",
+               self.sibling_affecting_selectors.len());
+        debug!(" - Got {} non-commong-style-attribute-affecting selectors",


typo: "commong" -> "common"

pcwalton · 2016-08-02T21:17:56Z

This look good cc @emilio

emilio · 2016-08-02T23:28:18Z

cool, can I r=you when I clean this up? (and the relevant selectors pr lands)

pcwalton · 2016-08-03T17:42:18Z

I'd like to take one final look over it after you clean it up, but yeah, basically.

@SimonSapin

Redesign the style sharing API. See servo/servo#12668 for context. r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-selectors/93)

emilio · 2016-08-11T06:18:05Z

@bors-servo: try

bors-servo · 2016-08-12T01:28:16Z

⌛ Trying commit 8a6858a with merge b93d5e9...

bors-servo · 2016-08-12T03:15:52Z

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

SimonSapin · 2016-08-17T21:03:39Z

components/style/matching.rs

+fn have_same_class<E: TElement>(element: &E, candidate: &E) -> bool {
+    // XXX Efficiency here, I'm only validating ideas.
+    let mut first = vec![];
+    let mut second = vec![];


These could be sets to be more accurate (fewer misses). If on the other hand we don’t mind being conservative (more misses) comparing the string values of class attributes might be slightly faster than building these vecs.

Sets or sorted vectors.

SimonSapin · 2016-08-17T21:08:58Z

Looks good to me.

@pcwalton Could you have that last look? This is upgrading to an already-released version of selector that has breaking changes, so it blocks using in Servo further breaking changes to selectors. (Version numbers don’t branch easily.)

pcwalton · 2016-08-17T21:11:46Z

r=me when rebased. Awesome!

The style candidate cache had regressed a few times (see servo#12534), and my intuition is that being able to disable all style sharing with a single rule in the page is really unfortunate. This commit redesigns the style sharing cache in order to be a optimistic cache, but then reject candidates if they match different sibling-affecting selectors in the page, for example. So far the numbers have improved, but not so much as I'd wanted (~10%/20% of non-incremental restyling time in general). The current implementation is really dumb though (we recompute and re-match a lot of stuff), so we should be able to optimise it quite a bit. I have different ideas for improving it (that may or may not work), apart of the low-hanging fruit like don't re-matching candidates all the time but I have to measure the real impact. Also, I need to verify it against try.

Haven't measured a lot, will do tomorrow, but I want a test run.

This was the underlying cause of the restyle_hints_state.html failure. It was uncovered only when fixing the HAS_EDGE_CHILD_SELECTOR flags because this was the series of events happening. * <script> starts executing. * All the restyle hint processing takes place. * <script> ends executing, gets appended to the body (I think this is also a bug). * <body> receives children_changed notification, with an Append mutation. * <body> had the HAS_EDGE_CHILD_SELECTOR flag, so due to its bogus value, it restyled the whole tree after <body>, fixing any mishandling of restyle hints.

…th children_changed.

emilio · 2016-08-17T21:18:57Z

@bors-servo: r=SimonSapin,pcwalton

Will fill followups for the perf improvements.

bors-servo · 2016-08-17T21:18:58Z

📌 Commit a46f0c3 has been approved by SimonSapin,pcwalton

bors-servo · 2016-08-17T21:34:31Z

⌛ Testing commit a46f0c3 with merge ec7efff...

Rewrite the style sharing candidate cache  See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12534  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12668)

bors-servo · 2016-08-17T21:54:38Z

💔 Test failed - linux-rel

emilio · 2016-08-17T22:06:50Z

@bors-servo: retry

That was my fault, I was investigating WebGL tests failures in the build
slave and I had a wpt socket already up, sorry.

On 08/17/2016 02:54 PM, bors-servo wrote:

💔 Test failed - linux-rel
http://build.servo.org/builders/linux-rel/builds/2689

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12668 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABQwupceLBqmNCwTaYrzb6mpEVLvxlvPks5qg4MsgaJpZM4JY-U9.

bors-servo · 2016-08-17T23:28:12Z

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

bors-servo · 2016-08-18T00:49:45Z

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

highfive assigned Manishearth Jul 31, 2016

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

highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 31, 2016

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jul 31, 2016

samlh reviewed Aug 1, 2016
View reviewed changes

highfive assigned pcwalton and unassigned Manishearth Aug 1, 2016

pcwalton reviewed Aug 2, 2016
View reviewed changes

emilio mentioned this pull request Aug 3, 2016

Redesign the style sharing API. servo/rust-selectors#93

Merged

emilio force-pushed the style-cache branch from b1a4217 to 058397c Compare August 11, 2016 06:17

emilio changed the title ~~[wip] Rewrite the style sharing candidate cache~~ Rewrite the style sharing candidate cache Aug 12, 2016

SimonSapin reviewed Aug 17, 2016
View reviewed changes

emilio added 5 commits August 17, 2016 14:16

Tidy up, make the cache a bit more performant.

173ec26

Haven't measured a lot, will do tomorrow, but I want a test run.

style: Run restyle_hints_state.html onload, so it doesn't interact wi…

51b6568

…th children_changed.

Lazily compute common style affecting attribute info.

a46f0c3

emilio force-pushed the style-cache branch from 8a6858a to a46f0c3 Compare August 17, 2016 21:17

highfive assigned SimonSapin and unassigned pcwalton Aug 17, 2016

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 Aug 17, 2016

emilio mentioned this pull request Aug 17, 2016

Lazily compute and cache the classnames of elements in the style sharing cache. #12912

Closed

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 17, 2016

bors-servo merged commit a46f0c3 into servo:master Aug 18, 2016

bors-servo mentioned this pull request Aug 18, 2016

Remove unused imports. #12903

Merged

emilio deleted the style-cache branch August 27, 2016 03:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the style sharing candidate cache #12668

Rewrite the style sharing candidate cache #12668

emilio commented Jul 31, 2016 •

edited by larsbergstrom

highfive commented Jul 31, 2016

emilio commented Jul 31, 2016

bors-servo commented Jul 31, 2016

bors-servo commented Jul 31, 2016

highfive commented Jul 31, 2016

emilio commented Jul 31, 2016 •

edited

bors-servo commented Jul 31, 2016

samlh Aug 1, 2016

emilio Aug 1, 2016

bors-servo commented Aug 1, 2016

emilio commented Aug 1, 2016 •

edited

bors-servo commented Aug 1, 2016

bors-servo commented Aug 1, 2016

emilio commented Aug 1, 2016

pcwalton Aug 2, 2016

pcwalton commented Aug 2, 2016

emilio commented Aug 2, 2016 •

edited

pcwalton commented Aug 3, 2016

emilio commented Aug 11, 2016

bors-servo commented Aug 12, 2016

bors-servo commented Aug 12, 2016

SimonSapin Aug 17, 2016

SimonSapin Aug 17, 2016

SimonSapin commented Aug 17, 2016

pcwalton commented Aug 17, 2016

emilio commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

emilio commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 18, 2016

Rewrite the style sharing candidate cache #12668

Rewrite the style sharing candidate cache #12668

Conversation

emilio commented Jul 31, 2016 • edited by larsbergstrom

highfive commented Jul 31, 2016

emilio commented Jul 31, 2016

bors-servo commented Jul 31, 2016

bors-servo commented Jul 31, 2016

highfive commented Jul 31, 2016

emilio commented Jul 31, 2016 • edited

bors-servo commented Jul 31, 2016

samlh Aug 1, 2016

Choose a reason for hiding this comment

emilio Aug 1, 2016

Choose a reason for hiding this comment

bors-servo commented Aug 1, 2016

emilio commented Aug 1, 2016 • edited

bors-servo commented Aug 1, 2016

bors-servo commented Aug 1, 2016

emilio commented Aug 1, 2016

pcwalton Aug 2, 2016

Choose a reason for hiding this comment

pcwalton commented Aug 2, 2016

emilio commented Aug 2, 2016 • edited

pcwalton commented Aug 3, 2016

emilio commented Aug 11, 2016

bors-servo commented Aug 12, 2016

bors-servo commented Aug 12, 2016

SimonSapin Aug 17, 2016

Choose a reason for hiding this comment

SimonSapin Aug 17, 2016

Choose a reason for hiding this comment

SimonSapin commented Aug 17, 2016

pcwalton commented Aug 17, 2016

emilio commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

emilio commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 18, 2016

emilio commented Jul 31, 2016 •

edited by larsbergstrom

emilio commented Jul 31, 2016 •

edited

emilio commented Aug 1, 2016 •

edited

emilio commented Aug 2, 2016 •

edited