Closed Bug 1377575 Opened 5 years ago Closed 5 years ago

Add telemetry for MemoryBlockCache memory usage watermark

Categories

(Core :: Audio/Video: Playback, enhancement)

Product:
Core
Component:
Audio/Video: Playback
Version:
56 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(3 files)

Bug 1377575 - No need to explicitly print 'this' from MemoryBlockCache logging -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1377575 - Rename MemoryBlockCache's mCombinedSizes to gCombinedSizes -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -
59 bytes, text/x-review-board-request
cpearce
: review+
francois
: review+
Details 
MEDIACACHE_MEMORY_WATERMARK will record the highest memory usage of MemoryBlockCache per session.
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 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 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+
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!
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
You need to log in before you can comment on or make changes to this bug.