1383977 - stylo: Measure Elements and ComputedValues

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

This is a decent chunk of Stylo memory usage. Measuring it also requires handling Arcs, which is non-trivial.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1383977 - stylo: Measure Elements and ComputedValues. .

The patch provides FFI access to Gecko's SeenPtrs type from Rust, in order to
record what has already been measured when measuring Arcs. (The SeenPtrs must
be initialized on the Gecko side because the same table is reused for measuring
all Elements within a window, because Elements can share ComputedValues.) I
have confirmed with DMD that this is working correctly.

The patch also introduces MallocSizeOfRepeats, which is like MallocSizeOf but
takes a SizeOfState, which holds a SeenPtrs table.

Review commit: https://reviewboard.mozilla.org/r/163210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163210/

Comment 2

[Nicholas Nethercote [inactive]]

I'm getting a Valgrind failure on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=865e4afd445964c86359b699eb1781d62c9d7e8b&selectedJob=119844863

> ==42523== Invalid write of size 4
> ==42523==    at 0xF9D1B25: Servo_Element_SizeOfExcludingThis (ServoBindingList
.h:23)
> ==42523==    by 0xE70C96D: mozilla::dom::Element::SizeOfExcludingThis(mozilla:
:SizeOfState&) const (Element.cpp:4152)
> ==42523==    by 0xE78772F: SizeOfIncludingThis (nsINode.h:339)
> ==42523==    by 0xE78772F: nsDocument::DocAddSizeOfExcludingThis(nsWindowSizes
*) const (nsDocument.cpp:12406)
> ...
> ==42523==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

So it's dereferencing a null pointer. My guess is that Element::mServoData is null. Should I be checking mServoData.Get() is non-null before calling Servo_Element_SizeOfExcludingThis()?

Comment 3

[Nicholas Nethercote [inactive]]

> > ==42523==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> 
> So it's dereferencing a null pointer. My guess is that Element::mServoData
> is null. Should I be checking mServoData.Get() is non-null before calling
> Servo_Element_SizeOfExcludingThis()?

I added an mServoData.Get() check and Valgrind is now happy. But now I'm getting a heap write hazard. (Sigh, this patch has everything.)

> [28.67s] #35 Analyzing Gecko_HaveSeenPtr ...
> Error: Field write PLDHashTable.mRemovedCount
> Location: _ZN12PLDHashTable3AddEPKvRKN7mozilla10fallible_tE$PLDHashEntryHdr* PLDHashTable::Add(void*, mozilla::fallible_t*) @ xpcom/ds/PLDHashTable.cpp#586
> Stack Trace:
> _ZN12nsTHashtableIN6detail14VoidPtrHashKeyEE8PutEntryEPKvRKN7mozilla10fallible_tE$EntryType* > nsTHashtable<EntryType>::PutEntry(nsTHashtable<EntryType>::KeyType, const fallible_t&) [with EntryType = detail::VoidPtrHashKey; nsTHashtable<EntryType>::KeyType = const void*; nsTHashtable<EntryType>::fallible_t = mozilla::fallible_t] @ obj-analyzed/dist/include/nsTHashtable.h#166
> _ZN12nsTHashtableI12nsPtrHashKeyIKvEE8PutEntryEPS1_RKN7mozilla10fallible_tE$nsTHashtable<nsPtrHashKey<T> >::EntryType* nsTHashtable<nsPtrHashKey<T> >::PutEntry(T*, const mozilla::fallible_t&) [with T = const void; nsTHashtable<nsPtrHashKey<T> >::EntryType = nsPtrHashKey<const void>] @ obj-analyzed/dist/include/nsTHashtable.h#570
> _ZN7mozilla8SeenPtrs11HaveSeenPtrEPKv$uint8 mozilla::SeenPtrs::HaveSeenPtr(void*) @ obj-analyzed/dist/include/mozilla/SizeOfState.h#35
> Gecko_HaveSeenPtr @ layout/style/ServoBindings.cpp#464

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8892277 [details]
Bug 1383977 - stylo: Measure Elements and ComputedValues. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163210/diff/1-2/

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8892277 [details]
Bug 1383977 - stylo: Measure Elements and ComputedValues. .

https://reviewboard.mozilla.org/r/163210/#review169044

Comment 6

[Manish Goregaokar [:manishearth]]

The IsMainThread assertion should have fixed the hazard, if not there's a bug in the hazard.

I assume HaveSeenPtr inserts the pointer into the table if not seen.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to Manish Goregaokar [:manishearth] from comment #6)
> The IsMainThread assertion should have fixed the hazard, if not there's a
> bug in the hazard.

It did fix it.

> I assume HaveSeenPtr inserts the pointer into the table if not seen.

Yes! There's a comment above the method in xpcom/base/SizeOfState.h:

> // Returns true if we have seen this pointer before, false otherwise. Also
> // remembers this pointer for later queries.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8892277 [details]
Bug 1383977 - stylo: Measure Elements and ComputedValues. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163210/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23fdfd155f72
stylo: Measure Elements and ComputedValues. r=manishearth.

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/23fdfd155f72