clear ServoNodeData from the entire document when the ServoStyleSet is shutting down

RESOLVED FIXED in Firefox 53

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

58 bytes, text/x-review-board-request
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Review
58 bytes, text/x-review-board-request
emilio
: review+
Details | Review
58 bytes, text/x-review-board-request
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Review
(Assignee)

Description

a year ago
If we wait until nodes are deleted to clear the ServoNodeData, then we will miss a last chance to GC the Servo RuleTree, which happens when the Stylist is dropped, and we leak the RuleNodes.

The simplest thing would be to iterate over the document and clear all the ServoNodeDatas when the ServoStyleSet is being shut down, but before it drops the RawServoStyleSet.

Timing on my machine, iteration over the single page HTML spec without doing the actual ServoNodeData dropping takes ~100ms, which is about 0.3 us per node we iterate over (there being ~340,000 nodes in the document).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c9dfade9b6efd3cf5c945b3b5db318e80eb6b0
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=770937a7163d2875b030921a68adc5543f5127c2

Comment 6

a year ago
mozreview-review
Comment on attachment 8811586 [details]
GC the RuleTree when the Stylist is dropped.

https://reviewboard.mozilla.org/r/93658/#review93750

::: servo/components/style/selector_matching.rs:102
(Diff revision 1)
>      non_common_style_affecting_attributes_selectors: Vec<Selector<TheSelectorImpl>>,
>  }
>  
> +impl Drop for Stylist {
> +    fn drop(&mut self) {
> +        unsafe { self.rule_tree.gc(); }

Can you add a comment here with why is it needed?
Attachment #8811586 - Flags: review?(ecoal95) → review+
(Assignee)

Comment 7

a year ago
One strange thing: at the last minute, I moved the ClearAllStyleData call from BeginShutdown to Shutdown, but I realize now that the leak persists because of that.  Moving it back fixes it.  Don't know why...
Comment on attachment 8811587 [details]
Mark the Gecko main thread as a Servo layout thread.

https://reviewboard.mozilla.org/r/93660/#review93906
Attachment #8811587 - Flags: review?(bobbyholley) → review+
Comment on attachment 8811585 [details]
Bug 1318238 - Clear all ServoNodeData during style set shutdown.

https://reviewboard.mozilla.org/r/93656/#review93912

Hm, so that traversal overhead is significant - about 10% of a full-document restyle IIUC.

I think we should probably do the thing you suggested where we transfer ownership of the style set to the document. However, I don't want to get in the way of fixing the leaks. Can you file this as a followup blocking stylo-perf and we'll get to it later?

::: layout/base/ServoRestyleManager.cpp:94
(Diff revision 1)
> +  aContent->ClearServoData();
> +  aContent->SetIsDirtyForServo();

Can you also clear the dirty descendants bit here?

::: layout/style/ServoStyleSet.cpp:42
(Diff revision 1)
> +  // It's important to do this before mRawSet is released, since that will cause
> +  // a RuleTree GC, which needs to happen after we have dropped all of the
> +  // document's strong references to RuleNodes.

Add a comment here about perf.
Attachment #8811585 - Flags: review?(bobbyholley) → review+
Oh, and let's call it "ClearServoDataFromSubtree" instead of "ClearAllStyleData".
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
https://github.com/servo/servo/pull/14273

Comment 15

a year ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8bea653cdb7
Clear all ServoNodeData during style set shutdown. r=bholley

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8bea653cdb7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.