Closed Bug 1318238 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(3 files)

Bug 1318238 - Clear all ServoNodeData during style set shutdown.
58 bytes, text/x-review-board-request
bholley
: review+
Details
GC the RuleTree when the Stylist is dropped.
58 bytes, text/x-review-board-request
emilio
: review+
Details
Mark the Gecko main thread as a Servo layout thread.
58 bytes, text/x-review-board-request
bholley
: review+
Details 
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 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+
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".
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8bea653cdb7
Clear all ServoNodeData during style set shutdown. r=bholley
https://hg.mozilla.org/mozilla-central/rev/d8bea653cdb7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: