Closed Bug 1281964 Opened 3 years ago Closed 2 years ago

hook up stylo memory reporters

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: heycam, Assigned: njn)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
Priority: -- → P3
I can take this, though I probably won't get to it until January.
Assignee: nobody → n.nethercote
Nick, do you have cycles for this now?
Flags: needinfo?(n.nethercote)
I've done a small amount here, and I'm planning to get it done this quarter. Apologies for the delays.
Flags: needinfo?(n.nethercote)
Sounds good! Q2 sounds like the perfect timeframe for us.
Depends on: 1353948
Depends on: 1353998
Attached file DMD output (obsolete) —
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)
> Here is the top 20 records from DMD

bz, you like looking at output like this...
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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?
> 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.
(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.
Blocks: stylo-release
No longer blocks: stylo
Depends on: 1383977
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)
  }
}
Depends on: 1387956, 1387958
Depends on: 1390404
Depends on: 1390760
Attached file 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.
Attachment #8874327 - Attachment is obsolete: true
(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?
Depends on: 1398737
Depends on: 1399758
Depends on: 1400769
Depends on: 1400078
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.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.