stylo: Measure missing ComputedValues

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

8.05 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 months ago
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)
>   }
> }
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 2

3 months ago
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.
(Assignee)

Comment 3

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

Comment 4

3 months ago
> 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.
Priority: -- → P2
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.
Flags: needinfo?(emilio+bugs)
Flags: needinfo?(cam)
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)
(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)
(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?
(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.
We don't seem to be measuring visited styles. Do we?
Flags: needinfo?(emilio+bugs)
(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?
(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).
Are we looking at styles for anonymous boxes?  If not, we should call AppendOwnedAnonBoxes() on each element's primary frame to find those.
Flags: needinfo?(cam)
(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?
Flags: needinfo?(emilio+bugs)
Yes, that's why I mentioned it.
Flags: needinfo?(emilio+bugs)
(Assignee)

Comment 16

3 months ago
(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.
(Assignee)

Comment 17

3 months ago
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.

MozReview-Commit-ID: 1bYWwSi4ihn
Attachment #8899336 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #8899336 - Flags: review?(bobbyholley) → review+
(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?
(Assignee)

Comment 19

3 months ago
> > 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)
(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.
(Assignee)

Comment 21

3 months ago
> 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.
(Assignee)

Comment 22

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

Comment 23

3 months ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50fd4bb9897f
Measure ServoComputedData::visited_style. r=bholley.
https://hg.mozilla.org/mozilla-central/rev/50fd4bb9897f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.