Closed Bug 1353998 Opened 3 years ago Closed 3 years ago

Do some minimal Stylo memory reporting

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [stylo])

Attachments

(1 file, 3 obsolete files)

I want to measure some small part of some data structure within Stylo, just to get the initial plumbing in place.
This currently doesn't actually measure anything on the Servo side. That needs
to be added before this lands.

BTW, am I going to have to follow the horrific "contribution workflow" at
https://public.etherpad-mozilla.org/p/stylo to land this?
Attachment #8855181 - Flags: feedback?(cam)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 8855181 [details] [diff] [review]
> (draft) - Do some minimal Stylo memory reporting
> 
> This currently doesn't actually measure anything on the Servo side. That
> needs
> to be added before this lands.
>
> BTW, am I going to have to follow the horrific "contribution workflow" at
> https://public.etherpad-mozilla.org/p/stylo to land this?

Yep. glob is working on a way for us to land code straight to m-c, but it's still in the works.

An important thing to measure memory-wise is the mServoData that hangs off mozilla::dom::Element. That's where all the computed style lives.
Comment on attachment 8855181 [details] [diff] [review]
(draft) - Do some minimal Stylo memory reporting

Review of attachment 8855181 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSheet.cpp
@@ +35,5 @@
>  {
>    size_t n = aMallocSizeOf(this);
>  
> +  // njn: safe to measure through the RefPtr? might double-count?
> +  n += Servo_StyleSheet_SizeOfIncludingThis(aMallocSizeOf, mSheet);

I think it's right to measure it here; that matches what's we're doing in stock Gecko (where, the CSSStyleSheetInner object measures the rules inside it, a single CSSStyleSheet is responsible for measuring a given Inner object (which might be shared among multiple CSSStyleSheets), and the document and nsLayoutStylesheetCache are responsible for measuring the CSSStyleSheets.  So just as nsStyleSet doesn't measure the sheets it has in it, neither should we make the Servo PerDocumentStyleData object measure the sheets it has.

::: servo/components/style/gecko_bindings/bindings.rs
@@ +1406,5 @@
>      pub fn Servo_StyleSheet_GetRules(sheet: RawServoStyleSheetBorrowed)
>       -> ServoCssRulesStrong;
>  }
>  extern "C" {
> +    pub fn Servo_StyleSheet_SizeOfIncludingThis(sheet: RawServoStyleSheetBorrowed)

This doesn't match glue.rs.  Did you manage to get bindgen working locally in the end?

::: servo/ports/geckolib/glue.rs
@@ +489,5 @@
>  pub extern "C" fn Servo_StyleSheet_GetRules(sheet: RawServoStyleSheetBorrowed) -> ServoCssRulesStrong {
>      Stylesheet::as_arc(&sheet).rules.clone().into_strong()
>  }
>  
> +type MallocSizeOf = unsafe fn(ptr: *const ::libc::c_void) -> ::libc::size_t;

It looks like MallocSizeOf is already bindgened -- you can see it in structs_debug.rs, where it's defined as:

pub type MallocSizeOf = ::std::option::Option<unsafe extern "C" fn(p: *const ::std::os::raw::c_void) -> ::std::os::raw::c_ulong>;

So you can add |use style::gecko_bindings::structs::MallocSizeOf;| at the top of glue.rs to use it.

@@ +497,5 @@
> +pub extern "C" fn Servo_StyleSheet_SizeOfIncludingThis(malloc_size_of: MallocSizeOf,
> +                                                       sheet: RawServoStyleSheetBorrowed) -> usize {
> +//    let global_style_data = &*GLOBAL_STYLE_DATA;
> +//    let guard = global_style_data.shared_lock.read();
> +    // njn: ok to go through the Arc?

Yes, I think that's the right thing to do.

@@ +499,5 @@
> +//    let global_style_data = &*GLOBAL_STYLE_DATA;
> +//    let guard = global_style_data.shared_lock.read();
> +    // njn: ok to go through the Arc?
> +//    let rules = Stylesheet::as_arc(&sheet).rules.read_with(&guard);
> +//    let x = rules.heap_size_blah();

I think it would be better to add a heap_size_blah() function to Stylesheet, and to have it call heap_size_blah() on its rules.
Attachment #8855181 - Flags: feedback?(cam) → feedback+
Whiteboard: [stylo]
> > +type MallocSizeOf = unsafe fn(ptr: *const ::libc::c_void) -> ::libc::size_t;
> 
> It looks like MallocSizeOf is already bindgened -- you can see it in
> structs_debug.rs, where it's defined as:
> 
> pub type MallocSizeOf = ::std::option::Option<unsafe extern "C" fn(p: *const
> ::std::os::raw::c_void) -> ::std::os::raw::c_ulong>;

The thing I'm unsure about here is the Option<>. (I imagine it's in the generated type because the pointer might be null.)

When I pass a MallocSizeOf function pointer from C++ as an argument to Servo_StyleSheet_SizeOfIncludingThis() what happens?

- Is there some kind of auto-wrapping?

- Or is the representation of an |Option<fn>| the same as a |fn| under the Rust covers, so it just magically works?

- An unwrap() is required on the Rust side... does that check for nullness without changing the representation?)
Flags: needinfo?(cam)
(In reply to Nicholas Nethercote [:njn] from comment #4)
> > > +type MallocSizeOf = unsafe fn(ptr: *const ::libc::c_void) -> ::libc::size_t;
> > 
> > It looks like MallocSizeOf is already bindgened -- you can see it in
> > structs_debug.rs, where it's defined as:
> > 
> > pub type MallocSizeOf = ::std::option::Option<unsafe extern "C" fn(p: *const
> > ::std::os::raw::c_void) -> ::std::os::raw::c_ulong>;
> 
> The thing I'm unsure about here is the Option<>. (I imagine it's in the
> generated type because the pointer might be null.)


