Bug 529808 - Remove the static atom table. r=froydnj
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 28 Feb 2018 07:34:12 +1100
changeset 460684 cb5de5ee2eadee9421639d3eb72d42a0444e05db
parent 460683 1e97b5ef21b648fa1aa2f05d9c266c456a65c9e9
child 460685 b9d175b75ba44acb9208d35d1142afd1e7aa8f73
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs529808, 1275755, 1440824, 1352874
milestone60.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 529808 - Remove the static atom table. r=froydnj Various atom-related things have improved recently. - The main atom table is now threadsafe (bug 1275755) and so can be accessed on any thread. It has also been split into pieces (bug 1440824), which greatly reduces lock contention. - A cache has been added to the HTML5 parser (bug 1352874) that removes the need for most of the full table lookups. As a result, there is no point having a separate static atom table. This patch removes it. MozReview-Commit-ID: 8ou1BrnPAwd
layout/build/nsLayoutStatics.cpp
toolkit/components/aboutmemory/tests/test_memoryReporters.xul
xpcom/base/nsMemoryReporterManager.cpp
xpcom/ds/nsAtom.h
xpcom/ds/nsAtomTable.cpp
xpcom/ds/nsAtomTable.h
--- a/layout/build/nsLayoutStatics.cpp
+++ b/layout/build/nsLayoutStatics.cpp
@@ -153,17 +153,17 @@ nsLayoutStatics::Initialize()
   nsCSSPseudoElements::AddRefAtoms();
   nsCSSKeywords::AddRefTable();
   nsCSSProps::AddRefTable();
   nsColorNames::AddRefTable();
   nsGkAtoms::AddRefAtoms();
   nsHTMLTags::RegisterAtoms();
   nsRDFAtoms::RegisterAtoms();
 
-  NS_SealStaticAtomTable();
+  NS_SetStaticAtomsDone();
 
   StartupJSEnvironment();
   rv = nsRegion::InitStatic();
   if (NS_FAILED(rv)) {
     NS_ERROR("Could not initialize nsRegion");
     return rv;
   }
 
--- a/toolkit/components/aboutmemory/tests/test_memoryReporters.xul
+++ b/toolkit/components/aboutmemory/tests/test_memoryReporters.xul
@@ -98,17 +98,17 @@
     } else if (aPath.search(/^explicit\/window-objects\/top\(.*\/js-compartment\(/) >= 0) {
       present.windowObjectsJsCompartments = true;
     } else if (aPath.search(/^explicit\/storage\/sqlite\/places.sqlite/) >= 0) {
       present.places = true;
     } else if (aPath.search(/^explicit\/images/) >= 0) {
       present.images = true;
     } else if (aPath.search(/^explicit\/xpti-working-set$/) >= 0) {
       present.xptiWorkingSet = true;
-    } else if (aPath.search(/^explicit\/atom-tables\/main$/) >= 0) {
+    } else if (aPath.search(/^explicit\/atom-table$/) >= 0) {
       present.atomTablesMain = true;
     } else if (/\[System Principal\].*this-is-a-sandbox-name/.test(aPath)) {
       // A system compartment with a location (such as a sandbox) should
       // show that location.
       present.sandboxLocation = true;
     } else if (aPath.includes(bigStringPrefix)) {
       present.bigString = true;
     } else if (aPath.includes("!)(*&")) {
@@ -256,17 +256,17 @@
     checkSizeReasonable("js-main-runtime-gc-heap-committed/used/gc-things",
                         jsGcHeapUsedGcThingsTotal);
 
     ok(present.jsNonWindowCompartments,     "js-non-window compartments are present");
     ok(present.windowObjectsJsCompartments, "window-objects/.../js compartments are present");
     ok(present.places,                      "places is present");
     ok(present.images,                      "images is present");
     ok(present.xptiWorkingSet,              "xpti-working-set is present");
-    ok(present.atomTablesMain,              "atom-tables/main is present");
+    ok(present.atomTablesMain,              "atom-table is present");
     ok(present.sandboxLocation,             "sandbox locations are present");
     ok(present.bigString,                   "large string is present");
     ok(present.smallString1,                "small string 1 is present");
     ok(present.smallString2,                "small string 2 is present");
 
     ok(!present.anonymizedWhenUnnecessary,
        "anonymized paths are not present when unnecessary. Failed case: " +
        present.anonymizedWhenUnnecessary);
--- a/xpcom/base/nsMemoryReporterManager.cpp
+++ b/xpcom/base/nsMemoryReporterManager.cpp
@@ -1390,26 +1390,21 @@ class AtomTablesReporter final : public 
   ~AtomTablesReporter() {}
 
 public:
   NS_DECL_ISUPPORTS
 
   NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
                             nsISupports* aData, bool aAnonymize) override
   {
-    size_t Main, Static;
-    NS_SizeOfAtomTablesIncludingThis(MallocSizeOf, &Main, &Static);
+    int64_t size = NS_SizeOfAtomTableIncludingThis(MallocSizeOf);
 
     MOZ_COLLECT_REPORT(
-      "explicit/atom-tables/main", KIND_HEAP, UNITS_BYTES, Main,
-      "Memory used by the main atoms table.");
-
-    MOZ_COLLECT_REPORT(
-      "explicit/atom-tables/static", KIND_HEAP, UNITS_BYTES, Static,
-      "Memory used by the static atoms table.");
+      "explicit/atom-table", KIND_HEAP, UNITS_BYTES, size,
+      "Memory used by the atom table.");
 
     return NS_OK;
   }
 };
 NS_IMPL_ISUPPORTS(AtomTablesReporter, nsIMemoryReporter)
 
 #ifdef DEBUG
 
