GC the rule tree only when the free list gets to a certain size. #14256

heycam · 2016-11-17T08:31:08Z

Use a heuristic similar to Gecko's to decide when to GC the rule tree.

r? @emilio

This change is

highfive · 2016-11-17T08:31:12Z

Heads up! This PR modifies the following files:

@bholley: components/style/rule_tree/mod.rs
@emilio: components/style/rule_tree/mod.rs

highfive · 2016-11-17T08:31:14Z

Warning

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

emilio

Looks good to me with that comment added, and probably your patch imported too :)

emilio · 2016-11-17T09:37:48Z

components/style/rule_tree/mod.rs

@@ -533,6 +563,7 @@ impl Drop for StrongRuleNode {
                                           Ordering::SeqCst);
            if existing == next_free {
                // Successfully inserted, yay! Otherwise try again.
+                root.free_count.fetch_add(1, Ordering::SeqCst);


This can be Ordering::Relaxed, given we're gc'ing in the main thread.

And the load and store we do in gc() itself remain Order::SeqCst?

emilio · 2016-11-17T09:37:48Z

components/style/rule_tree/mod.rs

+    /// just used as a heuristic to decide when to run GC.)
+    ///
+    /// Only used on the root RuleNode.
+    free_count: AtomicUsize,


Seems unfortunate to pay this price in every node only for this, probably we should be able to reuse one of the sibling pointers for that, but that's fine as a followup I guess. Could you leave a comment here on that?

emilio · 2016-11-17T09:37:48Z

components/layout_thread/lib.rs

-        // the list, find a good heuristic here for that.
-        unsafe { shared_layout_context.style_context.stylist.rule_tree.gc() }
+        // GC the rule tree if some heuristics are met.
+        unsafe { shared_layout_context.style_context.stylist.rule_tree.maybe_gc(); }


I guess this will need the patch over in https://bugzilla.mozilla.org/show_bug.cgi?id=1318238 to avoid leaking right? Feel free to drop it here :)

Ended up putting it in #14273.

emilio · 2016-11-17T09:37:57Z

@bors-servo try

bors-servo · 2016-11-17T09:38:00Z

⌛ Trying commit ce27483 with merge 2c8bed1...


        Auto merge of #14256 - heycam:rn-gc-heuristic, r=<try>

@emilio

GC the rule tree only when the free list gets to a certain size.  Use a heuristic similar to Gecko's to decide when to GC the rule tree. r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14256)

bors-servo · 2016-11-17T20:03:45Z

💥 Test timed out

heycam · 2016-11-18T03:22:03Z

@bors-servo try


        rule tree: Avoid yet another dumb assertion when dropping the rule tree.


        GC the rule tree only when the free list gets to a certain size.

heycam · 2016-11-18T09:22:38Z

@bors-servo r=emilio

bors-servo · 2016-11-18T09:22:40Z

📌 Commit 4e52bb4 has been approved by emilio

bors-servo · 2016-11-18T23:52:03Z

⌛ Testing commit 4e52bb4 with merge 13c3bd8...


        Auto merge of #14256 - heycam:rn-gc-heuristic, r=emilio

@emilio

GC the rule tree only when the free list gets to a certain size.  Use a heuristic similar to Gecko's to decide when to GC the rule tree. r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14256)

bors-servo · 2016-11-19T00:43:37Z

💔 Test failed - mac-rel-wpt1

emilio · 2016-11-19T00:48:45Z

@bors-servo retry #14267

bors-servo · 2016-11-19T02:46:24Z

⌛ Testing commit 4e52bb4 with merge 1666561...


        Auto merge of #14256 - heycam:rn-gc-heuristic, r=emilio

@emilio

GC the rule tree only when the free list gets to a certain size.  Use a heuristic similar to Gecko's to decide when to GC the rule tree. r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14256)

bors-servo · 2016-11-19T03:30:14Z

💔 Test failed - linux-rel-wpt

KiChjang · 2016-11-19T08:45:56Z

@bors-servo retry

#13776

bors-servo · 2016-11-19T17:02:53Z

⌛ Testing commit 4e52bb4 with merge f6396d4...


        Auto merge of #14256 - heycam:rn-gc-heuristic, r=emilio

@emilio

GC the rule tree only when the free list gets to a certain size.  Use a heuristic similar to Gecko's to decide when to GC the rule tree. r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14256)

bors-servo · 2016-11-19T18:02:52Z

💔 Test failed - linux-rel-wpt

highfive · 2016-11-19T18:02:54Z

  ▶ FAIL [expected PASS] /_mozilla/css/incremental_trailing_whitespace_a.html
  └   → /_mozilla/css/incremental_trailing_whitespace_a.html da54041086f2975f8e3c776d2283ad6609e6862a
/_mozilla/css/incremental_trailing_whitespace_ref.html f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
Testing da54041086f2975f8e3c776d2283ad6609e6862a == f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b

heycam · 2016-11-20T03:41:44Z

@bors-servo retry

#10473

bors-servo · 2016-11-20T04:01:13Z

⌛ Testing commit 4e52bb4 with merge b78622b...


        Auto merge of #14256 - heycam:rn-gc-heuristic, r=emilio

@emilio

GC the rule tree only when the free list gets to a certain size.  Use a heuristic similar to Gecko's to decide when to GC the rule tree. r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14256)

bors-servo · 2016-11-20T04:47:31Z

💔 Test failed - mac-rel-wpt1

heycam · 2016-11-20T04:50:32Z

@bors-servo retry

#11100

bors-servo · 2016-11-20T05:25:29Z

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

bors-servo · 2016-11-20T06:01:44Z

☀️ Test successful - 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 emilio Nov 17, 2016

highfive added the S-awaiting-review label Nov 17, 2016

emilio approved these changes Nov 17, 2016

View changes

bors-servo mentioned this pull request Nov 17, 2016

Urlmageddon #14246

Merged

emilio and others added 2 commits Nov 15, 2016

rule tree: Avoid yet another dumb assertion when dropping the rule tree.

181208a

GC the rule tree only when the free list gets to a certain size.

Loading status checks…

4e52bb4

heycam force-pushed the heycam:rn-gc-heuristic branch from ce27483 to 4e52bb4 Nov 18, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 18, 2016

heycam mentioned this pull request Nov 18, 2016

rule tree: Avoid yet another dumb assertion when dropping the rule tree. #14233

Closed

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

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

bors-servo mentioned this pull request Nov 19, 2016

Make document url mutable and implement location.replace() #13418

Merged

4 of 4 tasks complete

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

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

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

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 20, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 20, 2016

bors-servo merged commit 4e52bb4 into servo:master Nov 20, 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

bors-servo mentioned this pull request Nov 20, 2016

style: Don't assert when the final rule tree GC happens in script. #14278

Merged

servo / servo

GC the rule tree only when the free list gets to a certain size. #14256

GC the rule tree only when the free list gets to a certain size. #14256

heycam commented Nov 17, 2016 •

edited by larsbergstrom

highfive commented Nov 17, 2016

highfive commented Nov 17, 2016

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

emilio commented Nov 17, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 17, 2016

heycam commented Nov 18, 2016

heycam commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 19, 2016

emilio commented Nov 19, 2016

bors-servo commented Nov 19, 2016

bors-servo commented Nov 19, 2016

KiChjang commented Nov 19, 2016

bors-servo commented Nov 19, 2016

bors-servo commented Nov 19, 2016

highfive commented Nov 19, 2016

heycam commented Nov 20, 2016

bors-servo commented Nov 20, 2016

bors-servo commented Nov 20, 2016

heycam commented Nov 20, 2016

bors-servo commented Nov 20, 2016

bors-servo commented Nov 20, 2016

servo / servo

Join GitHub today

GC the rule tree only when the free list gets to a certain size. #14256

GC the rule tree only when the free list gets to a certain size. #14256

Conversation

heycam commented Nov 17, 2016 • edited by larsbergstrom

highfive commented Nov 17, 2016

highfive commented Nov 17, 2016

emilio left a comment

This comment has been minimized.

emilio Nov 17, 2016

This comment has been minimized.

heycam Nov 18, 2016

This comment has been minimized.

emilio Nov 17, 2016

This comment has been minimized.

emilio Nov 17, 2016

This comment has been minimized.

heycam Nov 18, 2016

emilio commented Nov 17, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 17, 2016

heycam commented Nov 18, 2016

heycam commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 19, 2016

emilio commented Nov 19, 2016

bors-servo commented Nov 19, 2016

bors-servo commented Nov 19, 2016

KiChjang commented Nov 19, 2016

bors-servo commented Nov 19, 2016

bors-servo commented Nov 19, 2016

highfive commented Nov 19, 2016

heycam commented Nov 20, 2016

bors-servo commented Nov 20, 2016

bors-servo commented Nov 20, 2016

heycam commented Nov 20, 2016

bors-servo commented Nov 20, 2016

bors-servo commented Nov 20, 2016

heycam commented Nov 17, 2016 •

edited by larsbergstrom