Yes, Option<extern fn> is well defined in rust as a nullable function pointer.

> When I pass a MallocSizeOf function pointer from C++ as an argument to
> Servo_StyleSheet_SizeOfIncludingThis() what happens?
> 
> - Is there some kind of auto-wrapping?

Option<extern fn> is just a pointer under the hood, so no auto-wrapping is required, it just works.

> - Or is the representation of an |Option<fn>| the same as a |fn| under the
> Rust covers, so it just magically works?
> 
> - An unwrap() is required on the Rust side... does that check for nullness
> without changing the representation?)

Yes, that basically ensures the pointer is non-null, or panics otherwise.
Flags: needinfo?(cam)
Thanks, Emilio! I tried changing the new function to take (unsafely) a non-Option-wrapped function pointer, and it worked just the same as the Option<> version. This explanation makes sense of that behaviour.
Attachment #8855181 - Attachment is obsolete: true
This patch includes the first instance of memory measurements in Stylo being
passed back to Gecko for reporting.

The patch introduces a new trait, MallocSizeOf, which is similar to the
existing HeapSizeOf trait from the heapsize crate. The only difference is that
MallocSizeOf's malloc_size_of_children() function takes an additional
MallocSizeOfFn argument, which is used to measure heap blocks. This extra
argument makes MallocSizeOf match how Gecko's memory measurements work, and is
required for Stylo to integrate with DMD.

The patch also introduces a second trait, MallocSizeOfWithGuard, which is
much the same as MallocSizeOf, but with a |guard| argument for the global style
lock.

A number of things within Stylo stylesheets are currently not measured, because
this patch is mostly about getting something basic working. With the Barack
Obama wikipedia page loaded, the new measurements account for just under 1 MiB
of memory usage across all processes on Linux64.
Attachment #8871124 - Flags: review?(cam)
Attachment #8870748 - Attachment is obsolete: true
One thing of note: the patch measures through several Arcs. I'm basically assuming that the Arcs I'm measuring are the "primary" or "owning" references, but I don't know if that's true. But I did do a DMD run on the Obama Wikipedia page and there was no double reporting, which is a good sign.
Comment on attachment 8871124 [details] [diff] [review]
Do some minimal Stylo memory reporting

