From 03a4a17ccffd0865dc948a767185ac2f6f23d836 Mon Sep 17 00:00:00 2001 From: Martok Date: Tue, 3 Jan 2023 22:55:13 +0100 Subject: [PATCH] Issue #2073 - m-c 1383404: make SourceBuffer::Compact more efficient (squashed) The first part also means that Compact no longer needs the SurfaceCache lock (used to be via CreateChunk->CanHold), which avoids potential deadlocks during shutdown that m-c 523950 would otherwise cause --- image/ImageFactory.cpp | 47 +++++++++++-------- image/ImageFactory.h | 3 +- image/SourceBuffer.cpp | 31 ++++++------ image/SourceBuffer.h | 33 ++++++++++--- image/imgTools.cpp | 19 ++++---- .../places/PageIconProtocolHandler.js | 1 + 6 files changed, 80 insertions(+), 54 deletions(-) diff --git a/image/ImageFactory.cpp b/image/ImageFactory.cpp index 428be1424e..343f7b582b 100644 --- a/image/ImageFactory.cpp +++ b/image/ImageFactory.cpp @@ -111,8 +111,32 @@ BadImage(const char* aMessage, RefPtr& aImage) return aImage.forget(); } +static void +SetSourceSizeHint(RasterImage* aImage, uint32_t aSize) +{ + // Pass anything usable on so that the RasterImage can preallocate + // its source buffer. + if (aSize == 0) { + return; + } + + // Bound by something reasonable + uint32_t sizeHint = std::min(aSize, 20000000); + nsresult rv = aImage->SetSourceSizeHint(sizeHint); + if (NS_FAILED(rv)) { + // Flush memory, try to get some back, and try again. + rv = nsMemory::HeapMinimize(true); + nsresult rv2 = aImage->SetSourceSizeHint(sizeHint); + // If we've still failed at this point, things are going downhill. + if (NS_FAILED(rv) || NS_FAILED(rv2)) { + NS_WARNING("About to hit OOM in imagelib!"); + } + } +} + /* static */ already_AddRefed -ImageFactory::CreateAnonymousImage(const nsCString& aMimeType) +ImageFactory::CreateAnonymousImage(const nsCString& aMimeType, + uint32_t aSizeHint /* = 0 */) { nsresult rv; @@ -127,6 +151,7 @@ ImageFactory::CreateAnonymousImage(const nsCString& aMimeType) return BadImage("RasterImage::Init failed", newImage); } + SetSourceSizeHint(newImage, aSizeHint); return newImage.forget(); } @@ -231,25 +256,7 @@ ImageFactory::CreateRasterImage(nsIRequest* aRequest, newImage->SetInnerWindowID(aInnerWindowId); - uint32_t len = GetContentSize(aRequest); - - // Pass anything usable on so that the RasterImage can preallocate - // its source buffer. - if (len > 0) { - // Bound by something reasonable - uint32_t sizeHint = std::min(len, 20000000); - rv = newImage->SetSourceSizeHint(sizeHint); - if (NS_FAILED(rv)) { - // Flush memory, try to get some back, and try again. - rv = nsMemory::HeapMinimize(true); - nsresult rv2 = newImage->SetSourceSizeHint(sizeHint); - // If we've still failed at this point, things are going downhill. - if (NS_FAILED(rv) || NS_FAILED(rv2)) { - NS_WARNING("About to hit OOM in imagelib!"); - } - } - } - + SetSourceSizeHint(newImage, GetContentSize(aRequest)); return newImage.forget(); } diff --git a/image/ImageFactory.h b/image/ImageFactory.h index 6c2e0f5045..fdd6460ec1 100644 --- a/image/ImageFactory.h +++ b/image/ImageFactory.h @@ -51,9 +51,10 @@ public: * the usual image loading mechanism. * * @param aMimeType The mimetype of the image. + * @param aSizeHint The length of the source data for the image. */ static already_AddRefed - CreateAnonymousImage(const nsCString& aMimeType); + CreateAnonymousImage(const nsCString& aMimeType, uint32_t aSizeHint = 0); /** * Creates a new multipart/x-mixed-replace image wrapper, and initializes it diff --git a/image/SourceBuffer.cpp b/image/SourceBuffer.cpp index de066e29fc..5961cc66ff 100644 --- a/image/SourceBuffer.cpp +++ b/image/SourceBuffer.cpp @@ -204,30 +204,27 @@ SourceBuffer::Compact() return NS_OK; } - Maybe newChunk = CreateChunk(length, /* aRoundUp = */ false); - if (MOZ_UNLIKELY(!newChunk || newChunk->AllocationFailed())) { - NS_WARNING("Failed to allocate chunk for SourceBuffer compacting - OOM?"); + Chunk& mergeChunk = mChunks[0]; + if (MOZ_UNLIKELY(!mergeChunk.SetCapacity(length))) { + NS_WARNING("Failed to reallocate chunk for SourceBuffer compacting - OOM?"); return NS_OK; } - // Copy our old chunks into the new chunk. - for (uint32_t i = 0 ; i < mChunks.Length() ; ++i) { - size_t offset = newChunk->Length(); - MOZ_ASSERT(offset < newChunk->Capacity()); - MOZ_ASSERT(offset + mChunks[i].Length() <= newChunk->Capacity()); + // Copy our old chunks into the newly reallocated first chunk. + for (uint32_t i = 1 ; i < mChunks.Length() ; ++i) { + size_t offset = mergeChunk.Length(); + MOZ_ASSERT(offset < mergeChunk.Capacity()); + MOZ_ASSERT(offset + mChunks[i].Length() <= mergeChunk.Capacity()); - memcpy(newChunk->Data() + offset, mChunks[i].Data(), mChunks[i].Length()); - newChunk->AddLength(mChunks[i].Length()); + memcpy(mergeChunk.Data() + offset, mChunks[i].Data(), mChunks[i].Length()); + mergeChunk.AddLength(mChunks[i].Length()); } - MOZ_ASSERT(newChunk->Length() == newChunk->Capacity(), + MOZ_ASSERT(mergeChunk.Length() == mergeChunk.Capacity(), "Compacted chunk has slack space"); - // Replace the old chunks with the new, compact chunk. - mChunks.Clear(); - if (MOZ_UNLIKELY(NS_FAILED(AppendChunk(Move(newChunk))))) { - return HandleError(NS_ERROR_OUT_OF_MEMORY); - } + // Remove the redundant chunks. + mChunks.RemoveElementsAt(1, mChunks.Length() - 1); mChunks.Compact(); return NS_OK; @@ -317,7 +314,7 @@ SourceBuffer::ExpectLength(size_t aExpectedLength) 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); } diff --git a/image/SourceBuffer.h b/image/SourceBuffer.h index e5aff1dcdd..6e3ef7a538 100644 --- a/image/SourceBuffer.h +++ b/image/SourceBuffer.h @@ -358,7 +358,7 @@ private: // Chunk type and chunk-related methods. ////////////////////////////////////////////////////////////////////////////// - class Chunk + class Chunk final { public: explicit Chunk(size_t aCapacity) @@ -366,13 +366,18 @@ private: , mLength(0) { MOZ_ASSERT(aCapacity > 0, "Creating zero-capacity chunk"); - mData.reset(new (fallible) char[mCapacity]); + mData = static_cast(malloc(mCapacity)); + } + + ~Chunk() + { + free(mData); } Chunk(Chunk&& aOther) : mCapacity(aOther.mCapacity) , mLength(aOther.mLength) - , mData(Move(aOther.mData)) + , mData(aOther.mData) { aOther.mCapacity = aOther.mLength = 0; aOther.mData = nullptr; @@ -380,9 +385,10 @@ private: Chunk& operator=(Chunk&& aOther) { + free(mData); mCapacity = aOther.mCapacity; mLength = aOther.mLength; - mData = Move(aOther.mData); + mData = aOther.mData; aOther.mCapacity = aOther.mLength = 0; aOther.mData = nullptr; return *this; @@ -395,7 +401,7 @@ private: char* Data() const { MOZ_ASSERT(mData, "Allocation failed but nobody checked for it"); - return mData.get(); + return mData; } void AddLength(size_t aAdditionalLength) @@ -404,13 +410,26 @@ private: mLength += aAdditionalLength; } + bool SetCapacity(size_t aCapacity) + { + MOZ_ASSERT(mData, "Allocation failed but nobody checked for it"); + char* data = static_cast(realloc(mData, aCapacity)); + if (!data) { + return false; + } + + mData = data; + mCapacity = aCapacity; + return true; + } + private: Chunk(const Chunk&) = delete; Chunk& operator=(const Chunk&) = delete; size_t mCapacity; size_t mLength; - UniquePtr mData; + char* mData; }; nsresult AppendChunk(Maybe&& aChunk); @@ -454,7 +473,7 @@ private: mutable Mutex mMutex; /// The data in this SourceBuffer, stored as a series of Chunks. - FallibleTArray mChunks; + AutoTArray mChunks; /// Consumers which are waiting to be notified when new data is available. nsTArray> mWaitingConsumers; diff --git a/image/imgTools.cpp b/image/imgTools.cpp index 29905c1ab3..3ac31102e5 100644 --- a/image/imgTools.cpp +++ b/image/imgTools.cpp @@ -67,15 +67,6 @@ imgTools::DecodeImage(nsIInputStream* aInStr, NS_ENSURE_ARG_POINTER(aInStr); - // Create a new image container to hold the decoded data. - nsAutoCString mimeType(aMimeType); - RefPtr image = ImageFactory::CreateAnonymousImage(mimeType); - RefPtr tracker = image->GetProgressTracker(); - - if (image->HasError()) { - return NS_ERROR_FAILURE; - } - // Prepare the input stream. nsCOMPtr inStream = aInStr; if (!NS_InputStreamIsBuffered(aInStr)) { @@ -92,6 +83,16 @@ imgTools::DecodeImage(nsIInputStream* aInStr, NS_ENSURE_SUCCESS(rv, rv); NS_ENSURE_TRUE(length <= UINT32_MAX, NS_ERROR_FILE_TOO_BIG); + // Create a new image container to hold the decoded data. + nsAutoCString mimeType(aMimeType); + RefPtr image = + ImageFactory::CreateAnonymousImage(mimeType, uint32_t(length)); + RefPtr tracker = image->GetProgressTracker(); + + if (image->HasError()) { + return NS_ERROR_FAILURE; + } + // Send the source data to the Image. rv = image->OnImageDataAvailable(nullptr, nullptr, inStream, 0, uint32_t(length)); diff --git a/toolkit/components/places/PageIconProtocolHandler.js b/toolkit/components/places/PageIconProtocolHandler.js index 05e43ccf3e..30bd8f1857 100644 --- a/toolkit/components/places/PageIconProtocolHandler.js +++ b/toolkit/components/places/PageIconProtocolHandler.js @@ -92,6 +92,7 @@ PageIconProtocolHandler.prototype = { try { channel.contentType = mime; + channel.contentLength = len; // Pass the icon data to the output stream. let stream = Cc["@mozilla.org/binaryoutputstream;1"] .createInstance(Ci.nsIBinaryOutputStream);