This patch splits up the existing "layout/style-sets" measurement into "layout/gecko-style-sets", or "layout/servo-style-sets/stylist/rule-tree" and "layout/servo-style-sets/other". (Additional things will be measure under "layout/servo-style-sets/" later, such as cascade data.) This requires introducing a new type, ServoStyleSetSizes, for transferring the multiple measurements from Rust code to C++ code. MozReview-Commit-ID: FbmzpsjBpgI

Comment on attachment 8902148 [details] Bug 1394729 - Measure memory usage of Stylo's Rule Tree. . https://reviewboard.mozilla.org/r/173608/#review178936 ::: servo/components/style/rule_tree/mod.rs:612 (Diff revision 1) > > +impl MallocSizeOf for RuleNode { > + fn malloc_size_of_children(&self, malloc_size_of: MallocSizeOfFn) -> usize { > + let mut n = 0; > + n += self.first_child.malloc_size_of_children(malloc_size_of); > + n += self.next_sibling.malloc_size_of_children(malloc_size_of); I'm not sure whether rule tree can be very wide... If it can, probably we should change this to be a loop over children, rather than simply doing recursion into `first_child` and `next_sibling`.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3) > I'm not sure whether rule tree can be very wide... If it can, probably we > should change this to be a loop over children, rather than simply doing > recursion into `first_child` and `next_sibling`. Yeah, I think we might get long child lists sometimes. It should be easy to change to iterate instead so let's do that.

Comment on attachment 8902148 [details] Bug 1394729 - Measure memory usage of Stylo's Rule Tree. . https://reviewboard.mozilla.org/r/173608/#review178938 r=me with iteration over each node's child list rather than recursion. ::: commit-message-7dddb:5 (Diff revision 1) > +Bug 1394729 - Measure memory usage of Stylo's Rule Tree. r=heycam. > + > +This patch splits up the existing "layout/style-sets" measurement into > +"layout/gecko-style-sets", or "layout/servo-style-sets/stylist/rule-tree" and > +"layout/servo-style-sets/other". (Additional things will be measure under measured ::: servo/components/style/rule_tree/mod.rs:612 (Diff revision 1) > > +impl MallocSizeOf for RuleNode { > + fn malloc_size_of_children(&self, malloc_size_of: MallocSizeOfFn) -> usize { > + let mut n = 0; > + n += self.first_child.malloc_size_of_children(malloc_size_of); > + n += self.next_sibling.malloc_size_of_children(malloc_size_of); For me it feels a bit strange that it's malloc_size_of_children's job to measure the RuleNode's size itself, although maybe you can think of the RuleNode as being a "child" of the AtomicPtr? ::: servo/components/style/stylesheets/memory.rs:128 (Diff revision 1) > + > + Nit: drop these two blank lines. ::: servo/ports/geckolib/glue.rs:3534 (Diff revision 1) > + sizes: *mut ServoStyleSetSizes, > + raw_data: RawServoStyleSetBorrowed > +) { > + let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); > + let malloc_size_of = malloc_size_of.unwrap(); > + let sizes = unsafe { &mut *sizes }; Nit: if you do let sizes = unsafe { sizes.as_mut() }.unwrap(); you'll get a nicer panic (rather than a crash) on passing in a null pointer.

> For me it feels a bit strange that it's malloc_size_of_children's job to > measure the RuleNode's size itself, although maybe you can think of the > RuleNode as being a "child" of the AtomicPtr? I understand the concern. I'm not entirely happy with all of the memory measurement in Rust code -- the MallocSizeOf traits, what exactly constitutes "children", etc. In general the Rust memory reporting code is more sophisticated but also harder to understand than the equivalent C++ code. (Especially with types like Arc.) I want to overhaul it but I need to think about it for a while to come up with the right design. So I'll leave this as is for now, but I've made a note to consider it when I do that redesign.

Comment on attachment 8902148 [details] Bug 1394729 - Measure memory usage of Stylo's Rule Tree. . https://reviewboard.mozilla.org/r/173608/#review179330 ::: servo/components/style/rule_tree/mod.rs:609 (Diff revisions 1 - 2) > > unsafe impl Sync for RuleTree {} > unsafe impl Send for RuleTree {} > > -impl MallocSizeOf for RuleNode { > - fn malloc_size_of_children(&self, malloc_size_of: MallocSizeOfFn) -> usize { > +impl RuleNode { > + fn malloc_size_of_including_self(&self, malloc_size_of: MallocSizeOfFn) -> usize { This should probably just moved down into the existing "impl RuleNode" block. ::: servo/components/style/rule_tree/mod.rs:611 (Diff revisions 1 - 2) > - n += self.first_child.malloc_size_of_children(malloc_size_of); > - n += self.next_sibling.malloc_size_of_children(malloc_size_of); > + let mut curr = self; > + loop { > + // Measure curr. I think this can be shortened a bit: let mut n = 0; unsafe { n += malloc_size_of(self as *const _ as *const _); for child in self.iter_children() { n += malloc_size_of_including_self(child.ptr() as *const _ as *const _); } } n

Pushed by nnethercote@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6b9d06ba6f76 Measure memory usage of Stylo's Rule Tree. r=heycam.