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.

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.

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.

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

Comment on attachment 8873692 [details] Bug 1369322 - MediaResourceIndex follows ShouldCacheReads recommendation - https://reviewboard.mozilla.org/r/145088/#review149946

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.

https://hg.mozilla.org/mozilla-central/rev/d6fa3a538265 https://hg.mozilla.org/mozilla-central/rev/26f5db4ebfaf