Bug 1397130 - Use signed integer for gUnusedAtomCount. r?froydnj draft
authorXidorn Quan <me@upsuper.org>
Wed, 06 Sep 2017 15:06:16 +1000
changeset 662654 feefd54d596266f1766844d35c8a8b38a64e00df
parent 662117 1d3ceb995429d6e0bf98f67908c149802c6e28ff
child 730937 95ea59fa4460e5bf94d00258c40046ae7476eb84
push id79155
push userxquan@mozilla.com
push dateMon, 11 Sep 2017 23:08:16 +0000
reviewersfroydnj
bugs1397130
milestone57.0a1
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));
 }
 
 }