Bug 1397130 - Use signed integer for gUnusedAtomCount. r=froydnj
authorXidorn Quan <me@upsuper.org>
Wed, 06 Sep 2017 15:06:16 +1000
changeset 429657 7dd4be007d1389ad90285d1a6d606d24973e82f8
parent 429656 de5d70dd47bbdd7668b7801f35e726f502b4d7a0
child 429658 7bf510328045e8ea7b39f955ba2cb606b1f71c00
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1397130
milestone57.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 1397130 - Use signed integer for gUnusedAtomCount. r=froydnj MozReview-Commit-ID: 9KweZdyu5WF
xpcom/ds/nsAtomTable.cpp
xpcom/tests/gtest/TestAtoms.cpp
--- a/xpcom/ds/nsAtomTable.cpp
+++ b/xpcom/ds/nsAtomTable.cpp
@@ -62,17 +62,24 @@ class CheckStaticAtomSizes
                   (offsetof(nsFakeStringBuffer<1>, mStringData) ==
                    sizeof(nsStringBuffer)),
                   "mocked-up strings' representations should be compatible");
   }
 };
 
 //----------------------------------------------------------------------
 
-static Atomic<uint32_t, ReleaseAcquire> gUnusedAtomCount(0);
+// gUnusedAtomCount is incremented when an atom loses its last reference
+// (and thus turned into unused state), and decremented when an unused
+// atom gets a reference again. The atom table relies on this value to
+// schedule GC. This value can temporarily go below zero when multiple
+// threads are operating the same atom, so it has to be signed so that
+// we wouldn't use overflow value for comparison.
+// See Atom::DynamicAddRef and Atom::DynamicRelease.
+static Atomic<int32_t, ReleaseAcquire> gUnusedAtomCount(0);
 
 #if defined(NS_BUILD_REFCNT_LOGGING)
 // nsFakeStringBuffers don't really use the refcounting system, but we
 // have to give a coherent series of addrefs and releases to the
 // refcount logging system, or we'll hit assertions when running with
 // XPCOM_MEM_LOG_CLASSES=nsStringBuffer.
 class FakeBufferRefcountHelper
 {
@@ -401,17 +408,17 @@ Atom::GCAtomTable()
 void
 Atom::GCAtomTableLocked(const MutexAutoLock& aProofOfLock, GCKind aKind)
 {
   MOZ_ASSERT(NS_IsMainThread());
   for (uint32_t i = 0; i < RECENTLY_USED_MAIN_THREAD_ATOM_CACHE_SIZE; ++i) {
     sRecentlyUsedMainThreadAtoms[i] = nullptr;
   }
 
-  uint32_t removedCount = 0; // Use a non-atomic temporary for cheaper increments.
+  int32_t removedCount = 0; // Use a non-atomic temporary for cheaper increments.
   nsAutoCString nonZeroRefcountAtoms;
   uint32_t nonZeroRefcountAtomsCount = 0;
   for (auto i = gAtomTable->Iter(); !i.Done(); i.Next()) {
     auto entry = static_cast<AtomTableEntry*>(i.Get());
     if (entry->mAtom->IsStaticAtom()) {
       continue;
     }
 
@@ -475,19 +482,19 @@ Atom::DynamicAddRef()
     gUnusedAtomCount--;
   }
   return count;
 }
 
 #ifdef DEBUG
 // We set a lower GC threshold for atoms in debug builds so that we exercise
 // the GC machinery more often.
-static const uint32_t kAtomGCThreshold = 20;
+static const int32_t kAtomGCThreshold = 20;
 #else
-static const uint32_t kAtomGCThreshold = 10000;
+static const int32_t kAtomGCThreshold = 10000;
 #endif
 
 MozExternalRefCountType
 Atom::DynamicRelease()
 {
   MOZ_ASSERT(IsDynamicAtom());
   MOZ_ASSERT(mRefCnt > 0);
   nsrefcnt count = --mRefCnt;
@@ -791,17 +798,17 @@ NS_AtomizeMainThread(const nsAString& aU
 nsrefcnt
 NS_GetNumberOfAtoms(void)
 {
   Atom::GCAtomTable(); // Trigger a GC so that we return a deterministic result.
   MutexAutoLock lock(*gAtomTableLock);
   return gAtomTable->EntryCount();
 }
 
-uint32_t
+int32_t
 NS_GetUnusedAtomCount(void)
 {
   return gUnusedAtomCount;
 }
 
 nsIAtom*
 NS_GetStaticAtom(const nsAString& aUTF16String)
 {
--- a/xpcom/tests/gtest/TestAtoms.cpp
+++ b/xpcom/tests/gtest/TestAtoms.cpp
@@ -12,17 +12,17 @@
 #include "nsIServiceManager.h"
 #include "nsStaticAtom.h"
 #include "nsThreadUtils.h"
 
 #include "gtest/gtest.h"
 
 using namespace mozilla;
 
-uint32_t NS_GetUnusedAtomCount(void);
+int32_t NS_GetUnusedAtomCount(void);
 
 namespace TestAtoms {
 
 TEST(Atoms, Basic)
 {
   for (unsigned int i = 0; i < ArrayLength(ValidStrings); ++i) {
     nsDependentString str16(ValidStrings[i].m16);
     nsDependentCString str8(ValidStrings[i].m8);
@@ -172,22 +172,22 @@ private:
 
 NS_IMPL_ISUPPORTS(nsAtomRunner, nsIRunnable)
 
 TEST(Atoms, ConcurrentAccessing)
 {
   static const size_t kThreadCount = 4;
   // Force a GC before so that we don't have any unused atom.
   NS_GetNumberOfAtoms();
-  EXPECT_EQ(NS_GetUnusedAtomCount(), uint32_t(0));
+  EXPECT_EQ(NS_GetUnusedAtomCount(), int32_t(0));
   nsCOMPtr<nsIThread> threads[kThreadCount];
   for (size_t i = 0; i < kThreadCount; i++) {
     nsresult rv = NS_NewThread(getter_AddRefs(threads[i]), new nsAtomRunner);
     EXPECT_TRUE(NS_SUCCEEDED(rv));
   }
   for (size_t i = 0; i < kThreadCount; i++) {
     threads[i]->Shutdown();
   }
   // We should have one unused atom from this test.
-  EXPECT_EQ(NS_GetUnusedAtomCount(), uint32_t(1));
+  EXPECT_EQ(NS_GetUnusedAtomCount(), int32_t(1));
 }
 
 }