style: Add a simple rule-tree benchmarks. #14485

emilio · 2016-12-07T21:05:12Z

Numbers on my machine, for the record:

test rule_tree::bench::bench_expensive_insersion          ... bench:   7,211,081 ns/iter (+/- 1,933,866)
test rule_tree::bench::bench_expensive_insersion_parallel ... bench:  78,728,097 ns/iter (+/- 11,738,010)
test rule_tree::bench::bench_insertion_basic              ... bench:     665,333 ns/iter (+/- 68,089)
test rule_tree::bench::bench_insertion_basic_parallel     ... bench:   1,587,203 ns/iter (+/- 372,124)

This change is

frewsxcv · 2016-12-08T00:31:05Z

Looks like there are a few tidy issues.

emilio · 2016-12-08T01:41:28Z

Fixed, also matched the two workloads so the test is more meaningful, and fixed a race condition in the node-dropping code.

emilio · 2016-12-08T01:41:56Z

test rule_tree::bench::bench_expensive_insersion          ... bench: 125,770,757 ns/iter (+/- 4,556,363)
test rule_tree::bench::bench_expensive_insersion_parallel ... bench:  76,942,732 ns/iter (+/- 12,462,937)
test rule_tree::bench::bench_insertion_basic              ... bench:   1,581,405 ns/iter (+/- 92,483)
test rule_tree::bench::bench_insertion_basic_parallel     ... bench:     938,729 ns/iter (+/- 62,497)
test rule_tree::bench::bench_insertion_basic_per_element  ... bench:         359 ns/iter (+/- 22)

bors-servo · 2016-12-10T10:25:17Z

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

emilio · 2016-12-10T19:55:35Z

rebased. ping @heycam :)

bors-servo · 2016-12-15T01:42:35Z

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

emilio · 2016-12-15T09:24:01Z

@heycam review ping?

heycam

I don't really understand why we need these changes to pop_from_free_list. Is it that we are indeed calling this from multiple threads at once, when we shouldn't be?

Can you also explain those changes to me as if I were a person who didn't understand the old code? :-)

Can we add some some more explicit assertions / debug-only locks that prevent us from calling into pop_from_free_list simultaneously? I guess from your comments that storing null in the free list head is a way to do this, but it would be nice if we could have something more obvious/explicit.

Also it would have been good to split out any substantive rule tree changes here from commits that add/change benchmarking.

heycam · 2016-12-16T10:10:23Z

tests/unit/style/rule_tree/bench.rs

