1281964 - hook up stylo memory reporters

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Cameron McCormack (:heycam)]

We don't currently call into Servo's heap_size_of functions when measuring memory use in Stylo-enabled Gecko builds.  We should do that (and fill out those heap_size_of functions, not all of which return sensible values at the moment).

Comment 1

[Nicholas Nethercote [inactive]]

I can take this, though I probably won't get to it until January.

Comment 2

[Bobby Holley (:bholley)]

Nick, do you have cycles for this now?

Comment 3

[Nicholas Nethercote [inactive]]

I've done a small amount here, and I'm planning to get it done this quarter. Apologies for the delays.

Comment 4

[Bobby Holley (:bholley)]

Sounds good! Q2 sounds like the perfect timeframe for us.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: DMD output

Here is the top 20 records from DMD, measuring the content process, with the Obama Wikipedia page and about:memory open in two tabs.

The types that come up are the following.

- ComputedValue (240 bytes; see bug 1367635 and bug 1367862)
- GeckoBox (432 bytes)
- GeckoPosition (1024 bytes)
- SelectorImpl (variable size)
- GeckoBackground (176)

Comment 6

[Nicholas Nethercote [inactive]]

> Here is the top 20 records from DMD

bz, you like looking at output like this...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah...

So the way memory reporting works in Gecko for the equivalents of ComputedValues and GeckoBox/Position/Background is that they are all allocated from the presshell arenas and we just report them from there.  I'm not quite sure how to go about reporting them with stylo short of doing something like that.

The actual number of allocations for ComputedValues will go down as we improve sharing.  The number of GeckoBox/Position/Background allocations, similar, though might not be quite as dramatic because we might be refcounting those some already even in non-sharing cases.

SelectorImpl it would be interesting to compare to Gecko's selector memory usage.  It should also be much simpler to add reporting for, since there's clearer ownership there.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #7)
> Yeah...
> 
> So the way memory reporting works in Gecko for the equivalents of
> ComputedValues and GeckoBox/Position/Background is that they are all
> allocated from the presshell arenas and we just report them from there.  I'm
> not quite sure how to go about reporting them with stylo short of doing
> something like that.
> 
> The actual number of allocations for ComputedValues will go down as we
> improve sharing.  The number of GeckoBox/Position/Background allocations,
> similar, though might not be quite as dramatic because we might be
> refcounting those some already even in non-sharing cases.
> 
> SelectorImpl it would be interesting to compare to Gecko's selector memory
> usage.  It should also be much simpler to add reporting for, since there's
> clearer ownership there.

Well, not entirely, since it's still a refcounted type (note that, after bug 1370107, it's all just Selector, which wraps a ThinArc). It's shared among the stylesheets and the various SelectorMaps.

I guess the right way to do it is just to bill it to the stylesheets, and ignore it on the SelectorMaps.

The ComputedValues and style structs (all shared among elements via copy-on-write) are much harder, and we can't just ignore them because that's the bulk of the memory overhead. I guess the obvious thing is to maintain a hashset of pointers and track which ones we've seen?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I guess the right way to do it is just to bill it to the stylesheets, and ignore it on the SelectorMaps.

Right.  If it gets removed from the sheet, it will also get removed from the SelectorMap.  So conceptually the sheet is the sole owner.

The hashset thing could work, because we're not really trying to pin these down more than on a "per document" level.

We'd need to be very sure we knew how to reach all of them.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #9)

> We'd need to be very sure we knew how to reach all of them.

As long as we trace through both mServoData (on Element) and mServoComputedValues (on nsStyleContext) I think we should be good on this front.

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Noticed today with DMD that SelectorMap's id_hash take significantly heap-unclassified memory

