Bug 1205358 - Fix up nsUrlClassifierPrefixSet memory reporting. r=gcp.
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 10 Feb 2016 08:30:48 +1100
changeset 283621 f3173fd2f161e8f4dcf6f45452e11cbca75ff657
parent 283620 122ae2d1cf825ec75509233c5cc26db6df2b2b99
child 283622 f5c043dfcb47a771044782ee5ee57334e31f234f
push id71615
push usernnethercote@mozilla.com
push dateTue, 09 Feb 2016 22:31:37 +0000
treeherdermozilla-inbound@f3173fd2f161 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgcp
bugs1205358, 1050108
milestone47.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 1205358 - Fix up nsUrlClassifierPrefixSet memory reporting. r=gcp. This patch reverts the "measure-in-advance" approach added in part 1 of bug 1050108 -- because that doesn't interact well with DMD -- and adds locking to avoid races between the url-classifier thread and the main thread.
toolkit/components/url-classifier/LookupCache.cpp
toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp
toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h
--- a/toolkit/components/url-classifier/LookupCache.cpp
+++ b/toolkit/components/url-classifier/LookupCache.cpp
@@ -579,17 +579,17 @@ LookupCache::ConstructPrefixSet(AddPrefi
   // construct new one, replace old entries
   nsresult rv = mPrefixSet->SetPrefixes(array.Elements(), array.Length());
   if (NS_FAILED(rv)) {
     goto error_bailout;
   }
 
 #ifdef DEBUG
   uint32_t size;
-  size = mPrefixSet->SizeInMemory();
+  size = mPrefixSet->SizeOfIncludingThis(moz_malloc_size_of);
   LOG(("SB tree done, size = %d bytes\n", size));
 #endif
 
   mPrimed = true;
 
   return NS_OK;
 
  error_bailout:
@@ -622,17 +622,17 @@ LookupCache::LoadPrefixSet()
     }
     mPrimed = true;
   } else {
     LOG(("no (usable) stored PrefixSet found"));
   }
 
 #ifdef DEBUG
   if (mPrimed) {
-    uint32_t size = mPrefixSet->SizeInMemory();
+    uint32_t size = mPrefixSet->SizeOfIncludingThis(moz_malloc_size_of);
     LOG(("SB tree done, size = %d bytes\n", size));
   }
 #endif
 
   return NS_OK;
 }
 
 nsresult
--- a/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp
@@ -30,21 +30,19 @@ using namespace mozilla;
 // NSPR_LOG_MODULES=UrlClassifierPrefixSet:5
 static const PRLogModuleInfo *gUrlClassifierPrefixSetLog = nullptr;
 #define LOG(args) MOZ_LOG(gUrlClassifierPrefixSetLog, mozilla::LogLevel::Debug, args)
 #define LOG_ENABLED() MOZ_LOG_TEST(gUrlClassifierPrefixSetLog, mozilla::LogLevel::Debug)
 
 NS_IMPL_ISUPPORTS(
   nsUrlClassifierPrefixSet, nsIUrlClassifierPrefixSet, nsIMemoryReporter)
 
-MOZ_DEFINE_MALLOC_SIZE_OF(UrlClassifierMallocSizeOf)
-
 nsUrlClassifierPrefixSet::nsUrlClassifierPrefixSet()
