1390760 - stylo: Measure missing ComputedValues

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

Even after bug 1387956 -- which measures DOM elements and frame trees -- we are still failing to measured some ComputedValues (and hence also some style structs).

The two obvious possible reasons for this.

- There are more CVs stashed somewhere else.

- There aren't, but the existing memory reporting code is somehow buggy and missing some.

Here is a DMD record for some of them. It looks extremely similar to some of the records for CVs that *are* being measured.

bholley, any ideas?

> Unreported {
>   1,034 blocks in heap block record 12 of 15,617
>   281,248 bytes (281,248 requested / 0 slop)
>   Individual block sizes: 272 x 1,034
>   0.38% of the heap (9.51% cumulative)
>   1.08% of unreported (27.02% cumulative)
>   Allocated at {
>     #01: replace_malloc (/home/njn/moz/mi2/memory/replace/dmd/DMD.cpp:1303 (discriminator 2))
>     #02: alloc::heap::exchange_malloc (/checkout/src/liballoc/heap.rs:154)
>     #03: alloc::boxed::{{impl}}::new<servo_arc::ArcInner<style::gecko_properties::ComputedValues>> (/checkout/src/liballoc/boxed.rs:239)
>     #04: style::gecko_properties::{{impl}}::to_outer_helper (/home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-a432e0520cb2b0e0/out/gecko_properties.rs:45
> 8)
>     #05: style::gecko_properties::{{impl}}::to_outer (/home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-a432e0520cb2b0e0/out/gecko_properties.rs:448)
>     #06: style::gecko_properties::{{impl}}::new (/home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-a432e0520cb2b0e0/out/gecko_properties.rs:254)
>     #07: style::properties::{{impl}}::build (/home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-a432e0520cb2b0e0/out/properties.rs:136972)
>     #08: style::properties::apply_declarations<closure,core::iter::FlatMap<style::rule_tree::SelfAndAncestors, core::iter::FilterMap<core::iter::Rev<core::slice::Iter<(style::properties::P
> ropertyDeclaration, style::properties::declaration_block::Importance)>>, closure>, closure>> (/home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-a432e0520c
> b2b0e0/out/properties.rs:138422)
>     #09: style::properties::cascade (/home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-a432e0520cb2b0e0/out/properties.rs:138039)
>     #10: style::style_resolver::{{impl}}::cascade_style<style::gecko::wrapper::GeckoElement> (/home/njn/moz/mi2/servo/components/style/style_resolver.rs:477)
>     #11: style::style_resolver::{{impl}}::resolve_primary_style<style::gecko::wrapper::GeckoElement> (/home/njn/moz/mi2/servo/components/style/style_resolver.rs:115)
>     #12: style::style_resolver::{{impl}}::resolve_style<style::gecko::wrapper::GeckoElement> (/home/njn/moz/mi2/servo/components/style/style_resolver.rs:144)
>     #13: style::style_resolver::{{impl}}::resolve_style_with_default_parents::{{closure}}<style::gecko::wrapper::GeckoElement> (/home/njn/moz/mi2/servo/components/style/style_resolver.rs:1
> 79)
>     #14: style::style_resolver::with_default_parent_styles (geckoservo.cgu-1.rs:?)
>     #15: style::style_resolver::{{impl}}::resolve_style_with_default_parents<style::gecko::wrapper::GeckoElement> (/home/njn/moz/mi2/servo/components/style/style_resolver.rs:180)
>     #16: style::traversal::compute_style<style::gecko::wrapper::GeckoElement> (/home/njn/moz/mi2/servo/components/style/traversal.rs:676)
>     #17: style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure> (/home/njn/moz/mi2/servo/components/style/traversal.rs:486)
>     #18: style::gecko::traversal::{{impl}}::process_preorder<closure> (/home/njn/moz/mi2/servo/components/style/gecko/traversal.rs:40)
>     #19: style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (/home/njn/moz/mi2/servo/components/style/parallel.rs:223)
>     #20: rayon_core::scope::{{impl}}::execute_job_closure::{{closure}}<closure,()> (/home/njn/moz/mi2/third_party/rust/rayon-core/src/scope/mod.rs:354)
>     #21: std::panic::{{impl}}::call_once<(),closure> (/checkout/src/libstd/panic.rs:297)
>     #22: std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> (/checkout/src/libstd/panicking.rs:456)
>     #23: panic_abort::__rust_maybe_catch_panic (/checkout/src/libpanic_abort/lib.rs:42)
>   }
> }

Comment 1

[Bobby Holley (:bholley)]

Hm, looks like those are styles allocated for regular elements. Some questions:
* How are we finding the documents to traverse? Are we handling iframes, svg-as-image, that sort of thing? Or put another way, is the memory reporting machinery known to reach all the nsIDocuments in the system?
* Is it easy to run DMD under rr? If so, we could figure out the address of the CVs held alive, then work from there to the element they were created for. Next, we'd check whether the element still points to those CVs (which means we're failing to measure the element), or not (which means that the element has been restyled, but something else is holding its old CVs alive).


Cameron/Emilio, do you have any ideas for what we might be missing with an AllChildrenIterator traveral of the DOM?

Comment 2

[Nicholas Nethercote [inactive]]

Here's where the windows are iterated over:
http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/dom/base/nsWindowMemoryReporter.cpp#563-568

Here's where each window is measured:
http://searchfox.org/mozilla-central/source/dom/base/nsWindowMemoryReporter.cpp#317

Here is where documents are measured:
http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#13576-13592

The last one has some non-trivial logic about which documents to measure. It might be missing some.

Comment 3

[Nicholas Nethercote [inactive]]

Oh, and we do measure SVG documents, here:
http://searchfox.org/mozilla-central/source/image/VectorImage.cpp#395

Comment 4

[Nicholas Nethercote [inactive]]

> Here is where documents are measured:
> http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#13576-13592
> 
> The last one has some non-trivial logic about which documents to measure. It
> might be missing some.

I just looked into this and for the Obama wikipedia page there aren't any documents being skipped by the condition under the "Multiple global windows can share a document" comment. So that doesn't appear to be the problem.

Comment 5

[Bobby Holley (:bholley)]