Unreported {
  5,179 blocks in heap block record 1 of 25,725
  10,610,688 bytes (6,631,680 requested / 3,979,008 slop)
  Individual block sizes: 4,096 x 2; 2,048 x 5,177
  4.94% of the heap (4.94% cumulative)
  12.35% of unreported (12.35% cumulative)
  Allocated at {
    #01: replace_malloc (/home/kanru/mozilla/gecko/memory/replace/dmd/DMD.cpp:1308)
    #02: malloc (/home/kanru/mozilla/gecko/memory/build/malloc_decls.h:49 (discriminator 3))
    #03: <std::collections::hash::map::HashMap<K, V, S>>::resize (/checkout/src/liballoc_system/lib.rs:90)
    #04: fnv::{{impl}}::write (/home/kanru/mozilla/gecko/third_party/rust/fnv/lib.rs:106)
    #05: style::selector_map::{{impl}}::insert<style::invalidation::element::invalidation_map::Dependency> (/home/kanru/mozilla/gecko/servo/components/style/selector_map.rs:216)
    #06: style::stylist::Stylist::rebuild (/home/kanru/mozilla/gecko/third_party/rust/smallvec/lib.rs:770)
    #07: style::gecko::data::{{impl}}::flush_stylesheets<style::gecko::wrapper::GeckoElement> (/home/kanru/mozilla/gecko/servo/components/style/gecko/data.rs:188)
    #08: core::sync::atomic::atomic_store<usize> (/checkout/src/libcore/sync/atomic.rs:1404)
    #09: mozilla::ServoStyleSet::RebuildData() (/home/kanru/mozilla/gecko/layout/style/ServoStyleSet.cpp:1001)
    #10: mozilla::ServoStyleSet::UpdateStylist() (/home/kanru/mozilla/gecko/layout/style/ServoStyleSet.cpp:1157)
    #11: mozilla::ServoStyleSet::UpdateStylistIfNeeded() (/home/kanru/mozilla/gecko/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ServoStyleSet.h:322)
    #12: nsIDocument::FlushUserFontSet() (/home/kanru/mozilla/gecko/dom/base/nsDocument.cpp:13083)
    #13: RefPtr<nsPresContext>::get() const (/home/kanru/mozilla/gecko/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:284)
    #14: mozilla::PresShell::DoFlushPendingNotifications(mozilla::FlushType) (/home/kanru/mozilla/gecko/layout/base/PresShell.cpp:4065)
    #15: mozilla::PresShell::HandlePostedReflowCallbacks(bool) (/home/kanru/mozilla/gecko/layout/base/PresShell.cpp:4034)
    #16: RefPtr<nsPresContext>::get() const (/home/kanru/mozilla/gecko/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:284)
    #17: mozilla::PresShell::ResizeReflowIgnoreOverride(int, int, int, int) (/home/kanru/mozilla/gecko/layout/base/PresShell.cpp:1987)
    #18: mozilla::PresShell::ResizeReflow(int, int, int, int) (/home/kanru/mozilla/gecko/layout/base/PresShell.cpp:1915)
    #19: nsViewManager::DoSetWindowDimensions(int, int) (/home/kanru/mozilla/gecko/view/nsViewManager.cpp:193)
    #20: nsViewManager::SetWindowDimensions(int, int, bool) (/home/kanru/mozilla/gecko/view/nsViewManager.cpp:236)
    #21: nsDocumentViewer::SetBoundsWithFlags(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, unsigned int) (/home/kanru/mozilla/gecko/layout/base/nsDocumentViewer.cpp:2058)
    #22: nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int) (/home/kanru/mozilla/gecko/docshell/base/nsDocShell.cpp:6060)
    #23: nsWebBrowser::SetPositionAndSize(int, int, int, int, unsigned int) (/home/kanru/mozilla/gecko/toolkit/components/browser/nsWebBrowser.cpp:1394)
    #24: RefPtr<mozilla::widget::PuppetWidget>::get() const (/home/kanru/mozilla/gecko/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:284)
  }
}

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: New DMD output

Here is current DMD output for the Obama wikipedia page.

- record 1 is Stylist HashMaps (bug 1387958)

- record 6 is PerDocumentStyleData; I'm not sure where that is stored.

- record 7 is ComputedValues (bug 1390760)

- record 8 involves parsing and possibly SelectorImpls; I'm not sure where they are stored.

- record 12 is ElementData; I'm not sure where that is stored.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> Created attachment 8898088 [details]
> New DMD output
> 
> Here is current DMD output for the Obama wikipedia page.
> 
> - record 1 is Stylist HashMaps (bug 1387958)
> 
> - record 6 is PerDocumentStyleData; I'm not sure where that is stored.

This is the Rust side of the ServoStyleSet. It is owned here:

http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/layout/style/ServoStyleSet.h#551

> 
> - record 7 is ComputedValues (bug 1390760)
> 
> - record 8 involves parsing and possibly SelectorImpls; I'm not sure where
> they are stored.

These are the stylesheets. They are owned by the stylist, which is owned by the PerDocumentStyleData, which is owned by the ServoStyleSet.

Note that the hashmaps also point to some of the selectors. We should probably measure selectors from the stylesheets, and not from the hashmaps, since the former should have them all.

> 
> - record 12 is ElementData; I'm not sure where that is stored.

ElementData is the mServoData that lives on mozilla::dom::Element. I thought you added code already to measure it in the DOM traversal?

Comment 14

[Nicholas Nethercote [inactive]]

At this point Stylo memory usage doesn't show up particularly high in DMD's measurements of unreported memory. I think we can declare victory here.