style: Don't loop over all the set of dependencies always. #13108

emilio · 2016-08-29T19:05:28Z

Instead, divide which kind of dependencies could match a mutation. This cuts down incremental restyle time in BrowserHTML quite a bit.

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes OR

The dependency count is not at all minor, and this way we avoid looping
through all of them in the common cases, mainly either changing state, or
attributes.

This change is

highfive · 2016-08-29T19:05:32Z

Heads up! This PR modifies the following files:

@bholley: components/style/restyle_hints.rs

highfive · 2016-08-29T19:05:33Z

Warning

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

emilio · 2016-08-29T19:05:43Z

r? @SimonSapin or @bholley

emilio · 2016-08-29T19:05:51Z

@bors-servo: try

bors-servo · 2016-08-29T19:05:54Z

⌛ Trying commit e65c3ea with merge 5687523...


        Auto merge of #13108 - emilio:stupidest-opt-ever, r=<try>

style: Don't loop over all the set of dependencies always.  Instead, divide which kind of dependencies could match a mutation. This cuts down incremental restyle time in BrowserHTML quite a bit. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes OR  The dependency count is not at all minor, and this way we avoid looping through all of them in the common cases, mainly either changing state, or attributes.

bholley · 2016-08-29T19:18:26Z

components/style/restyle_hints.rs

@@ -341,16 +341,38 @@ struct Dependency {
 #[derive(Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub struct DependencySet {


Please add a comment above this struct that there are measurable performances wins from storing these separately.

bholley · 2016-08-29T19:24:23Z

components/style/restyle_hints.rs

+            }
+        }
+
+        if !state_changes.is_empty() {


Rather than doing this in three places here, I think this should be abstracted into a method on DependencySet.

bholley · 2016-08-29T19:25:51Z

components/style/restyle_hints.rs

    }
 }

 impl DependencySet {
+    fn test_dependency<E>(&self,


This doesn't really make sense as a Dep on DependencySet. Can we impl it as a method on Vec, or does rust not let us do that? If not, I guess best would be to make it a non-method helper function.

bholley · 2016-08-29T19:26:06Z

Nice find! Looks good in general, though I'd like to look at the result of the refactor.

bors-servo · 2016-08-29T20:02:54Z

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

bholley · 2016-08-29T21:13:33Z

components/style/restyle_hints.rs

@@ -330,27 +337,52 @@ impl Sensitivities {
 /// This allows us to quickly scan through the dependency sites of all style
 /// rules and determine the maximum effect that a given state or attribute
 /// change may have on the style of elements in the document.
+///
+/// Note that there are measurable perf wins from storing them separately, and


I don't think the meaning of "storing them separately" is clear in this context.

Would probably be better to move this into the comment on DependencySet, and say "storing attribute and state dependencies separately".

bholley · 2016-08-29T21:22:34Z

components/style/restyle_hints.rs

+    }
+}
+
+fn test_dependencies<E>(deps: &[Dependency],


Hm, the free-floating function is still kind of annoying. Could we make a newtype:

struct DependencyList(Vec)

Inside DependencySet and then impl that?

Also, given that we're now storing restyle hints instead of combinators, maybe "test" isn't quite the right word anymore for the operations on DependencyList. Maybe we should call it compute_partial_hint or collect_hints?

bholley · 2016-08-29T22:56:59Z

components/style/restyle_hints.rs

                }
            }
+            if hint.is_all() {


Nit: Seems like this should go inside the branch where we actually modify hint, right?

Whoops, good catch :-)

bholley · 2016-08-29T22:58:36Z

components/style/restyle_hints.rs

+                               attrs_changed: bool,
+                               hint: &mut RestyleHint)
+        where E: ElementExt
+    {


It seems cleaner to check hint.is_all() here and early-return, and then not check it in the callers.

bholley · 2016-08-29T22:59:27Z

r=me with those fixes.

@bors-servo delegate+

bors-servo · 2016-08-29T22:59:28Z

✌️ @emilio can now approve this pull request


        style: Don't loop over all the set of dependencies always.

The dependency count is not at all minor, and this way we avoid looping through all of them in the common cases, mainly either changing state, or attributes.

emilio · 2016-08-29T23:03:33Z

@bors-servo: r=bholley

bors-servo · 2016-08-29T23:03:33Z

📌 Commit 3e80f48 has been approved by bholley

bors-servo · 2016-08-29T23:58:53Z

⌛ Testing commit 3e80f48 with merge 6dfb5f0...


        Auto merge of #13108 - emilio:stupidest-opt-ever, r=bholley

style: Don't loop over all the set of dependencies always.  Instead, divide which kind of dependencies could match a mutation. This cuts down incremental restyle time in BrowserHTML quite a bit. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes OR  The dependency count is not at all minor, and this way we avoid looping through all of them in the common cases, mainly either changing state, or attributes.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13108)

bors-servo · 2016-08-30T00:55:22Z

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


        Fix debug assertion introduced in servo#13108 by me.

I'm stupid, and when I did the last moving-the-code-around, I failed miserably to double-check it in a debug build.


        Auto merge of #13128 - emilio:stupidest-assertion-ever, r=SimonSapin

Fix debug assertion introduced in #13108 by me.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13125 (github issue number if applicable).  - [x] These changes do not require tests because #13127  I'm stupid, and when I did the last moving-the-code-around, I failed miserably to double-check it in a debug build.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13128)


        Fix debug assertion introduced in servo#13108 by me.

I'm stupid, and when I did the last moving-the-code-around, I failed miserably to double-check it in a debug build.

highfive assigned KiChjang Aug 29, 2016

highfive added the S-awaiting-review label Aug 29, 2016

highfive assigned SimonSapin and unassigned KiChjang Aug 29, 2016

bholley reviewed Aug 29, 2016
View changes

components/style/restyle_hints.rs Outdated

}

}

if !state_changes.is_empty() {

This comment has been minimized.

Sign in to view

bholley Aug 29, 2016
Contributor

Rather than doing this in three places here, I think this should be abstracted into a method on DependencySet.

bholley reviewed Aug 29, 2016
View changes

emilio force-pushed the emilio:stupidest-opt-ever branch from 811a3e4 to a02fcbd Aug 29, 2016

bholley reviewed Aug 29, 2016
View changes

components/style/restyle_hints.rs

attrs_changed: bool,

hint: &mut RestyleHint)

where E: ElementExt

{

This comment has been minimized.

Sign in to view

bholley Aug 29, 2016
Contributor

It seems cleaner to check hint.is_all() here and early-return, and then not check it in the callers.

emilio force-pushed the emilio:stupidest-opt-ever branch from a02fcbd to 3e80f48 Aug 29, 2016

highfive assigned bholley and unassigned SimonSapin Aug 29, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 29, 2016

bors-servo merged commit 3e80f48 into servo:master Aug 30, 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 Aug 30, 2016

samuknet added a commit to samuknet/servo that referenced this pull request Sep 6, 2016

Fix debug assertion introduced in servo#13108 by me.

34e24b2

I'm stupid, and when I did the last moving-the-code-around, I failed miserably to double-check it in a debug build.

emilio deleted the emilio:stupidest-opt-ever branch Apr 12, 2017

servo / servo

style: Don't loop over all the set of dependencies always. #13108

style: Don't loop over all the set of dependencies always. #13108

emilio commented Aug 29, 2016 •

edited by larsbergstrom

highfive commented Aug 29, 2016

highfive commented Aug 29, 2016

emilio commented Aug 29, 2016

emilio commented Aug 29, 2016

bors-servo commented Aug 29, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Aug 29, 2016

bors-servo commented Aug 29, 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.

bholley commented Aug 29, 2016

bors-servo commented Aug 29, 2016

emilio commented Aug 29, 2016

bors-servo commented Aug 29, 2016

bors-servo commented Aug 29, 2016

bors-servo commented Aug 30, 2016

servo / servo

Join GitHub today

style: Don't loop over all the set of dependencies always. #13108

style: Don't loop over all the set of dependencies always. #13108

Conversation

emilio commented Aug 29, 2016 • edited by larsbergstrom

highfive commented Aug 29, 2016

highfive commented Aug 29, 2016

emilio commented Aug 29, 2016

emilio commented Aug 29, 2016

bors-servo commented Aug 29, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Aug 29, 2016

bors-servo commented Aug 29, 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.

bholley commented Aug 29, 2016

bors-servo commented Aug 29, 2016

emilio commented Aug 29, 2016

bors-servo commented Aug 29, 2016

bors-servo commented Aug 29, 2016

bors-servo commented Aug 30, 2016

emilio commented Aug 29, 2016 •

edited by larsbergstrom