-  : mTotalPrefixes(0)
-  , mMemoryInUse(0)
+  : mLock("nsUrlClassifierPrefixSet.mLock")
+  , mTotalPrefixes(0)
   , mMemoryReportPath()
 {
   if (!gUrlClassifierPrefixSetLog)
     gUrlClassifierPrefixSetLog = PR_NewLogModule("UrlClassifierPrefixSet");
 }
 
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::Init(const nsACString& aName)
@@ -63,37 +61,39 @@ nsUrlClassifierPrefixSet::Init(const nsA
 nsUrlClassifierPrefixSet::~nsUrlClassifierPrefixSet()
 {
   UnregisterWeakMemoryReporter(this);
 }
 
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::SetPrefixes(const uint32_t* aArray, uint32_t aLength)
 {
+  MutexAutoLock lock(mLock);
+
   nsresult rv = NS_OK;
 
   if (aLength <= 0) {
     if (mIndexPrefixes.Length() > 0) {
       LOG(("Clearing PrefixSet"));
       mIndexDeltas.Clear();
       mIndexPrefixes.Clear();
       mTotalPrefixes = 0;
     }
   } else {
     rv = MakePrefixSet(aArray, aLength);
   }
 
-  mMemoryInUse = SizeOfIncludingThis(UrlClassifierMallocSizeOf);
-
   return rv;
 }
 
 nsresult
 nsUrlClassifierPrefixSet::MakePrefixSet(const uint32_t* aPrefixes, uint32_t aLength)
 {
+  mLock.AssertCurrentThreadOwns();
+
   if (aLength == 0) {
     return NS_OK;
   }
 
 #ifdef DEBUG
   for (uint32_t i = 1; i < aLength; i++) {
     MOZ_ASSERT(aPrefixes[i] >= aPrefixes[i-1]);
   }
@@ -133,16 +133,18 @@ nsUrlClassifierPrefixSet::MakePrefixSet(
   LOG(("Total number of delta chunks: %d", mIndexDeltas.Length()));
 
   return NS_OK;
 }
 
 nsresult
 nsUrlClassifierPrefixSet::GetPrefixesNative(FallibleTArray<uint32_t>& outArray)
 {
+  MutexAutoLock lock(mLock);
+
   if (!outArray.SetLength(mTotalPrefixes, fallible)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   uint32_t prefixIdxLength = mIndexPrefixes.Length();
   uint32_t prefixCnt = 0;
 
   for (uint32_t i = 0; i < prefixIdxLength; i++) {
@@ -158,16 +160,19 @@ nsUrlClassifierPrefixSet::GetPrefixesNat
   NS_ASSERTION(mTotalPrefixes == prefixCnt, "Lengths are inconsistent");
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::GetPrefixes(uint32_t* aCount,
                                       uint32_t** aPrefixes)
 {
+  // No need to get mLock here because this function does not directly touch
+  // the class's data members. (GetPrefixesNative() will get mLock, however.)
+
   NS_ENSURE_ARG_POINTER(aCount);
   *aCount = 0;
   NS_ENSURE_ARG_POINTER(aPrefixes);
   *aPrefixes = nullptr;
 
   FallibleTArray<uint32_t> prefixes;
   nsresult rv = GetPrefixesNative(prefixes);
   if (NS_FAILED(rv)) {
@@ -185,16 +190,18 @@ nsUrlClassifierPrefixSet::GetPrefixes(ui
 
   return NS_OK;
 }
 
 uint32_t nsUrlClassifierPrefixSet::BinSearch(uint32_t start,
                                              uint32_t end,
                                              uint32_t target)
 {
+  mLock.AssertCurrentThreadOwns();
+
   while (start != end && end >= start) {
     uint32_t i = start + ((end - start) >> 1);
     uint32_t value = mIndexPrefixes[i];
     if (value < target) {
       start = i + 1;
     } else if (value > target) {
       end = i - 1;
     } else {
@@ -202,16 +209,18 @@ uint32_t nsUrlClassifierPrefixSet::BinSe
     }
   }
   return end;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::Contains(uint32_t aPrefix, bool* aFound)
 {
+  MutexAutoLock lock(mLock);
+
   *aFound = false;
 
   if (mIndexPrefixes.Length() == 0) {
     return NS_OK;
   }
 
   uint32_t target = aPrefix;
 
@@ -246,50 +255,64 @@ nsUrlClassifierPrefixSet::Contains(uint3
 
   if (diff == 0) {
     *aFound = true;
   }
 
   return NS_OK;
 }
 
+MOZ_DEFINE_MALLOC_SIZE_OF(UrlClassifierMallocSizeOf)
+
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::CollectReports(nsIHandleReportCallback* aHandleReport,
                                          nsISupports* aData, bool aAnonymize)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+
+  // No need to get mLock here because this function does not directly touch
+  // the class's data members. (SizeOfIncludingThis() will get mLock, however.)
+
+  size_t amount = SizeOfIncludingThis(UrlClassifierMallocSizeOf);
+
   return aHandleReport->Callback(
-    EmptyCString(), mMemoryReportPath, KIND_HEAP, UNITS_BYTES,
-    mMemoryInUse,
+    EmptyCString(), mMemoryReportPath, KIND_HEAP, UNITS_BYTES, amount,
     NS_LITERAL_CSTRING("Memory used by the prefix set for a URL classifier."),
     aData);
 }
 
 size_t
 nsUrlClassifierPrefixSet::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)
 {
+  MutexAutoLock lock(mLock);
+
   size_t n = 0;
   n += aMallocSizeOf(this);
   n += mIndexDeltas.ShallowSizeOfExcludingThis(aMallocSizeOf);
   for (uint32_t i = 0; i < mIndexDeltas.Length(); i++) {
     n += mIndexDeltas[i].ShallowSizeOfExcludingThis(aMallocSizeOf);
   }
   n += mIndexPrefixes.ShallowSizeOfExcludingThis(aMallocSizeOf);
   return n;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::IsEmpty(bool * aEmpty)
 {
+  MutexAutoLock lock(mLock);
+
   *aEmpty = (mIndexPrefixes.Length() == 0);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::LoadFromFile(nsIFile* aFile)
 {
+  MutexAutoLock lock(mLock);
+
   Telemetry::AutoTimer<Telemetry::URLCLASSIFIER_PS_FILELOAD_TIME> timer;
 
   nsCOMPtr<nsIInputStream> localInFile;
   nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(localInFile), aFile,
                                            PR_RDONLY | nsIFile::OS_READAHEAD);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Convert to buffered stream
@@ -360,24 +383,24 @@ nsUrlClassifierPrefixSet::LoadFromFile(n
   } else {
     LOG(("Version magic mismatch, not loading"));
     return NS_ERROR_FILE_CORRUPTED;
   }
 
   MOZ_ASSERT(mIndexPrefixes.Length() == mIndexDeltas.Length());
   LOG(("Loading PrefixSet successful"));
 
-  mMemoryInUse = SizeOfIncludingThis(UrlClassifierMallocSizeOf);
-
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::StoreToFile(nsIFile* aFile)
 {
+  MutexAutoLock lock(mLock);
+
   nsCOMPtr<nsIOutputStream> localOutFile;
   nsresult rv = NS_NewLocalFileOutputStream(getter_AddRefs(localOutFile), aFile,
                                             PR_WRONLY | PR_TRUNCATE | PR_CREATE_FILE);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Preallocate the file storage
   {
     nsCOMPtr<nsIFileOutputStream> fos(do_QueryInterface(localOutFile));
--- a/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h
@@ -10,19 +10,19 @@
 #include "nsISupportsUtils.h"
 #include "nsID.h"
 #include "nsIFile.h"
 #include "nsIMemoryReporter.h"
 #include "nsIMutableArray.h"
 #include "nsIUrlClassifierPrefixSet.h"
 #include "nsTArray.h"
 #include "nsToolkitCompsCID.h"
+#include "mozilla/FileUtils.h"
 #include "mozilla/MemoryReporting.h"
-#include "mozilla/FileUtils.h"
-#include "mozilla/Atomics.h"
+#include "mozilla/Mutex.h"
 
 class nsUrlClassifierPrefixSet final
   : public nsIUrlClassifierPrefixSet
   , public nsIMemoryReporter
 {
 public:
   nsUrlClassifierPrefixSet();
 
@@ -30,45 +30,45 @@ public:
   NS_IMETHOD SetPrefixes(const uint32_t* aArray, uint32_t aLength) override;
   NS_IMETHOD GetPrefixes(uint32_t* aCount, uint32_t** aPrefixes) override;
   NS_IMETHOD Contains(uint32_t aPrefix, bool* aFound) override;
   NS_IMETHOD IsEmpty(bool* aEmpty) override;
   NS_IMETHOD LoadFromFile(nsIFile* aFile) override;
   NS_IMETHOD StoreToFile(nsIFile* aFile) override;
 
   nsresult GetPrefixesNative(FallibleTArray<uint32_t>& outArray);
-  size_t SizeInMemory() { return mMemoryInUse; };
+
+  size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
 
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIMEMORYREPORTER
 
-protected:
+private:
   virtual ~nsUrlClassifierPrefixSet();
 
   static const uint32_t BUFFER_SIZE = 64 * 1024;
   static const uint32_t DELTAS_LIMIT = 120;
   static const uint32_t MAX_INDEX_DIFF = (1 << 16);
   static const uint32_t PREFIXSET_VERSION_MAGIC = 1;
 
   nsresult MakePrefixSet(const uint32_t* aArray, uint32_t aLength);
   uint32_t BinSearch(uint32_t start, uint32_t end, uint32_t target);
 
-  // Return the estimated size of the set on disk and in memory, in bytes.
-  size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
-
+  // Lock to prevent races between the url-classifier thread (which does most
+  // of the operations) and the main thread (which does memory reporting).
+  // It should be held for all operations between Init() and destruction that
+  // touch this class's data members.
+  mozilla::Mutex mLock;
   // list of fully stored prefixes, that also form the
   // start of a run of deltas in mIndexDeltas.
   nsTArray<uint32_t> mIndexPrefixes;
   // array containing arrays of deltas from indices.
   // Index to the place that matches the closest lower
   // prefix from mIndexPrefix. Then every "delta" corresponds
   // to a prefix in the PrefixSet.
   nsTArray<nsTArray<uint16_t> > mIndexDeltas;
   // how many prefixes we have.
   uint32_t mTotalPrefixes;
 
-  // memory report collection might happen while we're updating the prefixset
-  // on another thread, so pre-compute and remember the size (bug 1050108).
-  mozilla::Atomic<size_t> mMemoryInUse;
   nsCString mMemoryReportPath;
 };
 
 #endif