Closed Bug 1368837 Opened 7 years ago Closed 7 years ago

Add lockless cache to MediaResourceIndex

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

(10 files)

Bug 1368837 - Implement SourceBufferResource::GetCachedDataEnd -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - MockMediaResource::GetCachedDataEnd should return the offset if out of range -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - MockMediaResource should fread individual bytes -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - WaveTrackDemuxer should copy the MediaResource* instead of a whole MediaResourceIndex -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - MediaResourceIndex::ReadAt tries to cache last-read block -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - media.cache.resource-index controls the MediaResourceIndex cache size -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - Replace debugging ReadAt with CachedReadAt code -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - Convert printfs into MOZ_LOG(MediaResourceIndex) -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - Document that MediaResource::GetCachedDataEnd should return aOffset when out of cache -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1368837 - BufferMediaResource::GetCachedDataEnd should return aOffset when out of cache -
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
While working on bug 1361625 and dependentes, timing experiments have shown that part of the issue around MediaCache-related main-thread jank is due to the sheer number of locking operations done when the demuxers&decoders access their data in small chunks, sometimes byte-by-byte.

Even though these reads mostly happen off the main thread, they do cause main thread jank simply because of all the lock request (and subsequent servicing) creating a traffic jam, which then delays lock requests on the main thread (which mostly happen when data arrives from the network, and then when the MediaCache updates its state.)

If it was possible to cache some of the data that is about to be read, and then serve it in a lockless manner, it would reduce the number of locks massively, and hopefully would free up the MediaCache lock enough to have a beneficial impact.

The best place to do this is in MediaResourceIndex, because each instance is guaranteed to only have one user in one thread (or task queue), there is no need to worry about locking when accessing its member data.
So it we could cache data, some reads could just happen from the local cache, without needing any locks or IOs from MediaResource, MediaCache, nor FileBlockCache.

One concern is that we need to get the same effects (most importantly blocking or not blocking) when a read is requested. To help with this, we can rely on the MediaResource 'CachedData' operations when attempting to read data in advance (to fill the cache).
Comment on attachment 8872811 [details]
Bug 1368837 - Implement SourceBufferResource::GetCachedDataEnd -

https://reviewboard.mozilla.org/r/144316/#review148174
Attachment #8872811 - Flags: review?(cpearce) → review+
Comment on attachment 8872812 [details]
Bug 1368837 - MockMediaResource::GetCachedDataEnd should return the offset if out of range -

https://reviewboard.mozilla.org/r/144318/#review148176
Attachment #8872812 - Flags: review?(cpearce) → review+
Comment on attachment 8872813 [details]
Bug 1368837 - MockMediaResource should fread individual bytes -

https://reviewboard.mozilla.org/r/144320/#review148178
Attachment #8872813 - Flags: review?(cpearce) → review+
Comment on attachment 8872814 [details]
Bug 1368837 - WaveTrackDemuxer should copy the MediaResource* instead of a whole MediaResourceIndex -

https://reviewboard.mozilla.org/r/144322/#review148180
Attachment #8872814 - Flags: review?(cpearce) → review+
Comment on attachment 8872815 [details]
Bug 1368837 - MediaResourceIndex::ReadAt tries to cache last-read block -

https://reviewboard.mozilla.org/r/144324/#review148188

r+, but I feel like this code needs more comments describing the caching strategy.

::: dom/media/MediaResource.h:863
(Diff revision 1)
>    int64_t mOffset;
> +
> +  // Local cache used by ReadAt().
> +  // mCachedBlock is valid when mCachedBytes != 0, in which case it contains
> +  // data corresponding to mCachedBytes starting at offset `mCachedOffset` in
> +  // the file, located at index `OffsetInCache(mCachedOffset)` in mCachedBlock.

s/OffsetInCache/IndexInCache/

Right? I don't see an OffsetInCache function declared anywhere...

