Bug 1439047 - Part 1: Make StartupCache::PutBuffer take ownership of the buffer. r=froydnj
☠☠ backed out by 2511fb2fd5cf ☠ ☠
authorEric Rahm <erahm@mozilla.com>
Fri, 16 Feb 2018 15:30:47 -0800
changeset 444054 7f0cedfb4bd85bfe1a523168019864c9c6c0e665
parent 444053 633b2c102c95325b4038c37adb9b56f738165a9b
child 444055 718961d941d4d495426e59c6b5044302059489f9
push id8
push userbugmail@asutherland.org
push dateSat, 11 Aug 2018 16:11:21 +0000
reviewersfroydnj
bugs1439047
milestone60.0a1
Bug 1439047 - Part 1: Make StartupCache::PutBuffer take ownership of the buffer. r=froydnj This avoids a redundant alloc and copy in `PutBuffer`. All existing callers were destroying the passed in buffer after the call.
dom/xbl/nsXBLDocumentInfo.cpp
dom/xul/nsXULPrototypeCache.cpp
js/xpconnect/loader/mozJSLoaderUtils.cpp
startupcache/StartupCache.cpp
startupcache/StartupCache.h
startupcache/test/TestStartupCache.cpp
--- a/dom/xbl/nsXBLDocumentInfo.cpp
+++ b/dom/xbl/nsXBLDocumentInfo.cpp
@@ -293,17 +293,17 @@ nsXBLDocumentInfo::WritePrototypeBinding
   stream->Close();
   NS_ENSURE_SUCCESS(rv, rv);
 
   uint32_t len;
   UniquePtr<char[]> buf;
   rv = NewBufferFromStorageStream(storageStream, &buf, &len);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  return startupCache->PutBuffer(spec.get(), buf.get(), len);
+  return startupCache->PutBuffer(spec.get(), Move(buf), len);
 }
 
 void
 nsXBLDocumentInfo::SetFirstPrototypeBinding(nsXBLPrototypeBinding* aBinding)
 {
   mFirstBinding = aBinding;
 }
 
--- a/dom/xul/nsXULPrototypeCache.cpp
+++ b/dom/xul/nsXULPrototypeCache.cpp
@@ -441,17 +441,17 @@ nsXULPrototypeCache::FinishOutputStream(
     rv = NewBufferFromStorageStream(storageStream, &buf, &len);
     NS_ENSURE_SUCCESS(rv, rv);
 
     if (!mStartupCacheURITable.GetEntry(uri)) {
         nsAutoCString spec(kXULCachePrefix);
         rv = PathifyURI(uri, spec);
         if (NS_FAILED(rv))
             return NS_ERROR_NOT_AVAILABLE;
-        rv = sc->PutBuffer(spec.get(), buf.get(), len);
+        rv = sc->PutBuffer(spec.get(), Move(buf), len);
         if (NS_SUCCEEDED(rv)) {
             mOutputStreamTable.Remove(uri);
             mStartupCacheURITable.PutEntry(uri);
         }
     }
 
     return rv;
 }
@@ -586,17 +586,17 @@ nsXULPrototypeCache::BeginCaching(nsIURI
                 rv = NS_ERROR_FILE_TOO_BIG;
             }
         }
 
         if (NS_SUCCEEDED(rv)) {
             buf = MakeUnique<char[]>(len);
             rv = inputStream->Read(buf.get(), len, &amtRead);
             if (NS_SUCCEEDED(rv) && len == amtRead)
-              rv = startupCache->PutBuffer(kXULCacheInfoKey, buf.get(), len);
+              rv = startupCache->PutBuffer(kXULCacheInfoKey, Move(buf), len);
             else {
                 rv = NS_ERROR_UNEXPECTED;
             }
         }
 
         // Failed again, just bail.
         if (NS_FAILED(rv)) {
             startupCache->InvalidateCache();
--- a/js/xpconnect/loader/mozJSLoaderUtils.cpp
+++ b/js/xpconnect/loader/mozJSLoaderUtils.cpp
@@ -57,13 +57,16 @@ WriteCachedScript(StartupCache* cache, n
         MOZ_ASSERT((code & JS::TranscodeResult_Throw) != 0);
         JS_ClearPendingException(cx);
         return NS_ERROR_OUT_OF_MEMORY;
     }
 
     size_t size = buffer.length();
     if (size > UINT32_MAX)
         return NS_ERROR_FAILURE;
+
+    // Move the vector buffer into a unique pointer buffer.
+    UniquePtr<char[]> buf(reinterpret_cast<char*>(buffer.extractOrCopyRawBuffer()));
     nsresult rv = cache->PutBuffer(PromiseFlatCString(uri).get(),
-                                   reinterpret_cast<char*>(buffer.begin()),
+                                   Move(buf),
                                    size);
     return rv;
 }
--- a/startupcache/StartupCache.cpp
+++ b/startupcache/StartupCache.cpp
@@ -297,27 +297,24 @@ StartupCache::GetBuffer(const char* id, 
 
   omnijar = mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE);
   // no need to checksum omnijarred entries
   return GetBufferFromZipArchive(omnijar, false, id, outbuf, length);
 }
 
 // Makes a copy of the buffer, client retains ownership of inbuf.
 nsresult
