From 2c597c5a3d87395d69cb54183e6fe3816b9698f3 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 26 May 2016 17:55:53 -0700 Subject: [PATCH] Bug 1275755 - Use a GC scheme to free unused atoms. r=froydnj --- xpcom/ds/nsAtomTable.cpp | 111 ++++++++++++++++++++++++++++++++------- 1 file changed, 93 insertions(+), 18 deletions(-) diff --git a/xpcom/ds/nsAtomTable.cpp b/xpcom/ds/nsAtomTable.cpp index 286da8b06d83..ecb5192a0854 100644 --- a/xpcom/ds/nsAtomTable.cpp +++ b/xpcom/ds/nsAtomTable.cpp @@ -27,9 +27,11 @@ // There are two kinds of atoms handled by this module. // // - DynamicAtom: the atom itself is heap allocated, as is the nsStringBuffer it -// points to. |gAtomTable| holds weak references to them DynamicAtoms, and -// when they are destroyed (due to their refcount reaching zero) they remove -// themselves from |gAtomTable|. +// points to. |gAtomTable| holds weak references to them DynamicAtoms. When +// the refcount of a DynamicAtom drops to zero, we increment a static counter. +// When that counter reaches a certain threshold, we iterate over the atom +// table, removing and deleting DynamicAtoms with refcount zero. This allows +// us to avoid acquiring the atom table lock during normal refcounting. // // - StaticAtom: the atom itself is heap allocated, but it points to a static // nsStringBuffer. |gAtomTable| effectively owns StaticAtoms, because such @@ -63,10 +65,22 @@ class CheckStaticAtomSizes //---------------------------------------------------------------------- +static Atomic gUnusedAtomCount(0); + class DynamicAtom final : public nsIAtom { public: + static already_AddRefed Create(const nsAString& aString, uint32_t aHash) + { + // The refcount is appropriately initialized in the constructor. + return dont_AddRef(new DynamicAtom(aString, aHash)); + } + + static void GCAtomTable(); + +private: DynamicAtom(const nsAString& aString, uint32_t aHash) + : mRefCnt(1) { mLength = aString.Length(); mIsStatic = false; @@ -100,19 +114,16 @@ public: private: // We don't need a virtual destructor because we always delete via a - // DynamicAtom* pointer (in Release(), defined via NS_IMPL_ISUPPORTS), not an - // nsIAtom* pointer. + // DynamicAtom* pointer (in GCAtomTable()), not an nsIAtom* pointer. ~DynamicAtom(); public: - NS_DECL_ISUPPORTS + NS_DECL_THREADSAFE_ISUPPORTS NS_DECL_NSIATOM void TransmuteToStatic(nsStringBuffer* aStringBuffer); }; -NS_IMPL_ISUPPORTS(DynamicAtom, nsIAtom) - class StaticAtom final : public nsIAtom { // This is the function that calls the private constructor. @@ -387,16 +398,72 @@ static const PLDHashTableOps AtomTableOps = { //---------------------------------------------------------------------- +void +DynamicAtom::GCAtomTable() +{ + MutexAutoLock lock(*gAtomTableLock); + uint32_t removedCount = 0; // Use a non-atomic temporary for cheaper increments. + for (auto i = gAtomTable->Iter(); !i.Done(); i.Next()) { + auto entry = static_cast(i.Get()); + if (!entry->mAtom->IsStaticAtom()) { + auto atom = static_cast(entry->mAtom); + if (atom->mRefCnt == 0) { + i.Remove(); + delete atom; + ++removedCount; + } + } + } + + // During the course of this function, the atom table is locked. This means + // that, barring refcounting bugs in consumers, an atom can never go from + // refcount == 0 to refcount != 0 during a GC. However, an atom _can_ go from + // refcount != 0 to refcount == 0 if a Release() occurs in parallel with GC. + // This means that we cannot assert that gUnusedAtomCount == removedCount, and + // thus that there are no unused atoms at the end of a GC. We can and do, + // however, assert this after the last GC at shutdown. + MOZ_ASSERT(removedCount <= gUnusedAtomCount); + gUnusedAtomCount -= removedCount; + +} + +NS_IMPL_QUERY_INTERFACE(DynamicAtom, nsIAtom) + +NS_IMETHODIMP_(MozExternalRefCountType) +DynamicAtom::AddRef(void) +{ + nsrefcnt count = ++mRefCnt; + if (count == 1) { + MOZ_ASSERT(gUnusedAtomCount > 0); + 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; +#else +static const uint32_t kAtomGCThreshold = 10000; +#endif + +NS_IMETHODIMP_(MozExternalRefCountType) +DynamicAtom::Release(void) +{ + MOZ_ASSERT(mRefCnt > 0); + nsrefcnt count = --mRefCnt; + if (count == 0) { + if (++gUnusedAtomCount >= kAtomGCThreshold) { + GCAtomTable(); + } + } + + return count; +} + DynamicAtom::~DynamicAtom() { - MOZ_ASSERT(gAtomTable, "uninitialized atom hashtable"); - MutexAutoLock lock(*gAtomTableLock); - - // DynamicAtoms must be removed from gAtomTable when their refcount reaches - // zero and they are released. - AtomTableKey key(mString, mLength, mHash); - gAtomTable->Remove(&key); - nsStringBuffer::FromData(mString)->Release(); } @@ -474,6 +541,13 @@ NS_ShutdownAtomTable() delete gStaticAtomTable; gStaticAtomTable = nullptr; +#ifdef NS_FREE_PERMANENT_DATA + // Do a final GC to satisfy leak checking. We skip this step in release + // builds. + DynamicAtom::GCAtomTable(); + MOZ_ASSERT(gUnusedAtomCount == 0); +#endif + // XXXbholley: it would be good to assert gAtomTable->EntryCount() == 0 // here, but that currently fails. Probably just a few things that need // to be fixed up. @@ -589,7 +663,7 @@ NS_Atomize(const nsACString& aUTF8String) // Actually, now there is, sort of: ForgetSharedBuffer. nsString str; CopyUTF8toUTF16(aUTF8String, str); - RefPtr atom = new DynamicAtom(str, hash); + RefPtr atom = DynamicAtom::Create(str, hash); he->mAtom = atom; @@ -617,7 +691,7 @@ NS_Atomize(const nsAString& aUTF16String) return atom.forget(); } - RefPtr atom = new DynamicAtom(aUTF16String, hash); + RefPtr atom = DynamicAtom::Create(aUTF16String, hash); he->mAtom = atom; return atom.forget(); @@ -626,6 +700,7 @@ NS_Atomize(const nsAString& aUTF16String) nsrefcnt NS_GetNumberOfAtoms(void) { + DynamicAtom::GCAtomTable(); // Trigger a GC so that we return a deterministic result. MutexAutoLock lock(*gAtomTableLock); return gAtomTable->EntryCount(); }