Add telemetry for MemoryBlockCache memory usage watermark

RESOLVED FIXED in Firefox 56

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

(Blocks: 1 bug)

56 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

7 months ago
MEDIACACHE_MEMORY_WATERMARK will record the highest memory usage of MemoryBlockCache per session.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

7 months ago
mozreview-review
Comment on attachment 8882713 [details]
Bug 1377575 - No need to explicitly print 'this' from MemoryBlockCache logging -

https://reviewboard.mozilla.org/r/153800/#review159026
Attachment #8882713 - Flags: review?(cpearce) → review+

Comment 7

7 months ago
mozreview-review
Comment on attachment 8882714 [details]
Bug 1377575 - Rename MemoryBlockCache's mCombinedSizes to gCombinedSizes -

https://reviewboard.mozilla.org/r/153802/#review159028
Attachment #8882714 - Flags: review?(cpearce) → review+

Comment 8

7 months ago
mozreview-review
Comment on attachment 8882715 [details]
Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -

https://reviewboard.mozilla.org/r/153804/#review159030

::: dom/media/MemoryBlockCache.cpp:81
(Diff revision 3)
> +      mozilla::services::GetObserverService();
> +    if (observerService) {
> +      observerService->AddObserver(
> +        gMemoryBlockCacheTelemetry, "profile-change-teardown", true);
> +    }
> +  }

Do you need to do a ClearOnShutdown(gMemoryBlockCacheTelementry) here so that the singleton is considered freed by the leak tracker?

::: dom/media/MemoryBlockCache.cpp:83
(Diff revision 3)
> +      observerService->AddObserver(
> +        gMemoryBlockCacheTelemetry, "profile-change-teardown", true);
> +    }
> +  }
> +
> +  // Udpate watermark if needed, report current watermark.

s/Udpate/Update/
Attachment #8882715 - Flags: review?(cpearce) → review+
Comment on attachment 8882715 [details]
Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -

https://reviewboard.mozilla.org/r/153804/#review159184

datareview+
Attachment #8882715 - Flags: review?(francois) → review+
(Assignee)

Comment 10

7 months ago
mozreview-review-reply
Comment on attachment 8882715 [details]
Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -

https://reviewboard.mozilla.org/r/153804/#review159030

> Do you need to do a ClearOnShutdown(gMemoryBlockCacheTelementry) here so that the singleton is considered freed by the leak tracker?

Good question.
Looking at the documentation for ClearOnShutdown, I should indeed call it to ensure we don't leak gMemoryBlockCacheTelemetry.

I was thinking of just resetting the pointer from Observe() when we get the "profile-change-teardown" notification, but we would risk re-creating it (and then leaking it) in case some MediaBlockCache work would racily happen after that notification. So I'll just add the ClearOnShutdown.

> s/Udpate/Update/

Tahnks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

7 months ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b0acd667dc6
No need to explicitly print 'this' from MemoryBlockCache logging - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/5caa90b684f3
Rename MemoryBlockCache's mCombinedSizes to gCombinedSizes - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/4bdafa7daf7d
MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark - r=cpearce,francois

Comment 15

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b0acd667dc6
https://hg.mozilla.org/mozilla-central/rev/5caa90b684f3
https://hg.mozilla.org/mozilla-central/rev/4bdafa7daf7d
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.