1394729 - Measure memory usage of Stylo's Rule Tree

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Nicholas Nethercote [inactive]]

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 1

[Nicholas Nethercote [inactive]]

Attachment: Measure memory usage of Stylo's Rule Tree

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1394729 - Measure memory usage of Stylo's Rule Tree. .

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.

Review commit: https://reviewboard.mozilla.org/r/173608/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173608/

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

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`.

Comment 4

[Cameron McCormack (:heycam)]

(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 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Nicholas Nethercote [inactive]]

> 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 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8902148 [details]
Bug 1394729 - Measure memory usage of Stylo's Rule Tree. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173608/diff/1-2/

Comment 8

[Cameron McCormack (:heycam)]

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

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8902148 [details]
Bug 1394729 - Measure memory usage of Stylo's Rule Tree. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173608/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b9d06ba6f76
Measure memory usage of Stylo's Rule Tree. r=heycam.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6b9d06ba6f76