--- a/xpcom/ds/nsAtom.h
+++ b/xpcom/ds/nsAtom.h
@@ -160,18 +160,18 @@ already_AddRefed<nsAtom> NS_AtomizeMainT
 // Currently this function is only used in tests, which should probably remain
 // the case.
 nsrefcnt NS_GetNumberOfAtoms();
 
 // Return a pointer for a static atom for the string or null if there's no
 // static atom for this string.
 nsStaticAtom* NS_GetStaticAtom(const nsAString& aUTF16String);
 
-// Seal the static atom table.
-void NS_SealStaticAtomTable();
+// Record that all static atoms have been inserted.
+void NS_SetStaticAtomsDone();
 
 class nsAtomString : public nsString
 {
 public:
   explicit nsAtomString(const nsAtom* aAtom) { aAtom->ToString(*this); }
 };
 
 class nsAtomCString : public nsCString
--- a/xpcom/ds/nsAtomTable.cpp
+++ b/xpcom/ds/nsAtomTable.cpp
@@ -233,16 +233,22 @@ class nsAtomSubTable
 {
   friend class nsAtomTable;
   Mutex mLock;
   PLDHashTable mTable;
   nsAtomSubTable();
   void GCLocked(GCKind aKind);
   size_t SizeOfExcludingThisLocked(MallocSizeOf aMallocSizeOf);
 
+  AtomTableEntry* Search(AtomTableKey& aKey)
+  {
+    mLock.AssertCurrentThreadOwns();
+    return static_cast<AtomTableEntry*>(mTable.Search(&aKey));
+  }
+
   AtomTableEntry* Add(AtomTableKey& aKey)
   {
     mLock.AssertCurrentThreadOwns();
     return static_cast<AtomTableEntry*>(mTable.Add(&aKey)); // Infallible
   }
 };
 
 // The outer atom table, which coordinates access to the inner array of
@@ -251,16 +257,17 @@ class nsAtomTable
 {
 public:
   nsAtomSubTable& SelectSubTable(AtomTableKey& aKey);
   size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf);
   void GC(GCKind aKind);
   already_AddRefed<nsAtom> Atomize(const nsAString& aUTF16String);
   already_AddRefed<nsAtom> Atomize(const nsACString& aUTF8String);
   already_AddRefed<nsAtom> AtomizeMainThread(const nsAString& aUTF16String);
+  nsStaticAtom* GetStaticAtom(const nsAString& aUTF16String);
   void RegisterStaticAtoms(const nsStaticAtomSetup* aSetup, uint32_t aCount);
 
   // The result of this function may be imprecise if other threads are operating
   // on atoms concurrently. It's also slow, since it triggers a GC before
   // counting.
   size_t RacySlowCount();
 
   // This hash table op is a static member of this class so that it can take
