Bug 1368837 - MediaResourceIndex::ReadAt tries to cache last-read block - r=cpearce
authorGerald Squelart <gsquelart@mozilla.com>
Tue, 30 May 2017 14:59:30 +1200
changeset 361721 b9d2d284df0d2224049140932cf0e4d9fe2f702d
parent 361720 9b1ef0d8d9605c49a7a96214107be59d94a227dd
child 361722 b3dff4b28a95446bdae8c91e15ec1a5b5ab5b6c7
push id31939
push usercbook@mozilla.com
push dateThu, 01 Jun 2017 11:49:28 +0000
treeherdermozilla-central@d96110d76619 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpearce
bugs1368837
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1368837 - MediaResourceIndex::ReadAt tries to cache last-read block - r=cpearce This is the core of this bug: - We try to read past the end of the requested range, and save a block-full of cached data. ("Block" is a memory range, with an alignment and size being a power of two, to match existing caching happening in MediaCache and FileBlockCache, and to play nice with the memory allocator.) - If part of a requested read touches the existing cache, we can just read from the cache, which means we don't involve any of the locking and IOs that normal reads use. The small extra work needed to cache more data in some reads is largely offset by all the lock&IO-heavy reads that we can subsequently avoid. UncachedReadAt, which is used internally by CachedReadAt, is left public because it could be useful if the caller knows for sure that a particular read is isolated. (Note: The printfs, and comparison code in ReadAt, will be removed in later patches. Also the block size will be later controlled by a pref.) MozReview-Commit-ID: GFiaP5Io7Hf
dom/media/MediaResource.cpp
dom/media/MediaResource.h
--- a/dom/media/MediaResource.cpp
+++ b/dom/media/MediaResource.cpp
@@ -1638,32 +1638,433 @@ MediaResourceIndex::Read(char* aBuffer, 
   nsresult rv = ReadAt(mOffset, aBuffer, aCount, aBytes);
   if (NS_FAILED(rv)) {
     return rv;
   }
   mOffset += *aBytes;
   return NS_OK;
 }
 
+nsCString
+ResultName(nsresult aResult)
+{
+  nsCString name;
+  GetErrorName(aResult, name);
+  return name;
+}
+
 nsresult
