diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index 198999f61b1..18c897eae13 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -122,6 +122,11 @@ impl RuleTree { self.root.clone() } + /// Returns true if there are no nodes in the tree aside from the rule node. + pub fn is_empty(&self) -> bool { + self.root.get().first_child.load(Ordering::Relaxed).is_null() + } + fn dump(&self, guards: &StylesheetGuards, writer: &mut W) { let _ = writeln!(writer, " + RuleTree"); self.root.get().dump(guards, writer, 0); @@ -498,6 +503,25 @@ impl CascadeLevel { } } +// The root node never has siblings, but needs a free count. We use the same +// storage for both to save memory. +struct PrevSiblingOrFreeCount(AtomicPtr); +impl PrevSiblingOrFreeCount { + fn new() -> Self { + PrevSiblingOrFreeCount(AtomicPtr::new(ptr::null_mut())) + } + + unsafe fn as_prev_sibling(&self) -> &AtomicPtr { + &self.0 + } + + unsafe fn as_free_count(&self) -> &AtomicUsize { + unsafe { + mem::transmute(&self.0) + } + } +} + /// A node in the rule tree. pub struct RuleNode { /// The root node. Only the root has no root pointer, for obvious reasons. @@ -516,18 +540,16 @@ pub struct RuleNode { refcount: AtomicUsize, first_child: AtomicPtr, next_sibling: AtomicPtr, - prev_sibling: AtomicPtr, + + /// Previous sibling pointer for all non-root nodes. + /// + /// For the root, stores the of RuleNodes we have added to the free list + /// since the last GC. (We don't update this if we rescue a RuleNode from + /// the free list. It's just used as a heuristic to decide when to run GC.) + prev_sibling_or_free_count: PrevSiblingOrFreeCount, /// The next item in the rule tree free list, that starts on the root node. next_free: AtomicPtr, - - /// Number of RuleNodes we have added to the free list since the last GC. - /// (We don't update this if we rescue a RuleNode from the free list. It's - /// just used as a heuristic to decide when to run GC.) - /// - /// Only used on the root RuleNode. (We could probably re-use one of the - /// sibling pointers to save space.) - free_count: AtomicUsize, } unsafe impl Sync for RuleTree {} @@ -547,9 +569,8 @@ impl RuleNode { refcount: AtomicUsize::new(1), first_child: AtomicPtr::new(ptr::null_mut()), next_sibling: AtomicPtr::new(ptr::null_mut()), - prev_sibling: AtomicPtr::new(ptr::null_mut()), + prev_sibling_or_free_count: PrevSiblingOrFreeCount::new(), next_free: AtomicPtr::new(ptr::null_mut()), - free_count: AtomicUsize::new(0), } } @@ -562,9 +583,8 @@ impl RuleNode { refcount: AtomicUsize::new(1), first_child: AtomicPtr::new(ptr::null_mut()), next_sibling: AtomicPtr::new(ptr::null_mut()), - prev_sibling: AtomicPtr::new(ptr::null_mut()), + prev_sibling_or_free_count: PrevSiblingOrFreeCount::new(), next_free: AtomicPtr::new(FREE_LIST_SENTINEL), - free_count: AtomicUsize::new(0), } } @@ -583,6 +603,16 @@ impl RuleNode { } } + fn prev_sibling(&self) -> &AtomicPtr { + debug_assert!(!self.is_root()); + unsafe { self.prev_sibling_or_free_count.as_prev_sibling() } + } + + fn free_count(&self) -> &AtomicUsize { + debug_assert!(self.is_root()); + unsafe { self.prev_sibling_or_free_count.as_free_count() } + } + /// Remove this rule node from the child list. /// /// This method doesn't use proper synchronization, and it's expected to be @@ -596,7 +626,7 @@ impl RuleNode { // NB: The other siblings we use in this function can also be dead, so // we can't use `get` here, since it asserts. let prev_sibling = - self.prev_sibling.swap(ptr::null_mut(), Ordering::Relaxed); + self.prev_sibling().swap(ptr::null_mut(), Ordering::Relaxed); let next_sibling = self.next_sibling.swap(ptr::null_mut(), Ordering::Relaxed); @@ -615,7 +645,7 @@ impl RuleNode { // otherwise we're done. if !next_sibling.is_null() { let next = &*next_sibling; - next.prev_sibling.store(prev_sibling, Ordering::Relaxed); + next.prev_sibling().store(prev_sibling, Ordering::Relaxed); } } @@ -760,7 +790,7 @@ impl StrongRuleNode { // only be accessed again in a single-threaded manner when // we're sweeping possibly dead nodes. if let Some(ref l) = last { - node.prev_sibling.store(l.ptr(), Ordering::Relaxed); + node.prev_sibling().store(l.ptr(), Ordering::Relaxed); } return StrongRuleNode::new(node); @@ -908,14 +938,14 @@ impl StrongRuleNode { let _ = Box::from_raw(weak.ptr()); } - me.free_count.store(0, Ordering::Relaxed); + me.free_count().store(0, Ordering::Relaxed); debug_assert!(me.next_free.load(Ordering::Relaxed) == FREE_LIST_SENTINEL); } unsafe fn maybe_gc(&self) { debug_assert!(self.get().is_root(), "Can't call GC on a non-root node!"); - if self.get().free_count.load(Ordering::Relaxed) > RULE_TREE_GC_INTERVAL { + if self.get().free_count().load(Ordering::Relaxed) > RULE_TREE_GC_INTERVAL { self.gc(); } } @@ -1339,6 +1369,11 @@ impl Drop for StrongRuleNode { // This can be relaxed since this pointer won't be read until GC. node.next_free.store(old_head, Ordering::Relaxed); + // Increment the free count. This doesn't need to be an RMU atomic + // operation, because the free list is "locked". + let old_free_count = root.free_count().load(Ordering::Relaxed); + root.free_count().store(old_free_count + 1, Ordering::Relaxed); + // This can be release because of the locking of the free list, that // ensures that all the other nodes racing with this one are using // `Acquire`. diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 35a257dcabe..a5a6313b3f2 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1283,10 +1283,10 @@ impl Drop for Stylist { // dropped all strong rule node references before now, then we will // leak them, since there will be no way to call gc() on the rule tree // after this point. - // - // TODO(emilio): We can at least assert all the elements in the free - // list are indeed free. unsafe { self.rule_tree.gc(); } + + // Assert against leaks. + debug_assert!(self.rule_tree.is_empty()); } } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 2b973124977..96577b34546 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -303,6 +303,16 @@ pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed, element.has_dirty_descendants() || element.borrow_data().unwrap().restyle.contains_restyle_data() } +/// Checks whether the rule tree has crossed its threshold for unused nodes, and +/// if so, frees them. +#[no_mangle] +pub extern "C" fn Servo_MaybeGCRuleTree(raw_data: RawServoStyleSetBorrowed) { + let per_doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); + unsafe { + per_doc_data.stylist.rule_tree().maybe_gc(); + } +} + #[no_mangle] pub extern "C" fn Servo_AnimationValues_Interpolate(from: RawServoAnimationValueBorrowed, to: RawServoAnimationValueBorrowed, diff --git a/tests/unit/stylo/size_of.rs b/tests/unit/stylo/size_of.rs index 633180d3289..4f1cb4bbae5 100644 --- a/tests/unit/stylo/size_of.rs +++ b/tests/unit/stylo/size_of.rs @@ -42,7 +42,7 @@ size_of_test!(test_size_of_application_declaration_block, ApplicableDeclarationB // FIXME(bholley): This can shrink with a little bit of work. // See https://github.com/servo/servo/issues/17280 -size_of_test!(test_size_of_rule_node, RuleNode, 96); +size_of_test!(test_size_of_rule_node, RuleNode, 88); // This is huge, but we allocate it on the stack and then never move it, // we only pass `&mut SourcePropertyDeclaration` references around.