Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change #11890

shinglyu · 2016-06-28T06:22:24Z

Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10104 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-06-28T06:22:27Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/outline.mako.rs, components/style/properties/longhand/list.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/data.py, components/style/traversal.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/pointing.mako.rs, components/style/properties/longhand/inherited_box.mako.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/longhand/effects.mako.rs, components/style/values.rs, components/style/properties/longhand/counters.mako.rs, components/style/dom.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/inherited_table.mako.rs, components/style/properties/longhand/column.mako.rs
@KiChjang: components/script/dom/node.rs, components/script/layout_wrapper.rs

highfive · 2016-06-28T06:22:28Z

Warning

These commits modify unsafe code. Please review it carefully!

shinglyu · 2016-06-28T06:23:12Z

r? @mbrubeck

shinglyu · 2016-06-28T06:40:56Z

Profiling shows that the restyle events dropped from

    _incremental?_  _iframe?_   _url_   _mean (ms)_ _median (ms)_   _min (ms)_  _max (ms)_  _events_
Compositing      N/A      N/A                N/A    7.4163  0.1395  0.0003  53.598  97
Layout   yes    yes http://localhost:8000/mozilla.html  40.7831 0.1542  0.0282  1242.3715   76
Layout   no yes http://localhost:8000/mozilla.html  2.7127  2.7127  2.7127  2.7127  1
+ Style Recalc   yes    yes http://localhost:8000/mozilla.html  14.7199 0.0465  0.0205  620.1162    97
+ Style Recalc   no yes http://localhost:8000/mozilla.html  1.3714  1.3714  1.3714  1.3714  1

to

_category_  _incremental?_  _iframe?_   _url_   _mean (ms)_ _median (ms)_   _min (ms)_  _max (ms)_  _events_
Compositing      N/A      N/A                N/A    4.3865  0.0998  0.0003  35.7685 33
Layout   yes    yes http://localhost:8000/mozilla.html  148.2419    0.1642  0.0456  1173.1  22
Layout   no yes http://localhost:8000/mozilla.html  2.9211  2.9211  2.9211  2.9211  1
+ Style Recalc   yes    yes http://localhost:8000/mozilla.html  46.404  0.0288  0.0259  725.4902    29
+ Style Recalc   no yes http://localhost:8000/mozilla.html  1.628   1.628   1.628   1.628   1

The HTML waterfall chart also showed improved restyle speed.

shinglyu · 2016-06-28T06:52:11Z

components/style/properties/properties.mako.rs

