Bug 1470771: Part 1 - Don't evict shared bundles from the bundle cache. r=erahm draft
authorKris Maglione <maglione.k@gmail.com>
Sun, 24 Jun 2018 14:32:19 -0700
changeset 812728 dd719a2c7210358995c14fee72dd8e1291d8e5b3
parent 812727 de745559f295843ae1ec520892acab79afde0fd7
child 812729 30e2e195018e1a17acaabc318327ccc798b934fd
push id114661
push usermaglione.k@gmail.com
push dateFri, 29 Jun 2018 22:31:07 +0000
reviewerserahm
bugs1470771
milestone62.0a1
Bug 1470771: Part 1 - Don't evict shared bundles from the bundle cache. r=erahm MozReview-Commit-ID: 6BLN4hOH5fW
intl/strres/nsStringBundle.cpp
intl/strres/nsStringBundle.h
intl/strres/nsStringBundleService.h
js/xpconnect/loader/AutoMemMap.cpp
js/xpconnect/loader/AutoMemMap.h
--- a/intl/strres/nsStringBundle.cpp
+++ b/intl/strres/nsStringBundle.cpp
@@ -675,17 +675,17 @@ NS_IMPL_ISUPPORTS(nsStringBundleService,
                   nsIStringBundleService,
                   nsIObserver,
                   nsISupportsWeakReference,
                   nsIMemoryReporter)
 
 nsStringBundleService::~nsStringBundleService()
 {
   UnregisterWeakMemoryReporter(this);
-  flushBundleCache();
+  flushBundleCache(false);
 }
 
 nsresult
 nsStringBundleService::Init()
 {
   nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
   if (os) {
     os->AddObserver(this, "memory-pressure", true);
@@ -710,48 +710,51 @@ nsStringBundleService::SizeOfIncludingTh
   return aMallocSizeOf(this) + n;
 }
 
 NS_IMETHODIMP
 nsStringBundleService::Observe(nsISupports* aSubject,
                                const char* aTopic,
                                const char16_t* aSomeData)
 {
-  if (strcmp("memory-pressure", aTopic) == 0 ||
-      strcmp("profile-do-change", aTopic) == 0 ||
+  if (strcmp("profile-do-change", aTopic) == 0 ||
       strcmp("chrome-flush-caches", aTopic) == 0 ||
       strcmp("intl:app-locales-changed", aTopic) == 0)
   {
-    flushBundleCache();
+    flushBundleCache(false);
+  } else if (strcmp("memory-pressure", aTopic) == 0) {
+    flushBundleCache(true);
   }
 
   return NS_OK;
 }
 
 void
-nsStringBundleService::flushBundleCache()
+nsStringBundleService::flushBundleCache(bool ignoreShared)
 {
-  // release all bundles in the cache
-  mBundleMap.Clear();
+  LinkedList<bundleCacheEntry_t> newList;
 
   while (!mBundleCache.isEmpty()) {
-    delete mBundleCache.popFirst();
+    UniquePtr<bundleCacheEntry_t> entry(mBundleCache.popFirst());
+    auto* bundle = nsStringBundleBase::Cast(entry->mBundle);
+
+    if (ignoreShared && bundle->IsShared()) {
+      newList.insertBack(entry.release());
+    } else {
+      mBundleMap.Remove(entry->mHashKey);
+    }
   }
 
-  // We never flush shared bundles, since their memory cannot be freed, so add
-  // them back to the map.
-  for (auto* entry : mSharedBundles) {
-    mBundleMap.Put(entry->mHashKey, entry);
-  }
+  mBundleCache = std::move(newList);
 }
 
 NS_IMETHODIMP
 nsStringBundleService::FlushBundles()
 {
-  flushBundleCache();
+  flushBundleCache(false);
   return NS_OK;
 }
 
 void
 nsStringBundleService::SendContentBundles(ContentParent* aContentParent) const
 {
   nsTArray<StringBundleDescriptor> bundles;
 
@@ -800,24 +803,21 @@ nsStringBundleService::getStringBundle(c
                                        nsIStringBundle **aResult)
 {
   nsDependentCString key(aURLSpec);
   bundleCacheEntry_t* cacheEntry = mBundleMap.Get(key);
 
   RefPtr<SharedStringBundle> shared;
 
   if (cacheEntry) {
-    // cache hit!
-    // remove it from the list, it will later be reinserted
-    // at the head of the list
+    // Remove the entry from the list so it can be re-inserted at the back.
     cacheEntry->remove();
 
     shared = do_QueryObject(cacheEntry->mBundle);
   } else {
-    // hasn't been cached, so insert it into the hash table
     nsCOMPtr<nsIStringBundle> bundle;
     bool isContent = IsContentBundle(key);
     if (!isContent || !XRE_IsParentProcess()) {
       bundle = new nsStringBundle(aURLSpec);
     }
 
     // If this is a bundle which is used by the content processes, we want to
     // load it into a shared memory region.
@@ -842,58 +842,58 @@ nsStringBundleService::getStringBundle(c
     }
 
     cacheEntry = insertIntoCache(bundle.forget(), key);
   }
 
   if (shared) {
     mSharedBundles.insertBack(cacheEntry);
   } else {
-    // at this point the cacheEntry should exist in the hashtable,
-    // but is not in the LRU cache.
-    // put the cache entry at the front of the list
-    mBundleCache.insertFront(cacheEntry);
+    mBundleCache.insertBack(cacheEntry);
   }
 
   // finally, return the value
   *aResult = cacheEntry->mBundle;
   NS_ADDREF(*aResult);
 }
 
-bundleCacheEntry_t *
+UniquePtr<bundleCacheEntry_t>
+nsStringBundleService::evictOneEntry()
+{
+  for (auto* entry : mBundleCache) {
+    auto* bundle = nsStringBundleBase::Cast(entry->mBundle);
+    if (!bundle->IsShared()) {
+      entry->remove();
+      mBundleMap.Remove(entry->mHashKey);
+      return UniquePtr<bundleCacheEntry_t>(entry);
+    }
+  }
+  return nullptr;
+}
+
+bundleCacheEntry_t*
 nsStringBundleService::insertIntoCache(already_AddRefed<nsIStringBundle> aBundle,
                                        const nsACString& aHashKey)
 {
-  bundleCacheEntry_t *cacheEntry;
+  UniquePtr<bundleCacheEntry_t> cacheEntry;
 
-  if (mBundleMap.Count() < MAX_CACHED_BUNDLES ||
-      mBundleCache.isEmpty()) {
-    // cache not full - create a new entry
-    cacheEntry = new bundleCacheEntry_t();
-  } else {
-    // cache is full
-    // take the last entry in the list, and recycle it.
-    cacheEntry = mBundleCache.getLast();
-
-    // remove it from the hash table and linked list
-    NS_ASSERTION(mBundleMap.Contains(cacheEntry->mHashKey),
-                 "Element will not be removed!");
-    mBundleMap.Remove(cacheEntry->mHashKey);
-    cacheEntry->remove();
+  if (mBundleMap.Count() >= MAX_CACHED_BUNDLES) {
+    cacheEntry = evictOneEntry();
   }
 
-  // at this point we have a new cacheEntry that doesn't exist
-  // in the hashtable, so set up the cacheEntry
+  if (!cacheEntry) {
+    cacheEntry.reset(new bundleCacheEntry_t());
+  }
+
   cacheEntry->mHashKey = aHashKey;
   cacheEntry->mBundle = aBundle;
 
-  // insert the entry into the cache and map, make it the MRU
-  mBundleMap.Put(cacheEntry->mHashKey, cacheEntry);
+  mBundleMap.Put(cacheEntry->mHashKey, cacheEntry.get());
 
-  return cacheEntry;
+  return cacheEntry.release();
 }
 
 NS_IMETHODIMP
 nsStringBundleService::CreateBundle(const char* aURLSpec,
                                     nsIStringBundle** aResult)
 {
   getStringBundle(aURLSpec,aResult);
   return NS_OK;
--- a/intl/strres/nsStringBundle.h
+++ b/intl/strres/nsStringBundle.h
@@ -23,16 +23,25 @@ public:
 
     NS_DECL_THREADSAFE_ISUPPORTS
     NS_DECL_NSISTRINGBUNDLE
 
     virtual nsresult LoadProperties() = 0;
 
     const nsCString& BundleURL() const { return mPropertiesURL; }
 
+    // Returns true if this bundle has more than one reference. If it has only
+    // a single reference, it is assumed to be held alive by the bundle cache.
+    bool IsShared() const { return mRefCnt > 1; }
+
+    static nsStringBundleBase* Cast(nsIStringBundle* aBundle)
+    {
+      return static_cast<nsStringBundleBase*>(aBundle);
+    }
+
 protected:
     virtual ~nsStringBundleBase();
 
     virtual nsresult GetStringImpl(const nsACString& aName, nsAString& aResult) = 0;
 
     virtual nsresult GetSimpleEnumerationImpl(nsISimpleEnumerator** elements) = 0;
 
     nsCString              mPropertiesURL;
--- a/intl/strres/nsStringBundleService.h
+++ b/intl/strres/nsStringBundleService.h
@@ -12,16 +12,17 @@
 #include "nsIPersistentProperties2.h"
 #include "nsIStringBundle.h"
 #include "nsIObserver.h"
 #include "nsWeakReference.h"
 #include "nsIErrorService.h"
 #include "nsIMemoryReporter.h"
 
 #include "mozilla/LinkedList.h"
+#include "mozilla/UniquePtr.h"
 
 struct bundleCacheEntry_t;
 
 class nsStringBundleService : public nsIStringBundleService,
                               public nsIObserver,
                               public nsSupportsWeakReference,
                               public nsIMemoryReporter
 {
@@ -59,21 +60,25 @@ public:
 private:
   virtual ~nsStringBundleService();
 
   void getStringBundle(const char *aUrl, nsIStringBundle** aResult);
   nsresult FormatWithBundle(nsIStringBundle* bundle, nsresult aStatus,
                             uint32_t argCount, char16_t** argArray,
                             nsAString& result);
 
-  void flushBundleCache();
+  void flushBundleCache(bool ignoreShared = true);
+
+  mozilla::UniquePtr<bundleCacheEntry_t> evictOneEntry();
 
   bundleCacheEntry_t* insertIntoCache(already_AddRefed<nsIStringBundle> aBundle,
                                       const nsACString &aHashKey);
 
   nsDataHashtable<nsCStringHashKey, bundleCacheEntry_t*> mBundleMap;
+  // LRU list of cached entries, with the least-recently-used entry first.
   mozilla::LinkedList<bundleCacheEntry_t> mBundleCache;
+  // List of cached shared-memory string bundles, in arbitrary order.
   mozilla::AutoCleanLinkedList<bundleCacheEntry_t> mSharedBundles;
 
   nsCOMPtr<nsIErrorService> mErrorService;
 };
 
 #endif
--- a/js/xpconnect/loader/AutoMemMap.cpp
+++ b/js/xpconnect/loader/AutoMemMap.cpp
@@ -101,22 +101,16 @@ AutoMemMap::init(const FileDescriptor& f
     MOZ_ASSERT(!fd);
     MOZ_ASSERT(!handle_);
     if (!file.IsValid()) {
         return Err(NS_ERROR_INVALID_ARG);
     }
 
     handle_ = file.ClonePlatformHandle().release();
 
-    return initInternal(prot, size);
-}
-
-Result<Ok, nsresult>
-AutoMemMap::initInternal(PRFileMapProtect prot, size_t size)
-{
     MOZ_ASSERT(!addr);
 
     size_ = size;
 
     addr = MapViewOfFile(
           handle_,
           prot == PR_PROT_READONLY ? FILE_MAP_READ : FILE_MAP_ALL_ACCESS,
           0, 0, size);
@@ -133,22 +127,16 @@ AutoMemMap::cloneHandle() const
 #else
 
 Result<Ok, nsresult>
 AutoMemMap::init(const FileDescriptor& file, size_t size, PRFileMapProtect prot)
 {
     return init(file, prot);
 }
 
-Result<Ok, nsresult>
-AutoMemMap::initInternal(PRFileMapProtect prot, size_t size)
-{
-    return initInternal(prot);
-}
-
 FileDescriptor
 AutoMemMap::cloneHandle() const
 {
     return cloneFileDescriptor();
 }
 
 #endif
 
--- a/js/xpconnect/loader/AutoMemMap.h
+++ b/js/xpconnect/loader/AutoMemMap.h
@@ -73,18 +73,16 @@ class AutoMemMap
         // Makes this mapping persistent. After calling this, the mapped memory
         // will remained mapped, even after this instance is destroyed.
         void setPersistent() { persistent_ = true; }
 
     private:
         Result<Ok, nsresult> initInternal(PRFileMapProtect prot = PR_PROT_READONLY,
                                           size_t expectedSize = 0);
 
-        Result<Ok, nsresult> initInternal(PRFileMapProtect prot, size_t size);
-
         AutoFDClose fd;
         PRFileMap* fileMap = nullptr;
 
 #ifdef XP_WIN
         // We can't include windows.h in this header, since it gets included
         // by some binding headers (which are explicitly incompatible with
         // windows.h). So we can't use the HANDLE type here.
         void* handle_ = nullptr;