@@ -566,59 +573,18 @@ nsAtom::Release()
     }
   }
 
   return count;
 }
 
 //----------------------------------------------------------------------
 
-class StaticAtomEntry : public PLDHashEntryHdr
-{
-public:
-  typedef const nsAString& KeyType;
-  typedef const nsAString* KeyTypePointer;
-
-  explicit StaticAtomEntry(KeyTypePointer aKey) {}
-  StaticAtomEntry(const StaticAtomEntry& aOther) : mAtom(aOther.mAtom) {}
-
-  // We do not delete the atom because that's done when gAtomTable is
-  // destroyed -- which happens immediately after gStaticAtomTable is destroyed
-  // -- in NS_PurgeAtomTable().
-  ~StaticAtomEntry() {}
-
-  bool KeyEquals(KeyTypePointer aKey) const
-  {
-    return mAtom->Equals(*aKey);
-  }
-
-  static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; }
-  static PLDHashNumber HashKey(KeyTypePointer aKey)
-  {
-    return HashString(*aKey);
-  }
-
-  enum { ALLOW_MEMMOVE = true };
-
-  // Static atoms aren't really refcounted. Because these entries live in a
-  // global hashtable, this reference is essentially owning.
-  nsStaticAtom* MOZ_OWNING_REF mAtom;
-};
-
-/**
- * A hashtable of static atoms that existed at app startup. This hashtable
- * helps nsHtml5AtomTable.
- */
-typedef nsTHashtable<StaticAtomEntry> StaticAtomTable;
-static StaticAtomTable* gStaticAtomTable = nullptr;
-
-/**
- * Whether it is still OK to add atoms to gStaticAtomTable.
- */
-static bool gStaticAtomTableSealed = false;
+// Have the static atoms been inserted into the table?
+static bool gStaticAtomsDone = false;
 
 class DefaultAtoms
 {
 public:
   NS_STATIC_ATOM_DECL(empty)
 };
 
 NS_STATIC_ATOM_DEFN(DefaultAtoms, empty)
@@ -644,42 +610,33 @@ NS_InitAtomTable()
   NS_RegisterStaticAtoms(sDefaultAtomSetup);
 }
 
 void
 NS_ShutdownAtomTable()
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(gAtomTable);
-  delete gStaticAtomTable;
-  gStaticAtomTable = nullptr;
 
 #ifdef NS_FREE_PERMANENT_DATA
   // Do a final GC to satisfy leak checking. We skip this step in release
   // builds.
   gAtomTable->GC(GCKind::Shutdown);
 #endif
 
   delete gAtomTable;
   gAtomTable = nullptr;
 }
 
-void
-NS_SizeOfAtomTablesIncludingThis(MallocSizeOf aMallocSizeOf,
-                                 size_t* aMain, size_t* aStatic)
+size_t
+NS_SizeOfAtomTableIncludingThis(MallocSizeOf aMallocSizeOf)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(gAtomTable);
-  *aMain = gAtomTable->SizeOfIncludingThis(aMallocSizeOf);
-
-  // The atoms pointed to by gStaticAtomTable are also pointed to by gAtomTable,
-  // and they're measured by the call above. So no need to measure them here.
-  *aStatic = gStaticAtomTable
-           ? gStaticAtomTable->ShallowSizeOfIncludingThis(aMallocSizeOf)
-           : 0;
+  return gAtomTable->SizeOfIncludingThis(aMallocSizeOf);
 }
 
 size_t
 nsAtomSubTable::SizeOfExcludingThisLocked(MallocSizeOf aMallocSizeOf)
 {
   mLock.AssertCurrentThreadOwns();
   size_t size = mTable.ShallowSizeOfExcludingThis(aMallocSizeOf);
   for (auto iter = mTable.Iter(); !iter.Done(); iter.Next()) {
@@ -689,25 +646,18 @@ nsAtomSubTable::SizeOfExcludingThisLocke
 
   return size;
 }
 
 void
 nsAtomTable::RegisterStaticAtoms(const nsStaticAtomSetup* aSetup,
                                  uint32_t aCount)
 {
-  // Note: gStaticAtomTable is main-thread-only until the table is sealed,
-  // after which it is immutable. So there is no lock protecting it.
   MOZ_ASSERT(NS_IsMainThread());
-  MOZ_RELEASE_ASSERT(!gStaticAtomTableSealed,
-                     "Atom table has already been sealed!");
-
-  if (!gStaticAtomTable) {
-    gStaticAtomTable = new StaticAtomTable();
-  }
+  MOZ_RELEASE_ASSERT(!gStaticAtomsDone, "Static atom insertion is finished!");
 
   for (uint32_t i = 0; i < aCount; ++i) {
     const char16_t* string = aSetup[i].mString;
     nsStaticAtom** atomp = aSetup[i].mAtom;
 
     MOZ_ASSERT(nsCRT::IsAscii(string));
 
     uint32_t stringLen = NS_strlen(string);
@@ -731,23 +681,16 @@ nsAtomTable::RegisterStaticAtoms(const n
           "Static atom registration for %s should be pushed back", name.get());
       }
       atom = static_cast<nsStaticAtom*>(he->mAtom);
     } else {
       atom = new nsStaticAtom(string, stringLen, hash);
       he->mAtom = atom;
     }
     *atomp = atom;
-
-    if (!gStaticAtomTableSealed) {
-      StaticAtomEntry* entry =
-        gStaticAtomTable->PutEntry(nsDependentAtomString(atom));
-      MOZ_ASSERT(atom->IsStaticAtom());
-      entry->mAtom = atom;
-    }
   }
 }
 
 void
 RegisterStaticAtoms(const nsStaticAtomSetup* aSetup, uint32_t aCount)
 {
   MOZ_ASSERT(gAtomTable);
   gAtomTable->RegisterStaticAtoms(aSetup, aCount);
@@ -885,20 +828,32 @@ int32_t
 NS_GetUnusedAtomCount(void)
 {
   return gUnusedAtomCount;
 }
 
 nsStaticAtom*
 NS_GetStaticAtom(const nsAString& aUTF16String)
 {
-  NS_PRECONDITION(gStaticAtomTable, "Static atom table not created yet.");
-  NS_PRECONDITION(gStaticAtomTableSealed, "Static atom table not sealed yet.");
-  StaticAtomEntry* entry = gStaticAtomTable->GetEntry(aUTF16String);
-  return entry ? entry->mAtom : nullptr;
+  MOZ_ASSERT(gStaticAtomsDone, "Static atom setup not yet done.");
+  MOZ_ASSERT(gAtomTable);
+  return gAtomTable->GetStaticAtom(aUTF16String);
+}
+
+nsStaticAtom*
+nsAtomTable::GetStaticAtom(const nsAString& aUTF16String)
+{
+  uint32_t hash;
+  AtomTableKey key(aUTF16String.Data(), aUTF16String.Length(), &hash);
+  nsAtomSubTable& table = SelectSubTable(key);
+  MutexAutoLock lock(table.mLock);
+  AtomTableEntry* he = table.Search(key);
+  return he && he->mAtom->IsStaticAtom()
+       ? static_cast<nsStaticAtom*>(he->mAtom)
+       : nullptr;
 }
 
 void
-NS_SealStaticAtomTable()
+NS_SetStaticAtomsDone()
 {
   MOZ_ASSERT(NS_IsMainThread());
-  gStaticAtomTableSealed = true;
+  gStaticAtomsDone = true;
 }
--- a/xpcom/ds/nsAtomTable.h
+++ b/xpcom/ds/nsAtomTable.h
@@ -8,12 +8,11 @@
 #define nsAtomTable_h__
 
 #include "mozilla/MemoryReporting.h"
 #include <stddef.h>
 
 void NS_InitAtomTable();
 void NS_ShutdownAtomTable();
 
-void NS_SizeOfAtomTablesIncludingThis(mozilla::MallocSizeOf aMallocSizeOf,
-                                      size_t* aMain, size_t* aStatic);
+size_t NS_SizeOfAtomTableIncludingThis(mozilla::MallocSizeOf aMallocSizeOf);
 
 #endif // nsAtomTable_h__