-StartupCache::PutBuffer(const char* id, const char* inbuf, uint32_t len)
+StartupCache::PutBuffer(const char* id, UniquePtr<char[]>&& inbuf, uint32_t len)
 {
   NS_ASSERTION(NS_IsMainThread(), "Startup cache only available on main thread");
   WaitOnWriteThread();
   if (StartupCache::gShutdownInitiated) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  auto data = MakeUnique<char[]>(len);
-  memcpy(data.get(), inbuf, len);
-
   nsCString idStr(id);
   // Cache it for now, we'll write all together later.
   CacheEntry* entry;
 
   if (mTable.Get(idStr)) {
     NS_WARNING("Existing entry in StartupCache.");
     // Double-caching is undesirable but not an error.
     return NS_OK;
@@ -325,17 +322,17 @@ StartupCache::PutBuffer(const char* id, 
 
 #ifdef DEBUG
   if (mArchive) {
     nsZipItem* zipItem = mArchive->GetItem(id);
     NS_ASSERTION(zipItem == nullptr, "Existing entry in disk StartupCache.");
   }
 #endif
 
-  entry = new CacheEntry(Move(data), len);
+  entry = new CacheEntry(Move(inbuf), len);
   mTable.Put(idStr, entry);
   mPendingWrites.AppendElement(idStr);
   return ResetStartupWriteTimer();
 }
 
 size_t
 StartupCache::SizeOfMapping()
 {
--- a/startupcache/StartupCache.h
+++ b/startupcache/StartupCache.h
@@ -108,18 +108,18 @@ public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIMEMORYREPORTER
 
   // StartupCache methods. See above comments for a more detailed description.
 
   // Returns a buffer that was previously stored, caller takes ownership.
   nsresult GetBuffer(const char* id, UniquePtr<char[]>* outbuf, uint32_t* length);
 
-  // Stores a buffer. Caller keeps ownership, we make a copy.
-  nsresult PutBuffer(const char* id, const char* inbuf, uint32_t length);
+  // Stores a buffer. Caller yields ownership.
+  nsresult PutBuffer(const char* id, UniquePtr<char[]>&& inbuf, uint32_t length);
 
   // Removes the cache file.
   void InvalidateCache();
 
   // Signal that data should not be loaded from the cache file
   static void IgnoreDiskCache();
 
   // In DEBUG builds, returns a stream that will attempt to check for
--- a/startupcache/test/TestStartupCache.cpp
+++ b/startupcache/test/TestStartupCache.cpp
@@ -91,17 +91,17 @@ TEST_F(TestStartupCache, StartupWriteRea
   nsresult rv;
   StartupCache* sc = StartupCache::GetSingleton();
 
   const char* buf = "Market opportunities for BeardBook";
   const char* id = "id";
   UniquePtr<char[]> outbuf;
   uint32_t len;
 
-  rv = sc->PutBuffer(id, buf, strlen(buf) + 1);
+  rv = sc->PutBuffer(id, UniquePtr<char[]>(strdup(buf)), strlen(buf) + 1);
   EXPECT_TRUE(NS_SUCCEEDED(rv));
 
   rv = sc->GetBuffer(id, &outbuf, &len);
   EXPECT_TRUE(NS_SUCCEEDED(rv));
   EXPECT_STREQ(buf, outbuf.get());
 
   rv = sc->ResetStartupWriteTimer();
   EXPECT_TRUE(NS_SUCCEEDED(rv));
@@ -117,17 +117,17 @@ TEST_F(TestStartupCache, WriteInvalidate
   nsresult rv;
   const char* buf = "BeardBook competitive analysis";
   const char* id = "id";
   UniquePtr<char[]> outbuf;
   uint32_t len;
   StartupCache* sc = StartupCache::GetSingleton();
   ASSERT_TRUE(sc);
 
-  rv = sc->PutBuffer(id, buf, strlen(buf) + 1);
+  rv = sc->PutBuffer(id, UniquePtr<char[]>(strdup(buf)), strlen(buf) + 1);
   EXPECT_TRUE(NS_SUCCEEDED(rv));
 
   sc->InvalidateCache();
 
   rv = sc->GetBuffer(id, &outbuf, &len);
   EXPECT_EQ(rv, NS_ERROR_NOT_AVAILABLE);
 }
 
@@ -172,17 +172,17 @@ TEST_F(TestStartupCache, WriteObject)
   EXPECT_TRUE(NS_SUCCEEDED(rv));
 
   UniquePtr<char[]> buf;
   uint32_t len;
   NewBufferFromStorageStream(storageStream, &buf, &len);
 
   // Since this is a post-startup write, it should be written and
   // available.
-  rv = sc->PutBuffer(id, buf.get(), len);
+  rv = sc->PutBuffer(id, Move(buf), len);
   EXPECT_TRUE(NS_SUCCEEDED(rv));
 
   UniquePtr<char[]> buf2;
   uint32_t len2;
   nsCOMPtr<nsIObjectInputStream> objectInput;
   rv = sc->GetBuffer(id, &buf2, &len2);
   EXPECT_TRUE(NS_SUCCEEDED(rv));