-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ensure RuleNodes are dropped when Gecko drops the Stylist #14273
Conversation
@bors-servo r=bholley,emilio |
📌 Commit cd164d6 has been approved by |
⌛ Testing commit cd164d6 with merge 09a5a7d... |
ensure RuleNodes are dropped when Gecko drops the Stylist <!-- Please describe your changes on the following line: --> These are the Servo-side patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1318238, which already have been reviewed by @bholley and @emilio there. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14273) <!-- Reviewable:end -->
💔 Test failed - linux-rel-wpt |
⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt... |
💔 Test failed - linux-rel-wpt |
☔ The latest upstream changes (presumably #14294) made this pull request unmergeable. Please resolve the merge conflicts. |
We only consider doing a GC currently if the root node has a zero refcount. But that only happens if it has no children -- even weak children keep a strong reference to their parent. So at the very least, we should do a GC specifically when the RuleTree is going away. (We probably want to add some other GC opportunities too at some point, otherwise it's easy to never GC a RuleTree.)
This is needed to keep the assertion in the RuleTree GC functions that checks we're being called on the non-worker layout thread.
cd164d6
to
226c946
Compare
@bors-servo r=bholley,emilio |
📌 Commit 226c946 has been approved by |
⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt... |
💔 Test failed - linux-rel-wpt |
@emilio This is hitting the new assertion that came from #14278. In that assertion, why do we assert that the refcount of the root node is 0? Is it because we are assuming that we have previously just done a GC that removed all nodes except for the root? With the patches in this PR, since we don't GC so often, we might be getting that final GC from the script thread (from the JS GC, it looks like) with some rule nodes still in the tree (and therefore with the root having refcount > 0). Is there a problem with that? Should we instead be asserting that there are no rule nodes in the rule tree that have refcounts that exceed their number of children (i.e. there are no strong references to them from elsewhere)? |
@bors-servo r- |
…trong references. It could still have children.
@emilio said on IRC it should be fine to remove the refcount=0 part of the assertion. |
@bors-servo r=bholley,emilio |
📌 Commit d92cc8e has been approved by |
ensure RuleNodes are dropped when Gecko drops the Stylist <!-- Please describe your changes on the following line: --> These are the Servo-side patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1318238, which already have been reviewed by @bholley and @emilio there. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14273) <!-- Reviewable:end -->
💔 Test failed - mac-rel-wpt1 |
@bors-servo retry #14267 |
ensure RuleNodes are dropped when Gecko drops the Stylist <!-- Please describe your changes on the following line: --> These are the Servo-side patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1318238, which already have been reviewed by @bholley and @emilio there. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14273) <!-- Reviewable:end -->
☀️ 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 |
These are the Servo-side patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1318238, which already have been reviewed by @bholley and @emilio there.
This change is