Bug 1470771: Part 1 - Don't evict shared bundles from the bundle cache. r=erahm
authorKris Maglione <maglione.k@gmail.com>
Sun, 24 Jun 2018 14:32:19 -0700
changeset 424577 967409251f5cfdb7acc7616f6da2774677854aeb
parent 424576 f6b1c959442d70238ded401204860504eb3a601f
child 424578 34f927a06700696cb78789e9e77770054556212c
push id34217
push userrgurzau@mozilla.com
push dateSun, 01 Jul 2018 21:50:17 +0000
treeherdermozilla-central@3cfc35010196 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerserahm
bugs1470771
milestone63.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 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
--- 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(/* ignoreShared = */ 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(/* ignoreShared = */ false);
+  } else if (strcmp("memory-pressure", aTopic) == 0) {
+    flushBundleCache(/* ignoreShared = */ 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(/* ignoreShared = */ false);
   return NS_OK;
 }
 
 void
 nsStringBundleService::SendContentBundles(ContentParent* aContentParent) const
 {
   nsTArray<StringBundleDescriptor> bundles;
 
@@ -801,24 +804,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.
@@ -843,58 +843,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