::: dom/media/MediaResource.h:866
(Diff revision 1)
> +  // mCachedBlock is valid when mCachedBytes != 0, in which case it contains
> +  // data corresponding to mCachedBytes starting at offset `mCachedOffset` in
> +  // the file, located at index `OffsetInCache(mCachedOffset)` in mCachedBlock.
> +  static constexpr int64_t BLOCK_SIZE = MediaCacheStream::BLOCK_SIZE;
> +  // Maps a file offset to a mCachedBlock index.
> +  static constexpr uint32_t IndexInCache(int64_t aOffsetInFile)

This needs way more comments describing how this works, as it's not obvious without careful reading of the code.

You should have comments describing how you're inserting data into the cached block, what range of data is stored in your cache block, and how  mCachedOffset relates to that. For a long time I thought it was the stream offset of index 0 in the block, but cached data actually starts at mCachedBlock[IndexInCache(mCachedOffset)] and extends for mCachedBytes. Right?

Initially I thought you were storing things in a ring buffer (because you're modding the offset), but you're actually caching the subregion of the MediaCache's block starting from where the read touches up to the end of the block. Right?

::: dom/media/MediaResource.h:870
(Diff revision 1)
> +  // Maps a file offset to a mCachedBlock index.
> +  static constexpr uint32_t IndexInCache(int64_t aOffsetInFile)
> +  {
> +    static_assert((BLOCK_SIZE & (BLOCK_SIZE - 1)) == 0, "BLOCK_SIZE must be power of 2");
> +    static_assert(BLOCK_SIZE <= int64_t(UINT32_MAX), "BLOCK_SIZE must fit in 32 bits");
> +    return uint32_t(aOffsetInFile & (BLOCK_SIZE - 1));

This code is more clever than clear. You need a comment explaining what it's doing, so mere mortals can appreciate your cleverness more thoroughly.

Or you could write it as:

uint32_t rv = uint32_t(aOffsetInFile) & (mCacheBlockSize - 1);
MOZ_ASSERT(rv == (aOffsetInFile % mCacheBlockSize));
return rv;

As that's pretty clear.

::: dom/media/MediaResource.h:876
(Diff revision 1)
> +  }
> +  // Starting file offset of the cache block that contains a given file offset.
> +  static constexpr int64_t CacheOffsetContaining(int64_t aOffsetInFile)
> +  {
> +    static_assert((BLOCK_SIZE & (BLOCK_SIZE - 1)) == 0, "BLOCK_SIZE must be power of 2");
> +    return aOffsetInFile & ~(BLOCK_SIZE - 1);

Ditto here, that can be:

int64_t rv = aOffsetInFile & ~(int64_t(mCacheBlockSize) - 1);
MOZ_ASSERT(rv == (aOffsetInFile - (aOffsetInFile % mCacheBlockSize)));
return rv;

::: dom/media/MediaResource.cpp:1646
(Diff revision 1)
>    }
>    mOffset += *aBytes;
>    return NS_OK;
>  }
>  
> +nsCString rvname(nsresult rv)

Formatting; return type on its own line. I recommend you just clang format.

::: dom/media/MediaResource.cpp:1646
(Diff revision 1)
>    }
>    mOffset += *aBytes;
>    return NS_OK;
>  }
>  
> +nsCString rvname(nsresult rv)

MozStyle is to have a capital letter at the start of function names, and "a" prefixing function args.

::: dom/media/MediaResource.cpp:1655
(Diff revision 1)
> +  return name;
> +}
> +
>  nsresult
>  MediaResourceIndex::ReadAt(int64_t aOffset, char* aBuffer,
> +                           uint32_t aCount, uint32_t* aBytes)

I did not review this function as it seems you remove it in a later patch.

::: dom/media/MediaResource.cpp:1714
(Diff revision 1)
> +    printf("**** [%p]ReadAt(%u@%d) - aCount==0 -> NS_OK, 0\n", this, oCount, oOffset);
> +    return NS_OK;
> +  }
> +
> +  const int64_t endOffset = aOffset + aCount;
> +  const int64_t lastBlockOffset = CacheOffsetContaining(endOffset - 1);

