1377575 - Add telemetry for MemoryBlockCache memory usage watermark

Status: RESOLVED FIXED

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

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

MEDIACACHE_MEMORY_WATERMARK will record the highest memory usage of MemoryBlockCache per session.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1377575 - No need to explicitly print 'this' from MemoryBlockCache logging -

Because it is already printed by LOG() itself.

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

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1377575 - Rename MemoryBlockCache's mCombinedSizes to gCombinedSizes -

As it is a static (hence Global) member.

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

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -

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

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8882715 [details]
Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -

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

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8882715 [details]
Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -

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

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

https://reviewboard.mozilla.org/r/153800/#review159026

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8882714 [details]
Bug 1377575 - Rename MemoryBlockCache's mCombinedSizes to gCombinedSizes -

https://reviewboard.mozilla.org/r/153802/#review159028

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

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/

Comment 9

[François Marier [:francois]]

Comment on attachment 8882715 [details]
Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -

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

datareview+

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

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

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8882714 [details]
Bug 1377575 - Rename MemoryBlockCache's mCombinedSizes to gCombinedSizes -

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

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8882715 [details]
Bug 1377575 - MEDIACACHE_MEMORY_WATERMARK records the MemoryBlockCache memory use watermark -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153804/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b0acd667dc6
https://hg.mozilla.org/mozilla-central/rev/5caa90b684f3
https://hg.mozilla.org/mozilla-central/rev/4bdafa7daf7d