+    let r = RuleTree::new();
+
+    b.iter(|| {
+        let rules_matched = parse_rules(


Do we want this outside of the |iter| call? Although it might be small compared to the 1000 test_insertion calls, I think we want to limit the measurement to the insertion, not parsing.

yup, that's right.

heycam · 2016-12-16T10:10:23Z

tests/unit/style/rule_tree/bench.rs

+        // This test case tests a case where you style a bunch of siblings
+        // matching the same rules, with a different style attribute each
+        // one.
+        let rules_matched = parse_rules(


Same here (and in the other functions). (I guess technically we would want the 1000 style attribute rule creations to happen outside of iter too, but that looks cheaper than the CSS parsing.)

heycam · 2016-12-16T10:10:23Z

tests/unit/style/rule_tree/bench.rs

+}
+
+#[bench]
+fn bench_expensive_insersion(b: &mut Bencher) {


heycam · 2016-12-16T10:10:23Z

tests/unit/style/rule_tree/bench.rs

+                s.spawn(|s| {
+                    for _ in 0..1000 {
+                        test::black_box(test_insertion(&r,
+                                                       rules_matched.clone()));


Can we make test_insertion take the Vec of rules by reference? Then we can avoid measuring the cost of cloning the Vec.

Sort of, we need to clone for the style attribute, so seemed a bit more fair this way.

heycam · 2016-12-16T10:10:23Z

tests/unit/style/rule_tree/bench.rs

+}
+
+#[bench]
+fn bench_expensive_insersion_parallel(b: &mut Bencher) {


heycam · 2016-12-16T10:10:23Z

components/style/rule_tree/mod.rs

-                      (thread_state::get().is_layout() ||
-                       thread_state::get().is_script()));
-        let current = me.next_free.load(Ordering::SeqCst);
+        if !cfg!(feature = "testing") {


Maybe just move the cfg!() expression into the debug_assert!() expression?

heycam · 2016-12-16T10:10:23Z

components/style/rule_tree/mod.rs

+        assert!(me.is_root());
+
+        let mut current = self.ptr;
+        let mut seen = vec![];


Can we use a HashSet instead? Otherwise we take quadratic time running these checks.

They're debug-only, so I didn't thought too much about it, but yeah, that's fine.

heycam · 2016-12-16T10:10:23Z

tests/unit/style/rule_tree/bench.rs

@@ -73,13 +73,29 @@ fn bench_insertion_basic(b: &mut Bencher) {
             .bar { height: 500px; } \
             .baz { display: block; }");

-        for _ in 0..1000 {
+        for _ in 0..(4000 + 400) {


Does 4000 + 400 have some significance?

The same workload as the others.

heycam · 2016-12-16T10:10:23Z

tests/unit/style/rule_tree/bench.rs

+    b.iter(|| {
+        test::black_box(test_insertion(&r, rules_matched.clone()));
+
+        unsafe { r.gc() };


Might be worth wrapping the RuleTree object into a struct so that you can call its gc() in a Drop impl, to save repeating this in each test.

Fine for me.

emilio · 2016-12-16T10:32:12Z

Let me try to elaborate on the drop changes.

It's not that we're calling pop_from_free_list from multiple threads, it's that we're dropping the same node twice from multiple threads. Most of the debug checks are during GC because I wrote them when I was investigating.

What we did was something like:

check the free list pointer. return if we were already in the free list.
Try to insert us in the free list.

But of course, something that doesn't happen while styling but happens during these benchmarks, is that that same node can be inserted in the free list between 1 and 2 (if it's "resurrected" and then dropped again).

The way I've solved this is setting the free list pointer to null while we're modifying it.

heycam · 2016-12-19T01:20:36Z

But of course, something that doesn't happen while styling but happens during these benchmarks, is that that same node can be inserted in the free list between 1 and 2 (if it's "resurrected" and then dropped again).

Ah, and the reason this can't happen during styling is that we'll never restyle the same element twice (which would otherwise possibly result in releasing then resurrecting a rule node).

heycam

r=me with the comments addressed as you see fit.

heycam · 2016-12-19T01:52:47Z

components/style/rule_tree/mod.rs

            }
        }
+
+        // If we've raced with the same node resurrected and dropped it, we're
+        // done, just store the stuff again and.


heycam · 2016-12-19T01:52:47Z

components/style/rule_tree/mod.rs

+        }
+
+        // Else store the old head as the next pointer, and store ourselves as
+        // the old head.


as the new head?

emilio · 2016-12-19T08:23:10Z

@bors-servo r=heycam

bors-servo · 2016-12-19T08:23:11Z

📌 Commit 3f5a486 has been approved by heycam

bors-servo · 2016-12-19T08:23:16Z

⌛ Testing commit 3f5a486 with merge bc9ebd8...


        Auto merge of #14485 - servo:rule-tree-bench, r=heycam

@heycam

style: Add a simple rule-tree benchmarks.  r? @heycam Numbers on my machine, for the record: ``` test rule_tree::bench::bench_expensive_insersion ... bench: 7,211,081 ns/iter (+/- 1,933,866) test rule_tree::bench::bench_expensive_insersion_parallel ... bench: 78,728,097 ns/iter (+/- 11,738,010) test rule_tree::bench::bench_insertion_basic ... bench: 665,333 ns/iter (+/- 68,089) test rule_tree::bench::bench_insertion_basic_parallel ... bench: 1,587,203 ns/iter (+/- 372,124) ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14485)

bors-servo · 2016-12-19T08:42:09Z

💔 Test failed - linux-dev


        style: Add simple rule-tree benchmarks. Fix rule node drop race.


        style: Use a HashSet in free-list check.

emilio · 2016-12-19T08:47:31Z

@bors-servo r=heycam

My import PR broke this one, heh.

bors-servo · 2016-12-19T08:47:32Z

📌 Commit fc6bed2 has been approved by heycam

bors-servo · 2016-12-19T08:47:36Z

⌛ Testing commit fc6bed2 with merge cb6a870...


        Auto merge of #14485 - servo:rule-tree-bench, r=heycam

@heycam

style: Add a simple rule-tree benchmarks.  r? @heycam Numbers on my machine, for the record: ``` test rule_tree::bench::bench_expensive_insersion ... bench: 7,211,081 ns/iter (+/- 1,933,866) test rule_tree::bench::bench_expensive_insersion_parallel ... bench: 78,728,097 ns/iter (+/- 11,738,010) test rule_tree::bench::bench_insertion_basic ... bench: 665,333 ns/iter (+/- 68,089) test rule_tree::bench::bench_insertion_basic_parallel ... bench: 1,587,203 ns/iter (+/- 372,124) ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14485)

bors-servo · 2016-12-19T10:04:35Z

💔 Test failed - mac-rel-wpt1

emilio · 2016-12-19T10:16:00Z

@bors-servo retry #12797

bors-servo · 2016-12-19T10:16:03Z

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

bors-servo · 2016-12-19T10:50:06Z

☀️ 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-dev

highfive assigned heycam Dec 7, 2016

highfive added the S-awaiting-review label Dec 7, 2016

frewsxcv added S-fails-tidy and removed S-awaiting-review labels Dec 8, 2016

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

emilio force-pushed the rule-tree-bench branch from 296e07a to a49c6a1 Dec 8, 2016

emilio removed the S-fails-tidy label Dec 8, 2016

highfive added the S-needs-rebase label Dec 10, 2016

emilio force-pushed the rule-tree-bench branch from a49c6a1 to 37ec4d8 Dec 10, 2016

highfive added the S-needs-rebase label Dec 15, 2016

emilio force-pushed the rule-tree-bench branch from 37ec4d8 to 0fca512 Dec 15, 2016

heycam reviewed Dec 16, 2016

View changes

emilio force-pushed the rule-tree-bench branch from 0fca512 to 8c3c7b8 Dec 16, 2016

heycam approved these changes Dec 19, 2016

View changes

emilio force-pushed the rule-tree-bench branch 2 times, most recently from 3e97e8f to 3f5a486 Dec 19, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Dec 19, 2016

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

emilio added 2 commits Dec 7, 2016

style: Add simple rule-tree benchmarks. Fix rule node drop race.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

e67ea42

style: Use a HashSet in free-list check.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

fc6bed2

emilio force-pushed the rule-tree-bench branch from 388ad3c to fc6bed2 Dec 19, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 19, 2016

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

SimonSapin deleted the rule-tree-bench branch Dec 19, 2016

servo / servo

Join GitHub today

style: Add a simple rule-tree benchmarks. #14485

style: Add a simple rule-tree benchmarks. #14485

Conversation

emilio commented Dec 7, 2016 • edited by larsbergstrom

frewsxcv commented Dec 8, 2016

emilio commented Dec 8, 2016

emilio commented Dec 8, 2016

bors-servo commented Dec 10, 2016

emilio commented Dec 10, 2016

bors-servo commented Dec 15, 2016

emilio commented Dec 15, 2016

heycam 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.

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.

emilio commented Dec 16, 2016 • edited

heycam commented Dec 19, 2016

heycam left a comment

This comment has been minimized.

This comment has been minimized.

emilio commented Dec 19, 2016

bors-servo commented Dec 19, 2016

bors-servo commented Dec 19, 2016

bors-servo commented Dec 19, 2016

emilio commented Dec 19, 2016

bors-servo commented Dec 19, 2016

bors-servo commented Dec 19, 2016

bors-servo commented Dec 19, 2016

emilio commented Dec 19, 2016

bors-servo commented Dec 19, 2016

bors-servo commented Dec 19, 2016

emilio commented Dec 7, 2016 •

edited by larsbergstrom

emilio commented Dec 16, 2016 •

edited