lastBlockOffset is not used until after this next branch, so move it's declaration down to closer to where it's used.

::: dom/media/MediaResource.cpp:1718
(Diff revision 1)
> +  const int64_t endOffset = aOffset + aCount;
> +  const int64_t lastBlockOffset = CacheOffsetContaining(endOffset - 1);
> +
> +  if (mCachedBytes != 0 &&
> +      mCachedOffset + mCachedBytes >= aOffset
> +      && mCachedOffset < endOffset) {

Inconsistent placement of &&. \`./mach clang-format\` would have fixed this?

::: dom/media/MediaResource.cpp:1767
(Diff revision 1)
> +      aBuffer += toCopy;
> +      printf("**** [%p]ReadAt(%u@%d) copied %u from cache(%u@%d) :-), remaining: %u@%d\n", this, oCount, oOffset, unsigned(toCopy), unsigned(mCachedBytes), int(mCachedOffset), unsigned(aCount), int(aOffset));
> +    }
> +
> +    if (aOffset - 1 >= lastBlockOffset) {
> +      // We were already reading cached data from the last block, we need more

The (aOffset - 1 >= lastBlockOffset) (line 1767) and the (else if (aOffset >= lastBlockOffset)) (line 1778) branches are basically the same... Can they cleanly be merged, maybe with by falling through here somehow?

"No" is acceptable, but it would be nice to reduce the number of branches here.

::: dom/media/MediaResource.cpp:1834
(Diff revision 1)
> +  int oOffset, unsigned oCount, const char* oContext,
> +  int64_t aOffset, char* aBuffer, uint32_t aCount, uint32_t* aBytes)
> +{
> +  MOZ_ASSERT(aCount > 0);
> +  const int64_t length = GetLength();
> +  // If length is unknow (-1), look at resource-cached data.

s/unknow/unknown/

::: dom/media/MediaResource.cpp:1849
(Diff revision 1)
> +      // Try to read as much resource-cached data as can fill our local cache.
> +      const uint32_t cacheIndex = IndexInCache(aOffset);
> +      const uint32_t toRead =
> +        uint32_t(std::min(cachedDataEnd - aOffset,
> +                          int64_t(BLOCK_SIZE - cacheIndex)));
> +      MOZ_ASSERT(toRead >= aCount);

How can you be sure that toRead>=aCount in the case where (toRead == BLOCK_SIZE-cacheIndex)? What if (cacheIndex==(BLOCK_SIZE-1))? Or can that not happen.

::: dom/media/MediaResource.cpp:1865
(Diff revision 1)
> +          // We were filling the cache from scratch, save new cache information.
> +          mCachedOffset = aOffset;
> +          mCachedBytes = toRead;
> +        }
> +        // Copy relevant part into output.
> +        memcpy(aBuffer, &mCachedBlock[cacheIndex], aCount);

This line and the line below are only safe if your assert online 1849 is always valid. If it's not, you need to be copying and incrementing by min(aCount,toRead) instead of aCount.
Attachment #8872815 - Flags: review?(cpearce) → review+
Comment on attachment 8872815 [details]
Bug 1368837 - MediaResourceIndex::ReadAt tries to cache last-read block -

https://reviewboard.mozilla.org/r/144324/#review148578

::: dom/media/MediaResource.h:863
(Diff revision 1)
>    int64_t mOffset;
> +
> +  // Local cache used by ReadAt().
> +  // mCachedBlock is valid when mCachedBytes != 0, in which case it contains
> +  // data corresponding to mCachedBytes starting at offset `mCachedOffset` in
> +  // the file, located at index `OffsetInCache(mCachedOffset)` in mCachedBlock.

Actually, this comment mostly describes how your caching works, modulo the typo of s/OffsetInCache/IndexInCache/.

I think instead of saying "data corresponding to mCachedBytes", it's better to say something like "data of length mCachedBytes".

I think I was suffering from Wall Of Text syndrome, and tiredness when first looking at this.
Comment on attachment 8872816 [details]
Bug 1368837 - media.cache.resource-index controls the MediaResourceIndex cache size -

https://reviewboard.mozilla.org/r/144326/#review148584

::: dom/media/MediaResource.h:900
(Diff revision 1)
>    }
> +
> +  RefPtr<MediaResource> mResource;
> +  int64_t mOffset;
> +
> +  // Local cache used by ReadAt().

Please make sure the comment changes you make to this comment addressing review comments in the previous patch survive when this comment is moved in this patch.
Attachment #8872816 - Flags: review?(cpearce) → review+
Comment on attachment 8872817 [details]
Bug 1368837 - Replace debugging ReadAt with CachedReadAt code -

https://reviewboard.mozilla.org/r/144328/#review148590
Attachment #8872817 - Flags: review?(cpearce) → review+
Comment on attachment 8872818 [details]
Bug 1368837 - Convert printfs into MOZ_LOG(MediaResourceIndex) -

https://reviewboard.mozilla.org/r/144330/#review148592

::: dom/media/MediaResource.cpp:1701
(Diff revision 1)
>          // Could not read everything we wanted, we're done.
> -        printf("**** [%p]ReadAt(%u@%d) uncached read before cache, incomplete -> OK, %u\n", this, oCount, oOffset, unsigned(*aBytes));
> +        ILOG("ReadAt(%" PRIu32 "@%" PRId64 ") uncached read before cache, incomplete -> OK, %" PRIu32,
> +             aCount, aOffset, *aBytes);
>          return NS_OK;
>        }
> +      ILOG("ReadAt(%" PRIu32 "@%" PRId64 ") uncached read before cache: %" PRIu32 ", remaining: %" PRIu32 "@%" PRId64 "...",

Some long lines. clang-format will fix these automagically for you.

I now always clang-format before amending/committing, as I got sick of you pointing out where I was breaching our style guide when you reviewed my patches! I suggest you consider doing the same. :)
Attachment #8872818 - Flags: review?(cpearce) → review+
Comment on attachment 8872815 [details]
Bug 1368837 - MediaResourceIndex::ReadAt tries to cache last-read block -

https://reviewboard.mozilla.org/r/144324/#review148188

> s/OffsetInCache/IndexInCache/
> 
> Right? I don't see an OffsetInCache function declared anywhere...

Ah yep, that was the old function name. :-)

> This needs way more comments describing how this works, as it's not obvious without careful reading of the code.
> 
> You should have comments describing how you're inserting data into the cached block, what range of data is stored in your cache block, and how  mCachedOffset relates to that. For a long time I thought it was the stream offset of index 0 in the block, but cached data actually starts at mCachedBlock[IndexInCache(mCachedOffset)] and extends for mCachedBytes. Right?
> 
> Initially I thought you were storing things in a ring buffer (because you're modding the offset), but you're actually caching the subregion of the MediaCache's block starting from where the read touches up to the end of the block. Right?

I'll clarify things and add a diagram.

> This code is more clever than clear. You need a comment explaining what it's doing, so mere mortals can appreciate your cleverness more thoroughly.
> 
> Or you could write it as:
> 
> uint32_t rv = uint32_t(aOffsetInFile) & (mCacheBlockSize - 1);
> MOZ_ASSERT(rv == (aOffsetInFile % mCacheBlockSize));
> return rv;
> 
> As that's pretty clear.

Great idea, thanks.

> lastBlockOffset is not used until after this next branch, so move it's declaration down to closer to where it's used.

It is used inside the if-block, in the else clause, as well as after, so I think that's the best spot for it.

> Inconsistent placement of &&. \`./mach clang-format\` would have fixed this?

Didn't know that existed! :-)

> The (aOffset - 1 >= lastBlockOffset) (line 1767) and the (else if (aOffset >= lastBlockOffset)) (line 1778) branches are basically the same... Can they cleanly be merged, maybe with by falling through here somehow?
> 
> "No" is acceptable, but it would be nice to reduce the number of branches here.

I'm afraid it's a "no" here...
I had seen this apparent duplication, but I don't think I can get rid of it:

The top-level scenarios are different: In the first branch we've just reached the exact end of what was cached, and we want to add to it; in the second branch we just discard any previous cached data and fill the cache with new data (usually from a different block).

Regarding the code itself, there is an important off-by-1 difference in the test. And in the first branch we keep the existing cache, while in the second branch we invalidate the cache first, before going to CacheOrReadAt.

> How can you be sure that toRead>=aCount in the case where (toRead == BLOCK_SIZE-cacheIndex)? What if (cacheIndex==(BLOCK_SIZE-1))? Or can that not happen.

For a start, I *want* it to be that way, because we want to cache at least as much as what is requested.

And I know it's always true:
- In the (toRead = cachedDataEnd - aOffset) case, because of the `if (cachedDataEnd >= aOffset + aCount)` above.
- In the (toRead = BLOCK_SIZE - cacheIndex) case, because each call to CacheOrReadAt is done when aOffset has reached the last "block", so (IndexInCache(aOffset) + aCount <= BLOCK_SIZE), and therefore (aCount <= BLOCK_SIZE - cacheIndex). :-)

if (cacheIndex == BLOCK_SIZE - 1), aCount should be 1 in this case, so we still have (aCount <= BLOCK_SIZE - cacheIndex).

To make it clearer, I will add a `MOZ_ASSERT(IndexInCache(aOffset) + aCount <= BLOCK_SIZE)` at the top of the function, with some explanatory comments.
Comment on attachment 8873293 [details]
Bug 1368837 - Document that MediaResource::GetCachedDataEnd should return aOffset when out of cache -

https://reviewboard.mozilla.org/r/144742/#review148640
Attachment #8873293 - Flags: review?(cpearce) → review+
Comment on attachment 8873294 [details]
Bug 1368837 - BufferMediaResource::GetCachedDataEnd should return aOffset when out of cache -

https://reviewboard.mozilla.org/r/144744/#review148638
Attachment #8873294 - Flags: review?(cpearce) → review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a404b72b9b8
Document that MediaResource::GetCachedDataEnd should return aOffset when out of cache - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/e38d5530bf2b
BufferMediaResource::GetCachedDataEnd should return aOffset when out of cache - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/d7836ae27b65
Implement SourceBufferResource::GetCachedDataEnd - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/a341e170f306
MockMediaResource::GetCachedDataEnd should return the offset if out of range - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/2f9a486c2dbb
MockMediaResource should fread individual bytes - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/9b1ef0d8d960
WaveTrackDemuxer should copy the MediaResource* instead of a whole MediaResourceIndex - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/b9d2d284df0d
MediaResourceIndex::ReadAt tries to cache last-read block - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/b3dff4b28a95
media.cache.resource-index controls the MediaResourceIndex cache size - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/cc5eaeefd445
Replace debugging ReadAt with CachedReadAt code - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/30a1c114fe23
Convert printfs into MOZ_LOG(MediaResourceIndex) - r=cpearce
Comment on attachment 8872818 [details]
Bug 1368837 - Convert printfs into MOZ_LOG(MediaResourceIndex) -

https://reviewboard.mozilla.org/r/144330/#review148712

::: dom/media/MediaResource.cpp:1693
(Diff revision 2)
>        MOZ_ASSERT(toRead > 0);
>        MOZ_ASSERT(toRead < aCount);
>        uint32_t read = 0;
>        nsresult rv = UncachedReadAt(aOffset, aBuffer, toRead, &read);
>        if (NS_FAILED(rv)) {
> -        printf("**** [%p]ReadAt(%u@%d) uncached read before cache -> %s\n",
> +        ILOG("ReadAt(%" PRIu32 "@%" PRId64

I think this patch should be merged with the one introducing the printf.

unecessary history, and no code with printf should ever hit the tree, except by accident.
No longer depends on: 1374441
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: