Closed Bug 1473735 Opened Last year Closed Last year

We should have memory reporting for MatchMedia (media lists)

Categories

(Core :: DOM: CSS Object Model, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bzbarsky, Assigned: heycam)

References

Details

Attachments

(1 file)

I have been poking around with DMD and here's what I see for one of my processes.  Note that I'm logging only partial, not full stacks, so not all allocations get stacks.

Unreported {
  14,719 blocks in heap block record 8 of 7,198
  4,239,072 bytes (4,239,072 requested / 0 slop)
  Individual block sizes: 288 x 14,719
  0.78% of the heap (26.44% cumulative)
  1.89% of unreported (64.14% cumulative)
  Allocated at {
    #01: replace_malloc(unsigned long) (DMD.cpp:1265, in libmozglue.dylib)
    #02: __rdl_alloc (in XUL) + 30
    #03: _$LT$alloc..raw_vec..RawVec$LT$T$C$$u20$A$GT$$GT$::double::h9de5f38c12436c12 (raw_vec.rs:328, in XUL)
    #04: style::media_queries::media_list::MediaList::parse::h598387075440015d (vec.rs:1743, in XUL)
    #05: Servo_MediaList_SetText (raw_vec.rs:204, in XUL)
    #06: mozilla::dom::MediaList::Create(nsTSubstring<char16_t> const&, mozilla::dom::CallerType) (nsTSubstring.h:77, in XUL)
    #07: mozilla::dom::MediaQueryList::MediaQueryList(nsIDocument*, nsTSubstring<char16_t> const&, mozilla::dom::CallerType) (RefPtr.h:64, in XUL)
    #08: nsIDocument::MatchMedia(nsTSubstring<char16_t> const&, mozilla::dom::CallerType) (RefPtr.h:112, in XUL)
  }
}

Unreported {
  10,106 blocks in heap block record 12 of 7,198
  1,778,656 bytes (1,697,808 requested / 80,848 slop)
  Individual block sizes: 176 x 10,106
  0.33% of the heap (28.41% cumulative)
  0.79% of unreported (68.92% cumulative)
  Allocated at {
    #01: replace_malloc(unsigned long) (DMD.cpp:1265, in libmozglue.dylib)
    #02: moz_xmalloc (mozalloc.cpp:71, in libmozglue.dylib)
    #03: nsIDocument::MatchMedia(nsTSubstring<char16_t> const&, mozilla::dom::CallerType) (mozalloc.h:156, in XUL)
    #04: mozilla::dom::Window_Binding::matchMedia(JSContext*, JS::Handle<JSObject*>, nsGlobalWindowInner*, JSJitMethodCallArgs const&) (.WindowBinding.cpp:3027, in XUL)
    #05: bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:3319, in XUL)
  }
}

That shows ~10k allocations of MediaQueryList instances from nsIDocument::MatchMedia and ~15k allocations of Rust vecs under the Servo_MediaList_SetText call the MediaQueryList constructor makes.  Keep in mind that both are lower bounds, given the  partial stacks.

We should ideally memory-report this.  Even better would be understanding why all these things are live...
I mean, presumably they're all live because they have "change" listeners attached and hence are keeping themselves alive...  The question is what site is messing up by adding them all.  This is where a memory reporter might be handy.
So I found one site that allocates a bunch of these things, by running the script at https://s1.wp.com/_static/??-eJyVUe1OwzAMfCFSj6Hx8QPxKChLvc5pvrCTlb09qaCjoCnSftk6352dC0xJmRgyhgxWoMcTGUyfnZU7WI18UcmVgYKAoxEFPgoWPOrQO+SFTMG40tdhNTLR+ypUjMmdO0+h4bhwv53tvGIPMlHCW0R/gIZwon7ALIClTuNIqJyeIKNPTmf8hzd8dF9fpfaawWvJyLVT8YTMNEdwwW50yKzN2Dpfm0wxzKJLdy1/Sb+Rr3FbP47PP6Vbs65lrDkWQQcWc6p3qQVo3TeSeMzqodvAe90Mh8i+wU9Rsjo4TQxy1ExhWGoVvfnX+932abPdvTw/2i8E5v+9 which does this bit in Firefox-specific code:

    var mediaQueryBinarySearch = function (property, unit, a, b, maxIter, epsilon) {
....
        function binarySearch(a, b, maxIter) {
            var mid = (a + b) / 2;
            if (maxIter <= 0 || b - a < epsilon) {
                return mid;
            }
            var query = "(" + property + ":" + mid + unit + ")";
            if (matchMedia(query).matches) {
                return binarySearch(mid, b, maxIter - 1);
            } else {
                return binarySearch(a, mid, maxIter - 1);
            }
        }

while to determine the zoom level by calling:

        var zoom = mediaQueryBinarySearch('min--moz-device-pixel-ratio', '', 0, 10, 20, 0.0001);

All those MediaQueryLists are dead immediately, so the next GC should kill them.  But before that happens thousands can build up.

I tried adding [ProbablyShortLivingWrapper] to MediaQueryList but that did not seem to really help...
I'll look at the memory reporting.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Priority: -- → P3
Depends on: 1473771
Comment on attachment 8990194 [details]
Bug 1473735 - Add memory reporting for MediaQueryLists.

https://reviewboard.mozilla.org/r/255222/#review262130

::: layout/style/MediaList.h:72
(Diff revision 1)
>  
> +  size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const;
> +
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
> +  {
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);

This is fine as long as the callers actually own the media list. Not sure if a comment to that effect would be worth it.

::: servo/ports/geckolib/glue.rs:3868
(Diff revision 1)
> +        Some(malloc_enclosing_size_of.unwrap()),
> +        None
> +    );
> +
> +    Locked::<MediaList>::as_arc(&list)
> +        .with_arc(|list| {

I guess you need with_arc so you can call the `servo_arc::Arc` function?

Maybe we should add the implementation for RawOffsetArc instead?

In any case, uber-nit: I'd maybe move the with_arc to the previous line, and deindent the rest, though I don't feel too strongly about it.
Attachment #8990194 - Flags: review?(emilio) → review+
Comment on attachment 8990194 [details]
Bug 1473735 - Add memory reporting for MediaQueryLists.

https://reviewboard.mozilla.org/r/255222/#review262130

> This is fine as long as the callers actually own the media list. Not sure if a comment to that effect would be worth it.

For the C++ MediaList object, the only way we can share it is by having multiple JS references to it, as far as I can tell.  So it's probably not worth mentioning.
Pushed by cam@mcc.id.au:
https://hg.mozilla.org/integration/autoland/rev/1df83f663c81
Add memory reporting for MediaQueryLists. r=emilio
https://hg.mozilla.org/mozilla-central/rev/1df83f663c81
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.