Ok. If emilio and heycam don't have other ideas we may need to resort to a first-principles "who is holding this alive" approach.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Some questions:

 * Are we looking into NAC / generated content? (doesn't look like it)
 * Are we measuring the EagerPseudoStyles? (looks like it)
 * Are we measuring XBL documents and that kind of stuff? (looks like we might, but not sure)

Comment 7

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Some questions:
> 
>  * Are we looking into NAC / generated content? (doesn't look like it)

We're using an AllChildrenIterator, so we should.

>  * Are we measuring the EagerPseudoStyles? (looks like it)
>  * Are we measuring XBL documents and that kind of stuff? (looks like we
> might, but not sure)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> > Some questions:
> > 
> >  * Are we looking into NAC / generated content? (doesn't look like it)
> 
> We're using an AllChildrenIterator, so we should.

I see no mention of AllChildrenIterator in https://hg.mozilla.org/mozilla-central/rev/5f2f00d59868, did that land in another bug?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> > > Some questions:
> > > 
> > >  * Are we looking into NAC / generated content? (doesn't look like it)
> > 
> > We're using an AllChildrenIterator, so we should.
> 
> I see no mention of AllChildrenIterator in
> https://hg.mozilla.org/mozilla-central/rev/5f2f00d59868, did that land in
> another bug?

Yes, bug 1390404.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

We don't seem to be measuring visited styles. Do we?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> We don't seem to be measuring visited styles. Do we?

Ah, good point!

So Nick, we also need to measure: http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/layout/style/ServoTypes.h#250

How easy would it be to give those a separate category, i.e. we'd have dom, non-dom, and visited?

Comment 12

[Bobby Holley (:bholley)]

(These styles are just to handle this annoying visited privacy stuff. It would be very interesting to see if we're spending a lot of memory on them).

Comment 13

[Cameron McCormack (:heycam)]

Are we looking at styles for anonymous boxes?  If not, we should call AppendOwnedAnonBoxes() on each element's primary frame to find those.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #13)
> Are we looking at styles for anonymous boxes?  If not, we should call
> AppendOwnedAnonBoxes() on each element's primary frame to find those.

We should get those from the frame tree traversal.

Emilio, is the stack from comment 0 compatible with what we'd get when resolving visited style?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Yes, that's why I mentioned it.

Comment 16

[Nicholas Nethercote [inactive]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> >
> > We don't seem to be measuring visited styles. Do we?
> 
> Ah, good point!

I've written code to measure them for the Obama wikipedia page it's catching ~90% of the missing CVs, which is good! I've done a straight measurement and DMD says there's a lot of double-counting going on, so I guess I'll need to add some seen-ptrs checking.

> How easy would it be to give those a separate category, i.e. we'd have dom,
> non-dom, and visited?

That's straightforward, and the code I've written does it.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Measure ServoComputedData::visited_style

For the Obama wikipedia page, this covers about 85% of the unmeasured
ComputedValues structs. The about:memory output looks like this:

> +---2,443,648 B (02.41%) -- computed-values
> |   +--1,088,272 B (01.07%) -- dom
> |   +----945,744 B (00.93%) -- non-dom
> |   +----409,632 B (00.40%) -- visited

I'm not sure why some CVs are still being missed.

MozReview-Commit-ID: 1bYWwSi4ihn

Comment 18

[Bobby Holley (:bholley)]

(In reply to Nicholas Nethercote [:njn] from comment #17)
> Created attachment 8899336 [details] [diff] [review]
> Measure ServoComputedData::visited_style
> 
> For the Obama wikipedia page, this covers about 85% of the unmeasured
> ComputedValues structs. The about:memory output looks like this:
> 
> > +---2,443,648 B (02.41%) -- computed-values
> > |   +--1,088,272 B (01.07%) -- dom
> > |   +----945,744 B (00.93%) -- non-dom
> > |   +----409,632 B (00.40%) -- visited
> 
> I'm not sure why some CVs are still being missed.

Do they have the same DMD stacks as comment 0, or are they different?

Comment 19

[Nicholas Nethercote [inactive]]

> > I'm not sure why some CVs are still being missed.
> 
> Do they have the same DMD stacks as comment 0, or are they different?

On the Obama page there are still 230 unmeasured CVs. 210 of them have a stack
identical or almost identical to the one in comment 0.

11 of them have this stack involving SVG:

> #01: replace_malloc (memory/replace/dmd/DMD.cpp:1303 (discriminator 2))
> #02: alloc::heap::exchange_malloc (/checkout/src/liballoc/heap.rs:154)
> #03: alloc::boxed::{{impl}}::new<servo_arc::ArcInner<style::gecko_properties::ComputedValues>> (/checkout/src/liballoc/boxed.rs:239)
> #04: style::gecko_properties::{{impl}}::to_outer_helper (build/style-a432e0520cb2b0e0/out/gecko_properties.rs:458)
> #05: style::gecko_properties::{{impl}}::to_outer (build/style-a432e0520cb2b0e0/out/gecko_properties.rs:448)
> #06: style::gecko_properties::{{impl}}::default_values (build/style-a432e0520cb2b0e0/out/gecko_properties.rs:288)
> #07: style::gecko::media_queries::{{impl}}::new (servo/components/style/gecko/media_queries.rs:72)
> #08: style::gecko::data::{{impl}}::new (servo/components/style/gecko/data.rs:133)
> #09: alloc::boxed::{{impl}}::new<style::gecko::data::PerDocumentStyleData> (/checkout/src/liballoc/boxed.rs:239)
> #10: mozilla::ServoStyleSet::Init(nsPresContext*, nsBindingManager*) (layout/style/ServoStyleSet.cpp:111 (discriminator 1))
> #11: mozilla::PresShell::Init(nsIDocument*, nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) (layout/base/PresShell.cpp:962 (discriminator 3))
> #12: nsDocument::CreateShell(nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) (dom/base/nsDocument.cpp:3920)
> #13: nsDocumentViewer::InitPresentationStuff(bool) (layout/base/nsDocumentViewer.cpp:702 (discriminator 4))
> #14: nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool) (layout/base/nsDocumentViewer.cpp:960)
> #15: nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) (layout/base/nsDocumentViewer.cpp:677)
> #16: mozilla::image::SVGDocumentWrapper::OnStartRequest(nsIRequest*, nsISupports*) (image/SVGDocumentWrapper.cpp:248 (discriminator 6))
> #17: mozilla::image::VectorImage::OnStartRequest(nsIRequest*, nsISupports*) (image/VectorImage.cpp:1186 (discriminator 1))
> #18: mozilla::image::ImageFactory::CreateVectorImage(nsIRequest*, mozilla::image::ProgressTracker*, nsCString const&, mozilla::image::ImageURL*, unsigned int, unsigned int) (image/ImageFactory.cpp:289)
> #19: mozilla::image::ImageFactory::CreateImage(nsIRequest*, mozilla::image::ProgressTracker*, nsCString const&, mozilla::image::ImageURL*, bool, unsigned int) (image/ImageFactory.cpp:118)
> #20: PrepareForNewPart(nsIRequest*, nsIInputStream*, unsigned int, mozilla::image::ImageURL*, bool, mozilla::image::Image*, mozilla::image::ProgressTracker*, unsigned int) (image/imgRequest.cpp:1045)
> #21: imgRequest::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (image/imgRequest.cpp:1140 (discriminator 1))
> #22: nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (netwerk/base/nsBaseChannel.cpp:882 (discriminator 2))
> #23: nsInputStreamPump::OnStateTransfer() (netwerk/base/nsInputStreamPump.cpp:615 (discriminator 2))
> #24: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (crtstuff.c:?)

9 of them have this stack involving text:

> #01: replace_malloc (memory/replace/dmd/DMD.cpp:1303 (discriminator 2))
> #02: alloc::heap::exchange_malloc (/checkout/src/liballoc/heap.rs:154)
> #03: alloc::boxed::{{impl}}::new<servo_arc::ArcInner<style::gecko_properties::ComputedValues>> (/checkout/src/liballoc/boxed.rs:239)
> #04: style::gecko_properties::{{impl}}::to_outer_helper (build/style-a432e0520cb2b0e0/out/gecko_properties.rs:458)
> #05: style::gecko_properties::{{impl}}::to_outer (build/style-a432e0520cb2b0e0/out/gecko_properties.rs:448)
> #06: style::gecko_properties::{{impl}}::new (build/style-a432e0520cb2b0e0/out/gecko_properties.rs:254)
> #07: style::properties::{{impl}}::build (build/style-a432e0520cb2b0e0/out/properties.rs:137815)
> #08: geckoservo::glue::Servo_ComputedValues_Inherit (servo/ports/geckolib/glue.rs:1844)
> #09: ResolveStyleForTextOrFirstLetterContinuation(RawServoStyleSet const*, mozilla::ServoStyleContext&, nsIAtom*) (layout/style/ServoStyleSet.cpp:392)
> #10: mozilla::ServoStyleSet::ResolveStyleForText(nsIContent*, mozilla::ServoStyleContext*) (layout/style/ServoStyleSet.cpp:411)
> #11: mozilla::StyleSetHandle::Ptr::ResolveStyleForText(nsIContent*, nsStyleContext*) (mozilla/StyleSetHandleInlines.h:118 (discriminator 13))
> #12: nsCSSFrameConstructor::ResolveStyleContext(nsStyleContext*, nsIContent*, nsFrameConstructorState*, mozilla::dom::Element*) (layout/base/nsCSSFrameConstructor.cpp:5239 (discriminator 1))
> #13: nsCSSFrameConstructor::ResolveStyleContext(nsIFrame*, nsIContent*, nsIContent*, nsFrameConstructorState*) (layout/base/nsCSSFrameConstructor.cpp:5172)
> #14: nsCSSFrameConstructor::ResolveStyleContext(nsCSSFrameConstructor::InsertionPoint const&, nsIContent*, nsFrameConstructorState*) (layout/base/nsCSSFrameConstructor.cpp:5187)
> #15: nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&) (layout/base/nsCSSFrameConstructor.cpp:5845)
> #16: nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) (layout/base/nsCSSFrameConstructor.cpp:11400 (discriminator 1))
> #17: nsCSSFrameConstructor::ConstructFrameWithAnonymousChild(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsFrameItems&, nsContainerFrame* (*)(nsIPresShell*, nsStyleContext*), nsContainerFrame* (*)(nsIPresShell*, nsStyleContext*), nsICSSAnonBoxPseudo*, bool) (layout/base/nsCSSFrameConstructor.cpp:5451)
> #18: nsCSSFrameConstructor::ConstructOuterSVG(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) (layout/base/nsCSSFrameConstructor.cpp:5468)
> #19: nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) (layout/base/nsCSSFrameConstructor.cpp:2693)
> #20: nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool, bool, TreeMatchContext*) (layout/base/nsCSSFrameConstructor.cpp:8113)
> #21: nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool, TreeMatchContext*) (o64sty/layout/base/../../../layout/base/nsCSSFrameConstructor.h:275)
> #22: nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (layout/base/nsCSSFrameConstructor.cpp:8005)
> #23: mozilla::PresShell::Initialize(int, int) (layout/base/PresShell.cpp:1807 (discriminator 1))
> #24: nsContentSink::StartLayout(bool) (dom/base/nsContentSink.cpp:1281)

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> > > I'm not sure why some CVs are still being missed.
> > 
> > Do they have the same DMD stacks as comment 0, or are they different?
> 
> On the Obama page there are still 230 unmeasured CVs. 210 of them have a
> stack
> identical or almost identical to the one in comment 0.

I'm not sure what can cause these ones, maybe missing frames during restyles? That'd be pretty weird... Can you paste another stack from them? I suspect these may be alive computed values from getComputedStyle and similar. The stack would be relatively close. 

> 11 of them have this stack involving SVG:
> 
> [...]

These are the default computed values, which live on the rust-side Device: https://github.com/servo/servo/blob/10779f02519b193f07df997ab013ce7ca45cf656/components/style/gecko/media_queries.rs#L45

> 9 of them have this stack involving text:
> [...]

I'm not 100% sure, but I suspect these may be reparented contexts that are kept alive on the various anonymous box caches. You may need to loop through ServoStyleContext::mNextInheritingAnonBoxStyle and mNextLazyPseudoStyle if we don't do that already.

Comment 21

[Nicholas Nethercote [inactive]]

> I'm not sure what can cause these ones, maybe missing frames during
> restyles? That'd be pretty weird... Can you paste another stack from them? I
> suspect these may be alive computed values from getComputedStyle and
> similar. The stack would be relatively close. 

The only difference between the stacks is that the `top_down_dom` frame is sometimes repeated once or twice.

Comment 22

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50fd4bb9897fb179ceeca364b04469c19900cc60
Bug 1390760 - Measure ServoComputedData::visited_style. r=bholley.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50fd4bb9897f
Measure ServoComputedData::visited_style. r=bholley.

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/50fd4bb9897f