Closed Bug 1399758 Opened 2 years ago Closed 2 years ago

stylo: Measure ImageValue objects

Categories

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

56 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(2 files, 1 obsolete file)

They're significant for gmail.
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 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?
Attachment #8907970 - Flags: review?(cam) → review+
> > +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.
Priority: -- → P3
Attachment #8907969 - Attachment is obsolete: true
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1536fa69bad4
Measure ImageValue objects. r=heycam.
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
Flags: needinfo?(n.nethercote)
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?
Flags: needinfo?(n.nethercote) → needinfo?(sphink)
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.
Flags: needinfo?(sphink)
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.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
heycam: the "take 2" patch should be pushed to autoland once #18550 merges. Thanks!
Flags: needinfo?(cam)
https://hg.mozilla.org/mozilla-central/rev/3816b3b4f793
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Sorry for missing this needinfo. emilio gave you better advice than I probably would have.
You need to log in before you can comment on or make changes to this bug.