1399758 - stylo: Measure ImageValue objects

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

They're significant for gmail.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Measure PropertyDeclaration more thoroughly.

This patch replaces the handwritten MallocSizeOf implementation for
PropertyDeclaration with a derived one, which gives much more thorough
measurement.

This requires (a) deriving MallocSizeOf for a *lot* of additional types (most
of which already have `derive(HeapSizeOf)` in Servo builds), and (b)
implementing MallocSizeOf for a few more types in the `malloc_size_of` crate.

These changes would significantly improve the reporting coverage for gmail if
it weren't for the fact that SpecifiedUrl isn't measured due to a lack of
clarity about its fields; that can be fixed as a follow-up once bug 1397971 has
landed.

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1399758 - Measure ImageValue objects. .

We have about 11,500 of these when loading gmail in a Stylo-enabled build, from
SpecifiedUrls; the objects themselves account for about 1.3 MiB of memory, and the
strings within them about 2.9 MiB.

We also have a very small number of them on the Gecko side.

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

Comment 3

[Nicholas Nethercote [inactive]]

Oh, please ignore the "Measure PropertyDeclaration more thoroughly." patch. That's https://github.com/servo/servo/pull/18495, which has been r+'d and will land soon; the patch got pulled into this bug because of multi-repo shenanigans.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8907970 [details]
Bug 1399758 - Measure ImageValue objects. .

https://reviewboard.mozilla.org/r/179642/#review184880

::: layout/style/ServoBindings.cpp:1530
(Diff revision 1)
>    RefPtr<css::ImageValue> value(
>      new css::ImageValue(url, do_AddRef(aURI.mExtraData)));
>    return value.forget().take();
>  }
>  
> +MOZ_DEFINE_MALLOC_ENCLOSING_SIZE_OF(GeckoImageValueMallocSizeOf)

Why ENCLOSING?

Comment 5

[Nicholas Nethercote [inactive]]

> > +MOZ_DEFINE_MALLOC_ENCLOSING_SIZE_OF(GeckoImageValueMallocSizeOf)
> 
> Why ENCLOSING?

Because I auto-completed and didn't pay sufficient attention :)  I will change it to MOZ_DEFINE_MALLOC_SIZE_OF.

Comment 6

[Nicholas Nethercote [inactive]]

https://github.com/servo/servo/pull/18515 is the Servo side.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8907970 [details]
Bug 1399758 - Measure ImageValue objects. .

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1536fa69bad4
Measure ImageValue objects. r=heycam.

Comment 9

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

Backed out for Hazard failure:

https://hg.mozilla.org/integration/autoland/rev/f7b896fc877805e3d3a51376f5f22b522f2e448a
https://hg.mozilla.org/integration/autoland/rev/a7f12e31f7e02368b00d72ac73bf3c661d55d0ad

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1536fa69bad4d6ee7f1f4c265fab57504f6263d9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=131210045&repo=autoland

[31.80s] #121 Analyzing Gecko_ImageValue_SizeOfIncludingThis ...
Error: Indirect call aMallocSizeOf
Location: _ZNK7mozilla3css10ImageValue19SizeOfIncludingThisEPFmPKvE$uint64 mozilla::css::ImageValue::SizeOfIncludingThis((uint64)(void*)*) const @ layout/style/nsCSSValue.cpp#3081
Stack Trace:
Gecko_ImageValue_SizeOfIncludingThis @ layout/style/ServoBindings.cpp#1535

Comment 10

[Nicholas Nethercote [inactive]]

sfink: I added Gecko_ImageValue_SizeOfIncludingThis(), which just calls ImageValue::SizeOfIncludingThis() on its |ImageValue*| parameter. It just measures memory usage, and so doesn't do any heap writes. I read https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_heap_write_hazard_failure but am not sure how to modify analyzeHeapWrites.js. Can you please advise?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Yeah, but it cannot reason abut a random function, and aMallocSizeOf could (in theory) do all sorts of random stuff.

We could whitelist it manually, I guess, but you can also MOZ_ASSERT(NS_IsMainThread()) to make the analysis happy.

Comment 12

[Nicholas Nethercote [inactive]]

https://github.com/servo/servo/pull/18550 for the second attempt.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (take 2) - Measure ImageValue objects

We have about 11,500 of these when loading gmail in a Stylo-enabled build, from
SpecifiedUrls; the objects themselves account for about 1.3 MiB of memory, and
the strings within them about 2.9 MiB.

We also have a very small number of them on the Gecko side.

Comment 14

[Nicholas Nethercote [inactive]]

heycam: the "take 2" patch should be pushed to autoland once #18550 merges. Thanks!

Comment 15

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/autoland/rev/3816b3b4f793fc3d02b8e22af1a4ddeceff3ea09

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3816b3b4f793

Comment 17

[Steve Fink [:sfink] [:s:]]

Sorry for missing this needinfo. emilio gave you better advice than I probably would have.