Review of attachment 8871124 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great.  I like the impls for Vec, pairs, etc.

::: servo/components/style/stylesheets.rs
@@ +125,5 @@
> +
> +impl<T: MallocSizeOf> MallocSizeOf for Vec<T> {
> +    fn malloc_size_of_children(&self, malloc_size_of: MallocSizeOfFn) -> usize {
> +        self.iter().fold(
> +            unsafe { malloc_size_of(self.as_ptr() as *const ::std::os::raw::c_void) },

You may even be able to write that as

  self.as_ptr() as *const _

and let type inference work out the specific pointer type.

By the way, are we guaranteed that self.as_ptr() will return a pointer to the start of an allocation?  (Or, even if it's in the middle of an allocation, maybe the malloc_sizeof stuff doesn't mind?)

@@ +320,5 @@
>      pub quirks_mode: QuirksMode,
>  }
>  
> +impl MallocSizeOfWithGuard for Stylesheet
> +{

Nit: "{" on previous line.

@@ +362,5 @@
>      Document(Arc<Locked<DocumentRule>>),
>  }
>  
> +impl MallocSizeOfWithGuard for CssRule
> +{

Nit: here too.

@@ +367,5 @@
> +    fn malloc_size_of_children(&self, guard: &SharedRwLockReadGuard,
> +                               malloc_size_of: MallocSizeOfFn) -> usize {
> +        match *self {
> +            CssRule::Style(ref lock) =>
> +                lock.read_with(guard).malloc_size_of_children(guard, malloc_size_of),

Nit: I think prevailing style is to put braces around the RHS of the match arm if it's not placed on the same line as the pattern.
Attachment #8871124 - Flags: review?(cam) → review+
> By the way, are we guaranteed that self.as_ptr() will return a pointer to
> the start of an allocation?

No! Though the obvious way to implement Vec results in that, and the implementation in practice gives us that, and the same approach has been working in Servo for ages.

Ideally the MallocSizeOf trait for Vec would be implemented within vec.rs, where these kinds of internal implementation details belong.

> (Or, even if it's in the middle of an
> allocation, maybe the malloc_sizeof stuff doesn't mind?)

It does mind, and is likely to crash if it's the middle of an allocation.
Comment on attachment 8871124 [details] [diff] [review]
Do some minimal Stylo memory reporting

Review of attachment 8871124 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/stylesheets.rs
@@ +125,5 @@
> +
> +impl<T: MallocSizeOf> MallocSizeOf for Vec<T> {
> +    fn malloc_size_of_children(&self, malloc_size_of: MallocSizeOfFn) -> usize {
> +        self.iter().fold(
> +            unsafe { malloc_size_of(self.as_ptr() as *const ::std::os::raw::c_void) },

This may need a special case for when the vector is empty. In that case, rust doesn't return a valid pointer. This has bitten us in the past, see bug 1344209.
> > +impl<T: MallocSizeOf> MallocSizeOf for Vec<T> {
> > +    fn malloc_size_of_children(&self, malloc_size_of: MallocSizeOfFn) -> usize {
> > +        self.iter().fold(
> > +            unsafe { malloc_size_of(self.as_ptr() as *const ::std::os::raw::c_void) },
> 
> This may need a special case for when the vector is empty.

Good catch! The heap size crate deals with this like so:

> pub unsafe fn heap_size_of(ptr: *const c_void) -> usize {
>     if ptr == 0x01 as *const c_void {
>         0
>     } else {
>         heap_size_of_impl(ptr)
>     }
> }

I'll need something similar here.
I can land the Servo and Gecko parts separately. I've filed https://github.com/servo/servo/pull/17044 for the Servo parts.
This is the Gecko part of the previously r+'d patch. It can land once the Servo
PR has been approved and merged.
Attachment #8871124 - Attachment is obsolete: true
Comment on attachment 8871550 [details] [diff] [review]
Start measuring Stylo style sheet memory usage

Review of attachment 8871550 [details] [diff] [review]:
-----------------------------------------------------------------

Carrying over r+.
Attachment #8871550 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/69582a890f53
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.