+    fn has_viewport_percentage(&self) -> bool {
+        match *self {
+            % for property in data.longhands:
+                PropertyDeclaration::${property.camel_case}(DeclaredValue::Value(ref val)) => val.has_viewport_percentage(),


This line is too long, but wrapping this will make the generated code ugly. Suggestions welcome.

shinglyu · 2016-06-28T06:56:56Z

Here are the profiler HTML file and tsv dump:
profiler_output.zip

bors-servo · 2016-06-28T23:24:43Z

☔ The latest upstream changes (presumably #11766) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-07-03T04:03:04Z

☔ The latest upstream changes (presumably #11972) made this pull request unmergeable. Please resolve the merge conflicts.

shinglyu · 2016-07-06T08:48:10Z

ping @mbrubeck

mbrubeck · 2016-07-06T18:54:19Z

I've started reviewing this and should have a complete review ready soon.

mbrubeck · 2016-07-08T22:21:56Z

@bors-servo delegate+

This looks good! Can land with r=mbrubeck after rebasing and addressing the minor whitespace/comment issues below.

Reviewed 1 of 1 files at r1, 1 of 3 files at r7, 5 of 14 files at r9, 21 of 21 files at r10.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/layout_thread/lib.rs, line 1073 [r10] (raw file):

                       .unwrap();
            }
            // FIXME (#10104): Only dirty nodes affected by vh/vw/vmin/vmax styles.

You can remove this comment.

components/layout_thread/lib.rs, line 1078 [r10] (raw file):

                    if node.needs_dirty_on_viewport_size_changed() {
                        node.dirty_self();
                        node.dirty_descendants();

It would be nice if we could avoid traversing the node's descendants after they are already dirtied. Maybe this could be a follow-up enhancement.

components/style/properties/properties.mako.rs, line 830 [r9] (raw file):

Previously, shinglyu (Shing Lyu) wrote…

This line is too long, but wrapping this will make the generated code ugly. Suggestions welcome.

I'd just wrap it, and don't worry much about the generated code ugliness.

Comments from Reviewable

bors-servo · 2016-07-08T22:21:58Z

✌️ @shinglyu can now approve this pull request

shinglyu · 2016-07-11T03:12:36Z

I'll land it first and make the skipping traversal part a followup, because this part of the code get bitrotted too fast.

shinglyu · 2016-07-11T03:13:52Z

@bors-servo r=mbrubeck

bors-servo · 2016-07-11T03:13:53Z

📌 Commit 720cf27 has been approved by mbrubeck

shinglyu · 2016-07-13T04:53:26Z

@mbrubeck I've updated the patch to support Stylo. One thing worth mentioning is that Gecko doesn't has the flag I just added, so I fallback to a full restyle when it is used in Stylo. Do you want to check again? Or I can just merge?

shinglyu · 2016-07-15T02:50:18Z

@bors-servo r=mbrubeck

bors-servo · 2016-07-15T02:50:19Z

📌 Commit 5452bba has been approved by mbrubeck

bors-servo · 2016-07-15T02:50:21Z

⌛ Testing commit 5452bba with merge d45080e...


        Auto merge of #11890 - shinglyu:viewport-percent-recalc, r=mbrubeck

Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change  Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10104 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11890)

bors-servo · 2016-07-15T03:00:51Z

💔 Test failed - linux-dev

bors-servo · 2016-07-16T06:29:27Z

☔ The latest upstream changes (presumably #12458) made this pull request unmergeable. Please resolve the merge conflicts.


        Only restyle viewport-relative nodes on viewport size change

shinglyu · 2016-07-18T03:12:36Z

Implement the HasViewportPercentage trait for vector_longhand helper

shinglyu · 2016-07-18T03:12:47Z

@bors-servo r=mbrubeck

bors-servo · 2016-07-18T03:12:48Z

📌 Commit f754cac has been approved by mbrubeck

bors-servo · 2016-07-18T03:46:25Z

⌛ Testing commit f754cac with merge d87ea67...


        Auto merge of #11890 - shinglyu:viewport-percent-recalc, r=mbrubeck

Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change  Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10104 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11890)

bors-servo · 2016-07-18T04:49:14Z

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

highfive assigned Ms2ger Jun 28, 2016

highfive added the S-awaiting-review label Jun 28, 2016

highfive assigned mbrubeck and unassigned Ms2ger Jun 28, 2016

shinglyu reviewed Jun 28, 2016
View changes

highfive added the S-needs-rebase label Jun 28, 2016

shinglyu force-pushed the shinglyu:viewport-percent-recalc branch from 07426f8 to 672dcf1 Jul 1, 2016

highfive added the S-needs-rebase label Jul 3, 2016

mbrubeck added S-needs-code-changes and removed S-awaiting-review labels Jul 8, 2016

shinglyu force-pushed the shinglyu:viewport-percent-recalc branch from 672dcf1 to 720cf27 Jul 11, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 11, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jul 11, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jul 13, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 15, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 15, 2016

highfive added the S-needs-rebase label Jul 16, 2016

Only restyle viewport-relative nodes on viewport size change

Loading status checks…

f754cac

shinglyu force-pushed the shinglyu:viewport-percent-recalc branch from 5452bba to f754cac Jul 18, 2016

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

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jul 18, 2016

bors-servo merged commit f754cac into servo:master Jul 18, 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

highfive removed the S-awaiting-merge label Jul 18, 2016

jdm mentioned this pull request Aug 12, 2016

Servo does no reflect window resize on vw & vh #12835

Closed

servo / servo

Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change #11890

Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change #11890

shinglyu commented Jun 28, 2016 •

edited by larsbergstrom

highfive commented Jun 28, 2016

highfive commented Jun 28, 2016

shinglyu commented Jun 28, 2016

shinglyu commented Jun 28, 2016

This comment has been minimized.

shinglyu commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jul 3, 2016

shinglyu commented Jul 6, 2016

mbrubeck commented Jul 6, 2016

mbrubeck commented Jul 8, 2016

bors-servo commented Jul 8, 2016

shinglyu commented Jul 11, 2016

shinglyu commented Jul 11, 2016

bors-servo commented Jul 11, 2016

shinglyu commented Jul 13, 2016

shinglyu commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 15, 2016

bors-servo commented Jul 16, 2016

shinglyu commented Jul 18, 2016

shinglyu commented Jul 18, 2016

bors-servo commented Jul 18, 2016

bors-servo commented Jul 18, 2016

bors-servo commented Jul 18, 2016

servo / servo

Join GitHub today

Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change #11890

Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change #11890

Conversation

shinglyu commented Jun 28, 2016 • edited by larsbergstrom