Bug 1383404 - Part 2. When SourceBuffer::ExpectLength creates the initial buffer, it should not round up. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 01 Aug 2017 06:59:11 -0400
changeset 420917 d8223b2142a9d5834e56f96598d07e9b50843e67
parent 420916 4f8e0cb21016059dd19fb4b706e960ec88c64023
child 420918 8d5e41b9498b971f3e4b0f704fa8cd81ccdac10e
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1383404
milestone56.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 1383404 - Part 2. When SourceBuffer::ExpectLength creates the initial buffer, it should not round up. r=tnikkel Currently SourceBuffer::ExpectLength will allocate a buffer which is a multiple of MIN_CHUNK_CAPACITY (4096) bytes, no matter what the expected size is. While it is true that HTTP servers can lie, and that we need to handle that for legacy purposes, it is more likely the HTTP servers are telling the truth when it comes to the content length. Additionally images sourced from other locations, such as the file system or data URIs, are always going to have the correct size information (barring a bug elsewhere in the file system or our code). We should be able to trust the size given as a good first guess. While overallocating in general is a waste of memory, SourceBuffer::Compact causes a far worse problem. After we have written all of the data, and there are no active readers, we attempt to shrink the allocated buffer(s) into a single contiguous chunk of the exact length that we need (e.g. N allocations to 1, or 1 oversized allocation to 1 perfect). Since we almost always overallocate, that means we almost always trigger the logic in SourceBuffer::Compact to reallocate the data into a properly sized buffer. If we had simply trusted the expected size in the first place, we could have avoided this situation for the majority of images. In the case that we really do get the wrong size, then we will allocate additional chunks which are multiples of MIN_CHUNK_CAPACITY bytes to fit the data. At most, this will increase the number of discrete allocations by 1, and trigger SourceBuffer::Compact to consolidate at the end. Since we are almost always doing that before, and now we rarely do, this is a significant win.
image/SourceBuffer.cpp
image/test/gtest/TestSourceBuffer.cpp
--- a/image/SourceBuffer.cpp
+++ b/image/SourceBuffer.cpp
@@ -329,17 +329,17 @@ SourceBuffer::ExpectLength(size_t aExpec
     return NS_OK;
   }
 
   if (MOZ_UNLIKELY(mChunks.Length() > 0)) {
     MOZ_ASSERT_UNREACHABLE("Duplicate or post-Append call to ExpectLength");
     return NS_OK;
   }
 
-  if (MOZ_UNLIKELY(NS_FAILED(AppendChunk(CreateChunk(aExpectedLength))))) {
+  if (MOZ_UNLIKELY(NS_FAILED(AppendChunk(CreateChunk(aExpectedLength, /* aRoundUp */ false))))) {
     return HandleError(NS_ERROR_OUT_OF_MEMORY);
   }
 
   return NS_OK;
 }
 
 nsresult
 SourceBuffer::Append(const char* aData, size_t aLength)
--- a/image/test/gtest/TestSourceBuffer.cpp
+++ b/image/test/gtest/TestSourceBuffer.cpp
@@ -368,32 +368,35 @@ TEST_F(ImageSourceBuffer, MinChunkCapaci
   CheckedCompleteBuffer(iterator, 1);
 
   // Verify that the iterator sees the new byte and a new chunk has been
   // allocated.
   CheckedAdvanceIterator(iterator, 1, 2, SourceBuffer::MIN_CHUNK_CAPACITY + 1);
   CheckIteratorIsComplete(iterator, 2, SourceBuffer::MIN_CHUNK_CAPACITY + 1);
 }
 
-TEST_F(ImageSourceBuffer, ExpectLengthDoesNotShrinkBelowMinCapacity)
+TEST_F(ImageSourceBuffer, ExpectLengthAllocatesRequestedCapacity)
 {
   SourceBufferIterator iterator = mSourceBuffer->Iterator();
 
   // Write SourceBuffer::MIN_CHUNK_CAPACITY bytes of test data to the buffer,
   // but call ExpectLength() first to make SourceBuffer expect only a single
-  // byte. We expect this to still result in only one chunk, because
-  // regardless of ExpectLength() we won't allocate a chunk smaller than
-  // MIN_CHUNK_CAPACITY bytes.
+  // byte. We expect this to still result in two chunks, because we trust the
+  // initial guess of ExpectLength() but after that it will only allocate chunks
+  // of at least MIN_CHUNK_CAPACITY bytes.
   EXPECT_TRUE(NS_SUCCEEDED(mSourceBuffer->ExpectLength(1)));
   CheckedAppendToBufferInChunks(10, SourceBuffer::MIN_CHUNK_CAPACITY);
   CheckedCompleteBuffer(iterator, SourceBuffer::MIN_CHUNK_CAPACITY);
 
-  // Verify that the iterator sees a single chunk.
-  CheckedAdvanceIterator(iterator, SourceBuffer::MIN_CHUNK_CAPACITY);
-  CheckIteratorIsComplete(iterator, 1, SourceBuffer::MIN_CHUNK_CAPACITY);
+  // Verify that the iterator sees a first chunk with 1 byte, and a second chunk
+  // with the remaining data.
+  CheckedAdvanceIterator(iterator, 1, 1, 1);
+  CheckedAdvanceIterator(iterator, SourceBuffer::MIN_CHUNK_CAPACITY - 1, 2,
+		                   SourceBuffer::MIN_CHUNK_CAPACITY);
+  CheckIteratorIsComplete(iterator, 2, SourceBuffer::MIN_CHUNK_CAPACITY);
 }
 
 TEST_F(ImageSourceBuffer, ExpectLengthGrowsAboveMinCapacity)
 {
   SourceBufferIterator iterator = mSourceBuffer->Iterator();
 
   // Write two times SourceBuffer::MIN_CHUNK_CAPACITY bytes of test data to the
   // buffer, calling ExpectLength() with the correct length first. We expect