Let MediaResource subclasses opt in/out of MediaResourceIndex caching

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

(Blocks 1 bug)

55 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

Following bug 1368837, MediaResourceIndex caches data ahead of reads, to hopefully avoid calling the underlying MediaResource's read functions over and over, as they usually perform locks and/or IOs.

But some MediaResource subclasses are lockless and fully memory-based, in which case adding a cache probably makes things a bit worse.

So this bug will add an virtual function that lets MediaResource subclasses choose whether caching them makes sense.
Summary: Let MediaSource subclasses to opt in/out of MediaResourceIndex caching → Let MediaSource subclasses opt in/out of MediaResourceIndex caching
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8873691 [details]
Bug 1369322 - MediaResource::ShouldCacheReads to indicate usefulness of caching -

https://reviewboard.mozilla.org/r/145086/#review149908

We spoke in person about this, but putting comments in here so there's a paper trail.

::: dom/media/MediaResource.h:237
(Diff revision 1)
> +  // 8*N+2 = Resource uses IOs, and partitions its data by blocks of size 8*N,
> +  //         and aligned at 8*N boundaries.
> +  // The partitioning information may be useful when trying to read and/or
> +  // cache data, to avoid crossing boundaries when possible, as each boundary
> +  // crossing could bring extra processing/locking/IOs.
> +  virtual uint32_t ReadStrategyHint() = 0;

I think this is a bad API design because it returns multiple things in a single value, and it's also returning int values instead of something more obvious, such as an enum etc.

The API is also needlessly complex as it requires bit mashing to use. It's like an API designed by a C programmer! :P

I also don't think we nececssarily need the MediaResource to tell us the best size to cache.

We're better to have a single API, bool ShouldCache() or somesuch. If we want the MediaResource subclass to tell us the best size to cache, we can change the API to return the cache size hint, or 0 if we shouldn't cache.
Attachment #8873691 - Flags: review?(cpearce) → review-

Comment 4

2 years ago
mozreview-review
Comment on attachment 8873692 [details]
Bug 1369322 - MediaResourceIndex follows ShouldCacheReads recommendation -

https://reviewboard.mozilla.org/r/145088/#review149906

::: dom/media/MediaResource.h:912
(Diff revision 1)
>    // Select the next power of 2 (in range 32B-128KB, or 0 -> no cache)
> -  static uint32_t SelectCacheSize(uint32_t aHint)
> +  // If aReadStrategyHint recommends a size&alignment, stay under it.
> +  static uint32_t SelectCacheSize(uint32_t aSizeHint,
> +                                  uint32_t aReadStrategyHint)
>    {
> -    if (aHint == 0) {
> +    if ((aReadStrategyHint & 8u) == 0) {

This branch here means that all the "return 2"s you added in the prevous patch are eqauivalent to "return 0". I think it's bad to have multiple return values that mean the same thing but which have different semantics.
Attachment #8873692 - Flags: review?(cpearce) → review-
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8873691 [details]
Bug 1369322 - MediaResource::ShouldCacheReads to indicate usefulness of caching -

https://reviewboard.mozilla.org/r/145086/#review149908

> I think this is a bad API design because it returns multiple things in a single value, and it's also returning int values instead of something more obvious, such as an enum etc.
> 
> The API is also needlessly complex as it requires bit mashing to use. It's like an API designed by a C programmer! :P
> 
> I also don't think we nececssarily need the MediaResource to tell us the best size to cache.
> 
> We're better to have a single API, bool ShouldCache() or somesuch. If we want the MediaResource subclass to tell us the best size to cache, we can change the API to return the cache size hint, or 0 if we shouldn't cache.

"API designed by a C programmer" -- Wow there, no need for insults! :-P

But you're right, I tried to pack too much in one number, including a cache size hint that's not that critical.

So I'll change it to `bool ShouldCacheReads()`. "Reads" to emphasize that we're encouraging caching resource-cached data (to reduce the number of uncached reads), but not other parts of the MediaResource API.
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8873692 [details]
Bug 1369322 - MediaResourceIndex follows ShouldCacheReads recommendation -

https://reviewboard.mozilla.org/r/145088/#review149906

> This branch here means that all the "return 2"s you added in the prevous patch are eqauivalent to "return 0". I think it's bad to have multiple return values that mean the same thing but which have different semantics.

Yeah, that was just wrong, I meant `% 8u` (or `& 7u`).
Anyway, this will just go away with the better API...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8873691 [details]
Bug 1369322 - MediaResource::ShouldCacheReads to indicate usefulness of caching -

https://reviewboard.mozilla.org/r/145086/#review149944
Attachment #8873691 - Flags: review?(cpearce) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8873692 [details]
Bug 1369322 - MediaResourceIndex follows ShouldCacheReads recommendation -

https://reviewboard.mozilla.org/r/145088/#review149946
Attachment #8873692 - Flags: review?(cpearce) → review+

Comment 11

2 years ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6fa3a538265
MediaResource::ShouldCacheReads to indicate usefulness of caching - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/26f5db4ebfaf
MediaResourceIndex follows ShouldCacheReads recommendation - r=cpearce
Thank you Chris for your review and suggestion for a simpler API.
Summary: Let MediaSource subclasses opt in/out of MediaResourceIndex caching → Let MediaResource subclasses opt in/out of MediaResourceIndex caching
https://hg.mozilla.org/mozilla-central/rev/d6fa3a538265
https://hg.mozilla.org/mozilla-central/rev/26f5db4ebfaf
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.