Closed Bug 1369322 Opened 7 years ago Closed 7 years ago

Let MediaResource subclasses opt in/out of MediaResourceIndex caching

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(2 files)

Bug 1369322 - MediaResource::ShouldCacheReads to indicate usefulness of caching -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1369322 - MediaResourceIndex follows ShouldCacheReads recommendation -
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
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 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 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-
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.
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 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 on attachment 8873692 [details]
Bug 1369322 - MediaResourceIndex follows ShouldCacheReads recommendation -

https://reviewboard.mozilla.org/r/145088/#review149946
Attachment #8873692 - Flags: review?(cpearce) → review+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: