mmgc-threadsafe
author jorendorff@mozilla.com
Fri, 04 Jan 2008 16:11:56 -0600
changeset 32 5900e6424cbd989bcd7f8d3ff2c4bc2857f98ac1
parent 31 e76f410f999ee1bc9091c760b98aa49841226c3f
child 33 89e259bfffb2247d50f643c9499715378fafaa2f
permissions -rw-r--r--
Preliminarily split mmgc-threadsafe into two parts, mmgc-threadsafe-baseline containing only the changes that are in tamarin-central, and mmgc-threadsafe containing additional actionmonkey-specific stuff on top of that.

diff --git a/MMgc/GC.cpp b/MMgc/GC.cpp
--- a/MMgc/GC.cpp
+++ b/MMgc/GC.cpp
@@ -418,15 +418,6 @@ namespace MMgc
 	void GC::CollectWithBookkeeping(bool callerHoldsLock,
 									bool callerHasActiveRequest)
 	{
-		// invoke precollect callback
-		bool vetoed = false;
-		GCCallback *cb = m_callbacks;
-		while (cb) {
-			if (!cb->precollect())
-				vetoed = true;
-			cb = cb->nextCB;
-		}
-
 #ifdef MMGC_THREADSAFE
 		GCAssert(callerHoldsLock == m_lock.IsHeld());
 #ifdef _DEBUG
@@ -467,6 +458,14 @@ namespace MMgc
 			//
 			if (m_exclusiveGCThread != thisThread) {
 				GCRoot *stackRoot;
+				void *stack;
+				size_t stackSize;
+
+				// Call this macro here, not under "if
+				// (callerHasActiveRequest)", because it introduces local
+				// variables that must survive for the whole lifetime of
+				// stackRoot.
+				MMGC_GET_STACK_EXTENTS(this, stack, stackSize);
 
 				// Before waiting, this thread must suspend any active
 				// requests.  This avoids a deadlock where m_exclusiveGCThread
@@ -478,10 +477,6 @@ namespace MMgc
 				// in a suspended request.
 				//
 				if (callerHasActiveRequest) {
-					void *stack;
-					size_t stackSize;
-
-					MMGC_GET_STACK_EXENTS(this, stack, stackSize);
 					stackRoot = new GCRoot(this, stack, stackSize);
 
 					OnLeaveRequestAlreadyLocked();
@@ -568,6 +563,12 @@ namespace MMgc
 		if (!callerHoldsLock)
 			m_lock.Release();
 #endif
+
+		{
+			USING_CALLBACK_LIST(this);
+			for (GCCallback *cb = m_callbacks; cb; cb = cb->nextCB)
+				cb->postcollect();
+		}
 	}
 
 	void GC::CollectImpl()
@@ -618,8 +619,11 @@ namespace MMgc
 		SAMPLE_CHECK();
 
 		// invoke lastmark on all callbacks
-		for (GCCallback *cb = m_callbacks; cb; cb = cb->nextCB)
-			cb->lastmark(work);
+		{
+			USING_CALLBACK_LIST(this);
+			for (GCCallback *cb = m_callbacks; cb; cb = cb->nextCB)
+				cb->lastmark(work);
+		}
 
 		if(stackStart == NULL) {
 			MarkQueueAndStack(work);
@@ -646,6 +650,9 @@ namespace MMgc
 
 	void *GC::AllocAlreadyLocked(size_t size, int flags/*0*/, int skip/*3*/)
 	{
+#ifdef MMGC_THREADSAFE
+		GCAssert(GCThread::GetCurrentThread()->IsInActiveRequest());
+#endif
 #ifdef DEBUGGER
 		avmplus::AvmCore *core = (avmplus::AvmCore*)GetGCContextVariable(GCV_AVMCORE);
 		if(core)
@@ -2730,8 +2737,11 @@ bail:
 		}
 
 		// invoke lastmark on all callbacks
-		for (GCCallback *cb = m_callbacks; cb; cb = cb->nextCB)
-			cb->lastmark(m_incrementalWork);
+		{
+			USING_CALLBACK_LIST(this);
+			for (GCCallback *cb = m_callbacks; cb; cb = cb->nextCB)
+				cb->lastmark(m_incrementalWork);
+		}
 
 		MarkQueueAndStack(m_incrementalWork);
 
@@ -2978,7 +2988,8 @@ bail:
 
 	void GC::AddEdgeCallback(GCEdgeCallback *cb)
 	{
-		CheckThread();
+		USING_CALLBACK_LIST(this);
+
 		cb->prevCB = NULL;
 		cb->nextCB = m_edgeCallbacks;
 		if(m_edgeCallbacks)
@@ -2988,7 +2999,8 @@ bail:
 
 	void GC::RemoveEdgeCallback(GCEdgeCallback *cb)
 	{
-		CheckThread();
+		USING_CALLBACK_LIST(this);
+
 		if( m_edgeCallbacks == cb )
 			m_edgeCallbacks = cb->nextCB;
 		else
@@ -3000,12 +3012,12 @@ bail:
 
 	void GC::FireFoundEdgeTo(const void *p)
 	{
+		// Don't acquire the spinlock here because (a) that would really hurt
+		// performance; (b) the m_edgeCallbacks list, unlike the m_callbacks
+		// list, is protected by the request model.
 		p = GetUserPointer(p);
-		GCEdgeCallback *cb = m_edgeCallbacks;
-		while(cb) {
+		for (GCEdgeCallback *cb = m_edgeCallbacks; cb; cb = cb->nextCB)
 			cb->foundEdgeTo(p);
-			cb = cb->nextCB;
-		}
 	}
 
 	void GC::PushWorkItem(GCStack<GCWorkItem> &stack, GCWorkItem item)
diff --git a/MMgc/GC.h b/MMgc/GC.h
--- a/MMgc/GC.h
+++ b/MMgc/GC.h
@@ -258,16 +258,25 @@ namespace MMgc
 		void Destroy();
 
 		/**
-		 * This method is called from GC::Collect(), before
-		 * anything else, even in cases where it's logically
-		 * certain that collection <em>will not start</em>
-		 * (e.g. because we're already collecting, or GC is
+		 * GC::Collect() fires this callback before anything else, even in
+		 * cases where it's logically certain that collection <em>will not
+		 * start</em> (e.g. because we're already collecting, or GC is
 		 * disabled).
+		 *
+		 * In an MMGC_THREADSAFE build, it is unspecified whether the calling
+		 * thread holds the gc-wide lock when this callback is called.
+		 * Multiple threads may fire this callback concurrently.
 		 *
 		 * Returns true to allow collection to happen, false
 		 * to veto.
 		 */
 		virtual bool precollect() { return true; }
+
+		/**
+		 * GC::Collect() fires this callback after everything else,
+		 * only if collection actually happened in this thread.
+		 */
+		virtual void postcollect() {}
 
 		/**
 		 * This method is invoked during the last mark phase
@@ -279,6 +288,8 @@ namespace MMgc
 		 * provided by the GCCallback base class.  The
 		 * corresponding methods of <code>class GC</code> are
 		 * private.)
+		 *
+		 * @access Requires(gc->exclusiveGC)
 		 */
 		virtual void lastmark(GCStack<GCWorkItem> &) {}
 
@@ -397,6 +408,17 @@ namespace MMgc
 	{
 		friend class GC;
 	public:
+		/**
+		 * Create an edge callback.
+		 *
+		 * If `gc` is non-null, this constructor adds this GCEdgeCallback to a
+		 * list of callbacks managed by `gc`.  This means the new object
+		 * immediately starts receiving calls, even if it isn't fully
+		 * constructed yet, potentially resulting in "Pure virtual method
+		 * called" or some other crash.  To avoid this race condition, call
+		 * this constructor with a non-null `gc` only from a thread that is in
+		 * a request on `gc`.
+		 */
 		GCEdgeCallback(GC *gc = NULL);
 		virtual ~GCEdgeCallback();
 
@@ -1446,6 +1468,24 @@ namespace MMgc
 		void AddCallback(GCCallback *cb);
 		void RemoveCallback(GCCallback *cb);
 
+		/**
+		 * Points to the head of a linked list of edge callback objects.
+		 *
+		 * @access Requires((request && m_callbackListLock) || exclusiveGC)
+		 *
+		 * In an MMGC_THREADSAFE build, this linked list is protected by the
+		 * request model.  A thread must only access the list (a) from
+		 * application code that is within a request AND holds
+		 * m_callbackListLock; or (b) from MMgc code in the
+		 * m_exclusiveGCThread.
+		 *
+		 * This policy is different from the one that covers m_callbacks for
+		 * two reasons.  First, m_callbacks can fire the precollect callback
+		 * even if the calling thread is not in a request at all, so this
+		 * policy would be insufficient for m_callbacks.  Second,
+		 * m_edgeCallbacks fires very frequently during marking, so a
+		 * lock-free policy is probably much faster.
+		 */
 		GCEdgeCallback *m_edgeCallbacks;
 		void AddEdgeCallback(GCEdgeCallback *cb);
 		void RemoveEdgeCallback(GCEdgeCallback *cb);
@@ -1453,9 +1493,10 @@ namespace MMgc
 		/**
 		 * Notify GCEdgeCallbacks of an edge.
 		 *
-		 * p is a "real pointer".  This method converts it to
-		 * a "user pointer" using GetUserPointer() before
-		 * calling callbacks.
+		 * p is a "real pointer".  This method converts it to a "user pointer"
+		 * using GetUserPointer() before calling callbacks.
+		 *
+		 * @access Requires(exclusiveGC)
 		 */
 		void FireFoundEdgeTo(const void *p);