-MediaResourceIndex::ReadAt(int64_t aOffset, char* aBuffer,
-                           uint32_t aCount, uint32_t* aBytes) const
+MediaResourceIndex::ReadAt(int64_t aOffset,
+                           char* aBuffer,
+                           uint32_t aCount,
+                           uint32_t* aBytes)
+{
+  const int oOffset = int(aOffset);
+  const unsigned oCount = unsigned(aCount);
+  nsresult rvu = UncachedReadAt(aOffset, aBuffer, aCount, aBytes);
+  printf("**** [%p]CompareReadAt(%u@%d) UncachedReadAt -> %s, %u\n",
+         this,
+         oCount,
+         oOffset,
+         ResultName(rvu).get(),
+         *aBytes);
+
+  printf("**** [%p]CompareReadAt(%u@%d) CachedReadAt: ----------------\n",
+         this,
+         oCount,
+         oOffset);
+  char* buf = new char[aCount];
+  uint32_t bytes = 0;
+  nsresult rvc = CachedReadAt(aOffset, buf, aCount, &bytes);
+
+  printf("**** [%p]CompareReadAt(%u@%d) ---------------- Comparing...\n",
+         this,
+         oCount,
+         oOffset);
+
+  if (rvu != rvc) {
+    printf("*** read(aOffset=%d, aCount=%u) - rvu=%s != rvc=%s\n",
+           int(aOffset),
+           unsigned(aCount),
+           ResultName(rvu).get(),
+           ResultName(rvc).get());
+    MOZ_ASSERT(rvu == rvc || rvc == NS_OK);
+    if (rvc == NS_OK) {
+      // Cached read wins!
+      *aBytes = bytes;
+      memcpy(aBuffer, buf, bytes);
+    }
+  } else if (NS_SUCCEEDED(rvu)) {
+    if (*aBytes != bytes) {
+      printf("*** read(aOffset=%d, aCount=%u) - bu=%u != bc=%u\n",
+             int(aOffset),
+             unsigned(aCount),
+             unsigned(*aBytes),
+             unsigned(bytes));
+      MOZ_ASSERT(*aBytes == bytes);
+    } else {
+      for (uint32_t i = 0; i < bytes; ++i) {
+        uint8_t bu = uint8_t(aBuffer[i]);
+        uint8_t bc = uint8_t(buf[i]);
+        if (bu != bc) {
+          printf("*** read(aOffset=%d, aCount=%u) - u[%u]=%u != c[%u]=%u\n",
+                 int(aOffset),
+                 unsigned(aCount),
+                 unsigned(i),
+                 unsigned(bu),
+                 unsigned(i),
+                 unsigned(bc));
+          MOZ_ASSERT(bu == bc);
+          break;
+        }
+      }
+    }
+  }
+
+  delete[] buf;
+
+  return rvc;
+}
+
+nsresult
+MediaResourceIndex::CachedReadAt(int64_t aOffset,
+                                 char* aBuffer,
+                                 uint32_t aCount,
+                                 uint32_t* aBytes)
+{
+  const int oOffset = int(aOffset);
+  const unsigned oCount = unsigned(aCount);
+  *aBytes = 0;
+
+  if (aCount == 0) {
+    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);
+
+  if (mCachedBytes != 0 && mCachedOffset + mCachedBytes >= aOffset &&
+      mCachedOffset < endOffset) {
+    // There is data in the cache that is not completely before aOffset and not
+    // completely after endOffset, so it could be usable (with potential top-up).
+    if (aOffset < mCachedOffset) {
+      // We need to read before the cached data.
+      const uint32_t toRead = uint32_t(mCachedOffset - aOffset);
+      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",
+               this,
+               oCount,
+               oOffset,
+               ResultName(rv).get());
+        return rv;
+      }
+      *aBytes = read;
+      if (read < toRead) {
+        // 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));
+        return NS_OK;
+      }
+      aOffset += read;
+      aBuffer += read;
+      aCount -= read;
+      printf("**** [%p]ReadAt(%u@%d) uncached read before cache: %u, "
+             "remaining: %u@%d\n",
+             this,
+             oCount,
+             oOffset,
+             unsigned(read),
+             unsigned(aCount),
+             int(aOffset));
+      // We should have reached the cache.
+      MOZ_ASSERT(aOffset == mCachedOffset);
+    }
+    MOZ_ASSERT(aOffset >= mCachedOffset);
+
+    // We've reached our cache.
+    const uint32_t toCopy =
+      std::min(aCount, uint32_t(mCachedOffset + mCachedBytes - aOffset));
+    // Note that we could in fact be just after the last byte of the cache, in
+    // which case we can't actually read from it! (But we will top-up next.)
+    if (toCopy != 0) {
+      memcpy(aBuffer, &mCachedBlock[IndexInCache(aOffset)], toCopy);
+      *aBytes += toCopy;
+      aCount -= toCopy;
+      if (aCount == 0) {
+        // All done!
+        printf("**** [%p]ReadAt(%u@%d) copied from cache(%u@%d) and done :-) "
+               "-> OK, %u\n",
+               this,
+               oCount,
+               oOffset,
+               unsigned(mCachedBytes),
+               int(mCachedOffset),
+               unsigned(*aBytes));
+        return NS_OK;
+      }
+      aOffset += toCopy;
+      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
+      // from it -> try to top-up, read what we can, and we'll be done.
+      MOZ_ASSERT(aOffset == mCachedOffset + mCachedBytes);
+      MOZ_ASSERT(endOffset <= lastBlockOffset + BLOCK_SIZE);
+      return CacheOrReadAt(
+        oOffset, oCount, "top-up cache", aOffset, aBuffer, aCount, aBytes);
+    }
+
+    // We were not in the last block (but we may just have crossed the line now)
+    MOZ_ASSERT(aOffset <= lastBlockOffset);
+    // Continue below...
+  } else if (aOffset >= lastBlockOffset) {
+    // There was nothing we could get from the cache.
+    // But we're already in the last block -> Cache or read what we can.
+    // Make sure to invalidate the cache first.
+    mCachedBytes = 0;
+    return CacheOrReadAt(
+      oOffset, oCount, "nothing in cache", aOffset, aBuffer, aCount, aBytes);
+  }
+
+  // If we're here, either there was nothing usable in the cache, or we've just
+  // read what was in the cache but there's still more to read.
+
+  if (aOffset < lastBlockOffset) {
+    // We need to read before the last block.
+    // Start with an uncached read up to the last block.
+    const uint32_t toRead = uint32_t(lastBlockOffset - aOffset);
+    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 last block failed -> %s\n",
+        this,
+        oCount,
+        oOffset,
+        ResultName(rv).get());
+      return rv;
+    }
+    if (read == 0) {
+      printf(
+        "**** [%p]ReadAt(%u@%d) uncached read 0 before last block -> OK, %u\n",
+        this,
+        oCount,
+        oOffset,
+        unsigned(*aBytes));
+      return NS_OK;
+    }
+    *aBytes += read;
+    if (read < toRead) {
+      // Could not read everything we wanted, we're done.
+      printf("**** [%p]ReadAt(%u@%d) uncached read before last block, "
+             "incomplete -> OK, %u\n",
+             this,
+             oCount,
+             oOffset,
+             unsigned(*aBytes));
+      return NS_OK;
+    }
+    aOffset += read;
+    aBuffer += read;
+    aCount -= read;
+    printf(
+      "**** [%p]ReadAt(%u@%d) read %u before last block, remaining: %u@%d\n",
+      this,
+      oCount,
+      oOffset,
+      unsigned(read),
+      unsigned(aCount),
+      int(aOffset));
+  }
+
+  // We should just have reached the start of the last block.
+  MOZ_ASSERT(aOffset == lastBlockOffset);
+  MOZ_ASSERT(aCount <= BLOCK_SIZE);
+  // Make sure to invalidate the cache first.
+  mCachedBytes = 0;
+  return CacheOrReadAt(
+    oOffset, oCount, "last block", aOffset, aBuffer, aCount, aBytes);
+}
+
+nsresult
+MediaResourceIndex::CacheOrReadAt(int oOffset,
+                                  unsigned oCount,
+                                  const char* oContext,
+                                  int64_t aOffset,
+                                  char* aBuffer,
+                                  uint32_t aCount,
+                                  uint32_t* aBytes)
+{
+  // We should be here because there is more data to read.
+  MOZ_ASSERT(aCount > 0);
+  // We should be in the last block, so we shouldn't try to read past it.
+  MOZ_ASSERT(IndexInCache(aOffset) + aCount <= BLOCK_SIZE);
+
+  const int64_t length = GetLength();
+  // If length is unknown (-1), look at resource-cached data.
+  // If length is known and equal or greater than requested, also look at
+  // resource-cached data.
+  // Otherwise, if length is known but same, or less than(!?), requested, don't
+  // attempt to access resource-cached data, as we're not expecting it to ever
+  // be greater than the length.
+  if (length < 0 || length >= aOffset + aCount) {
+    // Is there cached data covering at least the requested range?
+    const int64_t cachedDataEnd = mResource->GetCachedDataEnd(aOffset);
+    if (cachedDataEnd >= aOffset + aCount) {
+      // 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);
+      nsresult rv =
+        mResource->ReadFromCache(&mCachedBlock[cacheIndex], aOffset, toRead);
+      if (NS_SUCCEEDED(rv)) {
+        // Success means we have read the full `toRead` amount.
+        printf("**** [%p]ReadAt(%u@%d) - %s - ReadFromCache(%u@%d) succeeded\n",
+               this,
+               oCount,
+               oOffset,
+               oContext,
+               unsigned(toRead),
+               int(aOffset));
+        if (mCachedOffset + mCachedBytes == aOffset) {
+          // We were topping-up the cache, just update its size.
+          mCachedBytes += toRead;
+        } else {
+          // 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);
+        *aBytes += aCount;
+        printf("**** [%p]ReadAt(%u@%d) - %s - copied %u@%d, remaining: %u@%d "
+               "-> OK, %u\n",
+               this,
+               oCount,
+               oOffset,
+               oContext,
+               unsigned(aCount),
+               int(aOffset),
+               unsigned(aCount),
+               int(aOffset),
+               unsigned(*aBytes));
+        // We may not have read all that was requested, but we got everything
+        // we could get, so we're done.
+        return NS_OK;
+      }
+      printf("**** [%p]ReadAt(%u@%d) - %s - ReadFromCache(%u@%d) failed: %s, "
+             "will fallback to blocking read\n",
+             this,
+             oCount,
+             oOffset,
+             oContext,
+             unsigned(toRead),
+             int(aOffset),
+             ResultName(rv).get());
+      // Failure during reading. Note that this may be due to the cache
+      // changing between `GetCachedDataEnd` and `ReadFromCache`, so it's not
+      // totally unexpected, just hopefully rare; but we do need to handle it.
+
+      // Invalidate part of cache that may have been partially overridden.
+      if (mCachedOffset + mCachedBytes == aOffset) {
+        // We were topping-up the cache, just keep the old untouched data.
+        // (i.e., nothing to do here.)
+      } else {
+        // We were filling the cache from scratch, invalidate cache.
+        mCachedBytes = 0;
+      }
+    } else {
+      printf("**** [%p]ReadAt(%u@%d) - %s - no cached data, will fallback to "
+             "blocking read\n",
+             this,
+             oCount,
+             oOffset,
+             oContext);
+    }
+  } else {
+    printf("**** [%p]ReadAt(%u@%d) - %s - length is known and too short(!), "
+           "will fallback to blocking read as the caller requested\n",
+           this,
+           oCount,
+           oOffset,
+           oContext);
+  }
+  uint32_t read = 0;
+  nsresult rv = UncachedReadAt(aOffset, aBuffer, aCount, &read);
+  if (NS_SUCCEEDED(rv)) {
+    printf("**** [%p]ReadAt(%u@%d) - %s - fallback uncached read -> %s, %u\n",
+           this,
+           oCount,
+           oOffset,
+           oContext,
+           ResultName(rv).get(),
+           unsigned(read));
+    *aBytes += read;
+  } else {
+    printf("**** [%p]ReadAt(%u@%d) - %s - fallback uncached read -> %s\n",
+           this,
+           oCount,
+           oOffset,
+           oContext,
+           ResultName(rv).get());
+  }
+  return rv;
+}
+
+nsresult
+MediaResourceIndex::UncachedReadAt(int64_t aOffset,
+                                   char* aBuffer,
+                                   uint32_t aCount,
+                                   uint32_t* aBytes) const
 {
   *aBytes = 0;
-  while (aCount > 0) {
-    uint32_t bytesRead = 0;
-    nsresult rv = mResource->ReadAt(aOffset, aBuffer, aCount, &bytesRead);
-    NS_ENSURE_SUCCESS(rv, rv);
-    if (!bytesRead) {
-      break;
+  if (aCount != 0) {
+    for (;;) {
+      uint32_t bytesRead = 0;
+      nsresult rv = mResource->ReadAt(aOffset, aBuffer, aCount, &bytesRead);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+      if (bytesRead == 0) {
+        break;
+      }
+      *aBytes += bytesRead;
+      aCount -= bytesRead;
+      if (aCount == 0) {
+        break;
+      }
+      aOffset += bytesRead;
+      aBuffer += bytesRead;
     }
-    *aBytes += bytesRead;
-    aOffset += bytesRead;
-    aBuffer += bytesRead;
-    aCount -= bytesRead;
   }
   return NS_OK;
 }
 
 nsresult
 MediaResourceIndex::Seek(int32_t aWhence, int64_t aOffset)
 {
   switch (aWhence) {
--- a/dom/media/MediaResource.h
+++ b/dom/media/MediaResource.h
@@ -755,16 +755,18 @@ private:
  */
 
 class MediaResourceIndex
 {
 public:
   explicit MediaResourceIndex(MediaResource* aResource)
     : mResource(aResource)
     , mOffset(0)
+    , mCachedOffset(0)
+    , mCachedBytes(0)
   {}
 
   // Read up to aCount bytes from the stream. The buffer must have
   // enough room for at least aCount bytes. Stores the number of
   // actual bytes read in aBytes (0 on end of file).
   // May read less than aCount bytes if the number of
   // available bytes is less than aCount. Always check *aBytes after
   // read, and call again if necessary.
@@ -805,18 +807,32 @@ public:
   // Read up to aCount bytes from the stream. The read starts at
   // aOffset in the stream, seeking to that location initially if
   // it is not the current stream offset.
   // Unlike MediaResource::ReadAt, ReadAt only returns fewer bytes than
   // requested if end of stream or an error is encountered. There is no need to
   // call it again to get more data.
   // *aBytes will contain the number of bytes copied, even if an error occurred.
   // ReadAt doesn't have an impact on the offset returned by Tell().
-  nsresult ReadAt(int64_t aOffset, char* aBuffer,
-                  uint32_t aCount, uint32_t* aBytes) const;
+  nsresult ReadAt(int64_t aOffset,
+                  char* aBuffer,
+                  uint32_t aCount,
+                  uint32_t* aBytes);
+
+  nsresult CachedReadAt(int64_t aOffset,
+                        char* aBuffer,
+                        uint32_t aCount,
+                        uint32_t* aBytes);
+
+  // Same as ReadAt, but doesn't try to cache around the read.
+  // Useful if you know that you will not read again from the same area.
+  nsresult UncachedReadAt(int64_t aOffset,
+                          char* aBuffer,
+                          uint32_t aCount,
+                          uint32_t* aBytes) const;
 
   // Convenience methods, directly calling the MediaResource method of the same
   // name.
   // Those functions do not update the MediaResource offset as returned
   // by Tell().
 
   // This method returns nullptr if anything fails.
   // Otherwise, it returns an owned buffer.
@@ -830,15 +846,67 @@ public:
   // This can change over time; after a seek operation, a misbehaving
   // server may give us a resource of a different length to what it had
   // reported previously --- or it may just lie in its Content-Length
   // header and give us more or less data than it reported. We will adjust
   // the result of GetLength to reflect the data that's actually arriving.
   int64_t GetLength() const { return mResource->GetLength(); }
 
 private:
+  // If the resource has cached data past the requested range, try to grab it
+  // into our local cache.
+  // If there is no cached data, or attempting to read it fails, fallback on
+  // a (potentially-blocking) read of just what was requested, so that we don't
+  // get unexpected side-effects by trying to read more than intended.
+  nsresult CacheOrReadAt(int oOffset,
+                         unsigned oCount,
+                         const char* oContext,
+                         int64_t aOffset,
+                         char* aBuffer,
+                         uint32_t aCount,
+                         uint32_t* aBytes);
+
+  // Maps a file offset to a mCachedBlock index.
+  uint32_t IndexInCache(int64_t aOffsetInFile) const
+  {
+    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");
+    const uint32_t index = uint32_t(aOffsetInFile & (BLOCK_SIZE - 1));
+    MOZ_ASSERT(index == aOffsetInFile % BLOCK_SIZE);
+    return index;
+  }
+
+  // Starting file offset of the cache block that contains a given file offset.
+  int64_t CacheOffsetContaining(int64_t aOffsetInFile) const
+  {
+    static_assert((BLOCK_SIZE & (BLOCK_SIZE - 1)) == 0,
+                  "BLOCK_SIZE must be power of 2");
+    const int64_t offset = aOffsetInFile & ~(BLOCK_SIZE - 1);
+    MOZ_ASSERT(offset == aOffsetInFile - IndexInCache(aOffsetInFile));
+    return offset;
+  }
+
   RefPtr<MediaResource> mResource;
   int64_t mOffset;
+
+  // Local cache used by ReadAt().
+  // mCachedBlock is valid when mCachedBytes != 0, in which case it contains
+  // data of length mCachedBytes, starting at offset `mCachedOffset` in the
+  // resource, located at index `IndexInCache(mCachedOffset)` in mCachedBlock.
+  //
+  // resource: |------------------------------------------------------|
+  //                                          <----------> BLOCK_SIZE
+  //           <---------------------------------> mCachedOffset
+  //                                             <--> mCachedBytes
+  // mCachedBlock:                            |..----....|
+  //  CacheOffsetContaining(mCachedOffset)    <--> IndexInCache(mCachedOffset)
+  //           <------------------------------>
+  static constexpr int64_t BLOCK_SIZE = MediaCacheStream::BLOCK_SIZE;
+  int64_t mCachedOffset;
+  uint32_t mCachedBytes;
+  char mCachedBlock[BLOCK_SIZE];
 };
 
 } // namespace mozilla
 
 #endif