Refactor the cascade #9929

SimonSapin · 2016-03-08T21:20:12Z

Converting the specified value of some properties into a computed value depends on the value of other properties. For example, the em unit of any length depends on the font-size property.

Previously, we would do a first pass over applicable declarations to build up a values::computed::Context struct with a number of fields for each such piece of data from other properties.

This simplies the struct by instead having it contain the set of computed values (for a given element) that is being populated and classify properties into "early" and "other", such that the only dependencies can be from "other" to "early". We iterate applicable_declarations twice, first cascading "early" properties then "other". Unfortunately, it’s not easy to check that this classification is correct.

highfive · 2016-03-08T21:20:15Z

Heads up! This PR modifies the following files:

@bholley: components/style/values.rs, components/style/properties.mako.rs, components/style/viewport.rs

SimonSapin · 2016-03-08T21:20:20Z

@bors-servo try

bors-servo · 2016-03-08T21:20:23Z

⌛ Trying commit 75382bb with merge 586f463...


        Auto merge of #9929 - servo:cascade, r=<try>

Try I’ll edit this PR with more stuff later, but for now I want some try results.

bors-servo · 2016-03-08T22:13:40Z

💔 Test failed - linux-rel

SimonSapin · 2016-03-08T23:37:20Z

@bholley Alright, the last commit has the refactoring discussed today or IRC, but I’m really not confident that it’s correct or that our current test suite covers edge cases where is might not be correct.

Still, let’s see what happens with:

@bors-servo try

bors-servo · 2016-03-08T23:37:22Z

⌛ Trying commit f23ed92 with merge 24115a9...


        Auto merge of #9929 - servo:cascade, r=<try>

Try I’ll edit this PR with more stuff later, but for now I want some try results.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9929)

bors-servo · 2016-03-09T00:22:06Z

💔 Test failed - mac-rel-wpt

pcwalton · 2016-03-10T21:56:24Z

components/style/properties.mako.rs

+        // Like calling to_computed_value, which wouldn't type check.
+        if style.border.border_${side}_style.none_or_hidden() &&
+           style.border.border_${side}_width != Au(0) {
+            Arc::make_mut(&mut style.border).border_${side}_width = Au(0);


Pushing the Arc::make_mut() into the if condition like you did here may improve cascading performance significantly, FWIW. Thanks!

Yes, that was the idea.

SimonSapin · 2016-03-10T22:02:37Z

r? @pcwalton

There’s at least one bug causing these two failures, but I don’t understand why these tests produce the results they do with this PR :(

  ▶ FAIL [expected PASS] /_mozilla/css/inline_absolute_hypothetical_clip_a.html
  ▶ FAIL [expected PASS] /_mozilla/css/inline_hypothetical_box_a.html

pcwalton · 2016-03-11T23:35:12Z

@bors-servo: r+

Nice idea and any simplification here is helpful, since this is performance-critical code and the i-cache burden is significant.

bors-servo · 2016-03-11T23:35:13Z

📌 Commit f1c46c2 has been approved by pcwalton

pcwalton · 2016-03-11T23:35:33Z

@bors-servo: r-

Tests are failing. But r=me when they're fixed.


        Fix the rem unit.

(I think it’s never worked in over a year since it was implemented…)


        Fix up overflow_x and _y after the fact rather than have their values…

… in computed::Context.

bors-servo · 2016-03-18T19:30:08Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

jdm · 2016-03-18T19:53:43Z

@SimonSapin: #9012 - nothing new about it.

SimonSapin · 2016-03-18T22:46:15Z

Ok, I’ll remember to try more variations of github search.

@pcwalton try passed, please r? the bug fixes, I’m not super confident in them.

pcwalton · 2016-03-18T23:00:50Z

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

components/style/properties.mako.rs, line 6681 [r5] (raw file):
Does it make sense to only call Arc::make_mut() if we're actually going to make a change? Arc::make_mut() is always high in the profile.

Comments from the review on Reviewable.io

SimonSapin · 2016-03-18T23:02:42Z

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

components/style/properties.mako.rs, line 6681 [r5] (raw file):
Yes, we only go inside if let Some(… if we’re going to make a change. The match above is None for no change.

Comments from the review on Reviewable.io

pcwalton · 2016-03-18T23:03:32Z

Looks good, r=me.

SimonSapin · 2016-03-18T23:03:55Z

@bors-servo r=pcwalton

bors-servo · 2016-03-18T23:03:56Z

📌 Commit 529164e has been approved by pcwalton

bors-servo · 2016-03-18T23:03:58Z

⌛ Testing commit 529164e with merge 1ade640...


        Auto merge of #9929 - servo:cascade, r=pcwalton

Refactor the cascade Converting the specified value of some properties into a computed value depends on the value of other properties. For example, the `em` unit of any length depends on the `font-size` property. Previously, we would do a first pass over applicable declarations to build up a `values::computed::Context` struct with a number of fields for each such piece of data from other properties. This simplies the struct by instead having it contain the set of computed values (for a given element) that is being populated and classify properties into "early" and "other", such that the only dependencies can be from "other" to "early". We iterate applicable_declarations twice, first cascading "early" properties then "other". Unfortunately, it’s not easy to check that this classification is correct.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9929)

bors-servo · 2016-03-18T23:45:15Z

💔 Test failed - linux-rel

bholley · 2016-03-18T23:56:22Z

@bors-servo retry

mbrubeck · 2016-03-19T00:05:41Z

The failed test was #9724.

bors-servo · 2016-03-19T01:22:22Z

⚡ Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor are reusable. Rebuilding only linux-rel...

bors-servo · 2016-03-19T02:10:50Z

💔 Test failed - linux-rel

KiChjang · 2016-03-19T02:38:54Z

@bors-servo retry

#9818

bors-servo · 2016-03-19T02:38:56Z

⌛ Testing commit 529164e with merge 2d6283c...


        Auto merge of #9929 - servo:cascade, r=pcwalton

Refactor the cascade Converting the specified value of some properties into a computed value depends on the value of other properties. For example, the `em` unit of any length depends on the `font-size` property. Previously, we would do a first pass over applicable declarations to build up a `values::computed::Context` struct with a number of fields for each such piece of data from other properties. This simplies the struct by instead having it contain the set of computed values (for a given element) that is being populated and classify properties into "early" and "other", such that the only dependencies can be from "other" to "early". We iterate applicable_declarations twice, first cascading "early" properties then "other". Unfortunately, it’s not easy to check that this classification is correct.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9929)

bors-servo · 2016-03-19T03:27:51Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

bholley · 2016-03-22T02:12:24Z

components/style/properties.mako.rs

                                        .${property.ident} = computed_value;
                                % endif

                                % if property.name in DERIVED_LONGHANDS:
-                                    % if not style_struct.inherited:


@SimonSapin what happened to this bit?

It’s not needed since derive_from_* functions don’t take a computed_value parameter anymore. Computed values (as known "so far") are already available through e.g. context.style.box_.display, whereas context.display was previously the specified value, so when the computed value was needed it had to be passed separately.

I was talking about the |if not style_struct.inherited| bit.

Yes. In this function non-inherited styles are cached so we don’t need to run the cascade for them again. let computed_value = …; is in a larger block above is in % if style_struct.inherited: (without not).

Before this PR, computed_value was needed unconditionally, so this % if not style_struct.inherited: block (with not) defined it for the rest of the cases. Now that computed_value is not used in the derive_from_* call anymore, this second let computed_value = …; statement would be dead code.

highfive assigned jdm Mar 8, 2016

highfive added the S-awaiting-review label Mar 8, 2016

bors-servo added a commit that referenced this pull request Mar 8, 2016

Auto merge of #9929 - servo:cascade, r=<try>

Loading status checks…

586f463

Try I’ll edit this PR with more stuff later, but for now I want some try results.

SimonSapin assigned SimonSapin and unassigned jdm Mar 8, 2016

highfive added S-tests-failed and removed S-tests-failed labels Mar 8, 2016

highfive added the S-tests-failed label Mar 9, 2016

SimonSapin changed the title ~~Try~~ Refactor the cascade Mar 10, 2016

pcwalton reviewed Mar 10, 2016
View changes

SimonSapin force-pushed the cascade branch from f23ed92 to f1c46c2 Mar 10, 2016

highfive removed the S-tests-failed label Mar 10, 2016

highfive assigned pcwalton and unassigned SimonSapin Mar 10, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 11, 2016

SimonSapin added 2 commits Mar 8, 2016

Fix the rem unit.

54790d0

(I think it’s never worked in over a year since it was implemented…)

Fix up overflow_x and _y after the fact rather than have their values…

bab28e5

… in computed::Context.

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Mar 18, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 18, 2016

highfive added the S-tests-failed label Mar 19, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Mar 19, 2016

bors-servo merged commit 529164e into master Mar 19, 2016
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

SimonSapin deleted the cascade branch Mar 19, 2016

bholley reviewed Mar 22, 2016
View changes

servo / servo

Refactor the cascade #9929

Refactor the cascade #9929

SimonSapin commented Mar 8, 2016

highfive commented Mar 8, 2016

SimonSapin commented Mar 8, 2016

bors-servo commented Mar 8, 2016

bors-servo commented Mar 8, 2016

SimonSapin commented Mar 8, 2016

bors-servo commented Mar 8, 2016

bors-servo commented Mar 9, 2016

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Mar 10, 2016

pcwalton commented Mar 11, 2016

bors-servo commented Mar 11, 2016

pcwalton commented Mar 11, 2016

bors-servo commented Mar 18, 2016

jdm commented Mar 18, 2016

SimonSapin commented Mar 18, 2016

pcwalton commented Mar 18, 2016

SimonSapin commented Mar 18, 2016

pcwalton commented Mar 18, 2016

SimonSapin commented Mar 18, 2016

bors-servo commented Mar 18, 2016

bors-servo commented Mar 18, 2016

bors-servo commented Mar 18, 2016

bholley commented Mar 18, 2016

mbrubeck commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

KiChjang commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

servo / servo

Join GitHub today

Refactor the cascade #9929

Refactor the cascade #9929

Conversation

SimonSapin commented Mar 8, 2016

highfive commented Mar 8, 2016

SimonSapin commented Mar 8, 2016

bors-servo commented Mar 8, 2016

bors-servo commented Mar 8, 2016

SimonSapin commented Mar 8, 2016

bors-servo commented Mar 8, 2016

bors-servo commented Mar 9, 2016

This comment has been minimized.

pcwalton Mar 10, 2016

This comment has been minimized.

SimonSapin Mar 10, 2016

SimonSapin commented Mar 10, 2016

pcwalton commented Mar 11, 2016

bors-servo commented Mar 11, 2016

pcwalton commented Mar 11, 2016

bors-servo commented Mar 18, 2016

jdm commented Mar 18, 2016

SimonSapin commented Mar 18, 2016

pcwalton commented Mar 18, 2016

SimonSapin commented Mar 18, 2016

pcwalton commented Mar 18, 2016

SimonSapin commented Mar 18, 2016

bors-servo commented Mar 18, 2016

bors-servo commented Mar 18, 2016

bors-servo commented Mar 18, 2016

bholley commented Mar 18, 2016

mbrubeck commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

KiChjang commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

This comment has been minimized.

bholley Mar 22, 2016

This comment has been minimized.

SimonSapin Mar 22, 2016

This comment has been minimized.

bholley Mar 22, 2016

This comment has been minimized.

SimonSapin Mar 23, 2016