Closed Bug 1366936 Opened 7 years ago Closed 7 years ago

Add telemetry in MediaCache to monitor the maximum number of block owners

Categories

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

55 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

There is quite a heavy machinery used to keep track of multiple block owners in MediaCache, which could make sense if the same resources are used by different elements and/or pages.

But it would be interesting to see how true that is. If multi-ownership is rare, we could potentially explore other ways to deal with it (e.g.: Just don't share blocks!), which could simplify the code quite a bit, and hopefully make it less CPU-heavy.
... Or at least we could possibly micro-optimize Block::Owners to expect one owner; e.g., use `AutoTArray<BlockOwners, 1>` to reduce memory allocations in most cases.
Comment on attachment 8870271 [details]
Bug 1366936 - Telemetry: Watermark of number of block owners during MediaCache sessions -

https://reviewboard.mozilla.org/r/141730/#review145344

datareview+

::: toolkit/components/telemetry/Histograms.json:8350
(Diff revision 1)
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["gsquelart@mozilla.com"],
> +    "bug_numbers": [1366936],
> +    "expires_in_version": "60",
> +    "kind": "enumerated",
> +    "n_values": 32,

There is a hard limit of 32 owners I assume?
Attachment #8870271 - Flags: review?(francois) → review+
Comment on attachment 8870271 [details]
Bug 1366936 - Telemetry: Watermark of number of block owners during MediaCache sessions -

https://reviewboard.mozilla.org/r/141730/#review145344

> There is a hard limit of 32 owners I assume?

I see, thanks.
I've added a check so that we only report 0-31.
Comment on attachment 8870271 [details]
Bug 1366936 - Telemetry: Watermark of number of block owners during MediaCache sessions -

https://reviewboard.mozilla.org/r/141730/#review145344

> I see, thanks.
> I've added a check so that we only report 0-31.

And... I just got word that in fact "enumeration" handles too-big values by placing them in the last bucket (which is an extra bucket after the expected n_values buckets).
So I'll undo this last patch :-)
Comment on attachment 8870271 [details]
Bug 1366936 - Telemetry: Watermark of number of block owners during MediaCache sessions -

https://reviewboard.mozilla.org/r/141730/#review145456

I'm not sure this probe tell us anything actionable. Or maybe you can convince me otherwise?

This probe tells us the maximum number of owners a shared block has. What we really care about is how much memory we save by sharing blocks right? And that's a function of all the shared blocks, not just the single most-shared block.

So every time we share a block, can we increment a counter, and every time we unshare a block, can we decrement the counter, and track the high-water mark of the counter, and counter*BLOCK_SIZE should be the memory savings this feature is giving us? Or maybe there's a better way to report how much this feature helps us reduce memory use?
Attachment #8870271 - Flags: review?(cpearce)
Though maybe what we really want to report here is the ratio of shared blocks vs unshared blocks. Coupled with the high water mark for the size of the cache, we can work out how many bytes we're saving that way.
I think this number would already be useful on its own:
If it's '1' in 99% of cases, we could already take some action, e.g., make mOwners a `AutoTArray<BlockOwners, 1>` (though this particular optimization would have very little impact on our main-thread issue from bug 1361625).
And then we could study ways in which single-ownership can somehow be a fast path, with a slower fallback on multi-ownership when needed.

And if instead we get more "interesting" numbers that show a significant multi-ownership use, that's useful information telling us that dealing with it is in fact an important part of the job.

So I would still like to go ahead with it as-is...


But also I would open a separate bug to start adding you other suggestions.

As you said, we'd like to know how much memory we're saving, i.e., how much more memory we would need if we didn't share blocks.
Your counter suggestion should work, keeping tracks of memory saved by 2nd (and later) owners, but I think we would need to record the watermark of normal_watermark+memory_saved; and in ~MediaCache we would report the ratio of (normal+saved)/normal -- I think it's more useful that the raw number.
Though I'm scared that some uses may lead to strange numbers (e.g., a small page with lots of repeated resources would give us an unnaturally big ratio, though the memory amount itself could stay small). Maybe we could prevent this by only reporting the ratio when we've used some significant amount of memory, say 16MB.
What do you think?


And a thought:
Since we're reserving memory by blocks anyway, we don't need to worry about their actual content for the above telemetry.
However, maybe it would also be interesting to somehow monitor how much memory is wasted by non-full blocks? (To possibly make them smaller, at the cost of extra management of more blocks.)
And the reverse too: What would be the effect of bigger blocks?
Comment on attachment 8870271 [details]
Bug 1366936 - Telemetry: Watermark of number of block owners during MediaCache sessions -

https://reviewboard.mozilla.org/r/141730/#review145714

OK. We should follow up with more comprehensive telemetry.
Attachment #8870271 - Flags: review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/805340903ba8
Telemetry: Watermark of number of block owners during MediaCache sessions - r=cpearce,francois
https://hg.mozilla.org/mozilla-central/rev/805340903ba8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: