From 450cf95e35e7c6b1a26f9a5da363cbd8d1b40f11 Mon Sep 17 00:00:00 2001 From: Igor Bukanov Date: Sun, 24 Jan 2010 14:15:38 +0300 Subject: [PATCH] bug 538275 - ClaimTitle cleanup. r=brendan, jorendorff --- js/src/jscntxt.cpp | 38 --------------- js/src/jscntxt.h | 29 ++++------- js/src/jsgc.cpp | 71 +++++++++++++++++++++++---- js/src/jslock.cpp | 119 ++++++++++++++------------------------------- js/src/jsobj.cpp | 8 +-- js/src/jsscope.h | 36 ++++++++++---- 6 files changed, 137 insertions(+), 164 deletions(-) diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp index be7b8d76e6d..7dbdc25c6ad 100644 --- a/js/src/jscntxt.cpp +++ b/js/src/jscntxt.cpp @@ -910,44 +910,6 @@ js_WaitForGC(JSRuntime *rt) } } -uint32 -js_DiscountRequestsForGC(JSContext *cx) -{ - uint32 requestDebit; - - JS_ASSERT(cx->thread); - JS_ASSERT(cx->runtime->gcThread != cx->thread); - -#ifdef JS_TRACER - if (JS_ON_TRACE(cx)) { - JS_UNLOCK_GC(cx->runtime); - LeaveTrace(cx); - JS_LOCK_GC(cx->runtime); - } -#endif - - requestDebit = js_CountThreadRequests(cx); - if (requestDebit != 0) { - JSRuntime *rt = cx->runtime; - JS_ASSERT(requestDebit <= rt->requestCount); - rt->requestCount -= requestDebit; - if (rt->requestCount == 0) - JS_NOTIFY_REQUEST_DONE(rt); - } - return requestDebit; -} - -void -js_RecountRequestsAfterGC(JSRuntime *rt, uint32 requestDebit) -{ - while (rt->gcLevel > 0) { - JS_ASSERT(rt->gcThread); - JS_AWAIT_GC_DONE(rt); - } - if (requestDebit != 0) - rt->requestCount += requestDebit; -} - #endif static JSDHashNumber diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h index 81b6147c532..bd48578f692 100644 --- a/js/src/jscntxt.h +++ b/js/src/jscntxt.h @@ -463,6 +463,15 @@ struct JSThread { */ ptrdiff_t gcThreadMallocBytes; + /* + * This thread is inside js_GC, either waiting until it can start GC, or + * waiting for GC to finish on another thread. This thread holds no locks; + * other threads may steal titles from it. + * + * Protected by rt->gcLock. + */ + bool gcWaiting; + /* * Deallocator task for this thread. */ @@ -1715,7 +1724,7 @@ js_NextActiveContext(JSRuntime *, JSContext *); /* * Count the number of contexts entered requests on the current thread. */ -uint32 +extern uint32 js_CountThreadRequests(JSContext *cx); /* @@ -1727,24 +1736,6 @@ js_CountThreadRequests(JSContext *cx); extern void js_WaitForGC(JSRuntime *rt); -/* - * If we're in one or more requests (possibly on more than one context) - * running on the current thread, indicate, temporarily, that all these - * requests are inactive so a possible GC can proceed on another thread. - * This function returns the number of discounted requests. The number must - * be passed later to js_ActivateRequestAfterGC to reactivate the requests. - * - * This function must be called with the GC lock held. - */ -uint32 -js_DiscountRequestsForGC(JSContext *cx); - -/* - * This function must be called with the GC lock held. - */ -void -js_RecountRequestsAfterGC(JSRuntime *rt, uint32 requestDebit); - #else /* !JS_THREADSAFE */ # define js_WaitForGC(rt) ((void) 0) diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp index 4ae39bb870d..655a64c5b33 100644 --- a/js/src/jsgc.cpp +++ b/js/src/jsgc.cpp @@ -2863,7 +2863,7 @@ js_GC(JSContext *cx, JSGCInvocationKind gckind) JSTracer trc; JSGCArena *emptyArenas, *a, **ap; #ifdef JS_THREADSAFE - uint32 requestDebit; + size_t requestDebit; #endif #ifdef JS_GCMETER uint32 nlivearenas, nkilledarenas, nthings; @@ -2951,12 +2951,51 @@ js_GC(JSContext *cx, JSGCInvocationKind gckind) METER_UPDATE_MAX(rt->gcStats.maxlevel, rt->gcLevel); /* - * If the GC runs on another thread, temporarily suspend the current - * request and wait until the GC is done. + * If the GC runs on another thread, temporarily suspend all requests + * running on the current thread and wait until the GC is done. */ if (rt->gcThread != cx->thread) { - requestDebit = js_DiscountRequestsForGC(cx); - js_RecountRequestsAfterGC(rt, requestDebit); + requestDebit = js_CountThreadRequests(cx); + JS_ASSERT(requestDebit <= rt->requestCount); +#ifdef JS_TRACER + JS_ASSERT_IF(requestDebit == 0, !JS_ON_TRACE(cx)); +#endif + if (requestDebit != 0) { +#ifdef JS_TRACER + if (JS_ON_TRACE(cx)) { + /* + * Leave trace before we decrease rt->requestCount and + * notify the GC. Otherwise the GC may start immediately + * after we unlock while this thread is still on trace. + */ + JS_UNLOCK_GC(rt); + LeaveTrace(cx); + JS_LOCK_GC(rt); + } +#endif + rt->requestCount -= requestDebit; + + if (rt->requestCount == 0) + JS_NOTIFY_REQUEST_DONE(rt); + + /* + * See comments before another call to js_ShareWaitingTitles + * below. + */ + cx->thread->gcWaiting = true; + js_ShareWaitingTitles(cx); + + /* + * Check that we did not release the GC lock above and let the + * GC to finish before we wait. + */ + JS_ASSERT(rt->gcLevel > 0); + do { + JS_AWAIT_GC_DONE(rt); + } while (rt->gcLevel > 0); + cx->thread->gcWaiting = false; + rt->requestCount += requestDebit; + } } if (!(gckind & GC_LOCK_HELD)) JS_UNLOCK_GC(rt); @@ -2983,10 +3022,24 @@ js_GC(JSContext *cx, JSGCInvocationKind gckind) */ requestDebit = js_CountThreadRequests(cx); JS_ASSERT_IF(cx->requestDepth != 0, requestDebit >= 1); - rt->requestCount -= requestDebit; - while (rt->requestCount > 0) - JS_AWAIT_REQUEST_DONE(rt); - rt->requestCount += requestDebit; + JS_ASSERT(requestDebit <= rt->requestCount); + if (requestDebit != rt->requestCount) { + rt->requestCount -= requestDebit; + + /* + * Share any title that is owned by the GC thread before we wait, to + * avoid a deadlock with ClaimTitle. We also set the gcWaiting flag so + * that ClaimTitle can claim the title ownership from the GC thread if + * that function is called while the GC is waiting. + */ + cx->thread->gcWaiting = true; + js_ShareWaitingTitles(cx); + do { + JS_AWAIT_REQUEST_DONE(rt); + } while (rt->requestCount > 0); + cx->thread->gcWaiting = false; + rt->requestCount += requestDebit; + } #else /* !JS_THREADSAFE */ diff --git a/js/src/jslock.cpp b/js/src/jslock.cpp index ff216adad3e..ab323d590d7 100644 --- a/js/src/jslock.cpp +++ b/js/src/jslock.cpp @@ -520,16 +520,18 @@ js_NudgeOtherContexts(JSContext *cx) * specific thread. */ static void -NudgeThread(JSThread *thread) +NudgeThread(JSRuntime *rt, JSThread *thread) { - JSCList *link; - JSContext *acx; + JS_ASSERT(thread); - link = &thread->contextList; - while ((link = link->next) != &thread->contextList) { - acx = CX_FROM_THREAD_LINKS(link); - JS_ASSERT(acx->thread == thread); - if (acx->requestDepth) + /* + * We cannot walk here over thread->contextList as that is manipulated + * outside the GC lock and must be accessed only from the the thread that + * owns JSThread. + */ + JSContext *acx = NULL; + while ((acx = js_NextActiveContext(rt, acx)) != NULL) { + if (acx->thread == thread) JS_TriggerOperationCallback(acx); } } @@ -545,48 +547,45 @@ NudgeThread(JSThread *thread) static JSBool ClaimTitle(JSTitle *title, JSContext *cx) { - JSRuntime *rt; - JSContext *ownercx; - uint32 requestDebit; + JSRuntime *rt = cx->runtime; + JS_ASSERT_IF(cx->requestDepth == 0, + cx->thread == rt->gcThread && rt->gcRunning); - rt = cx->runtime; JS_RUNTIME_METER(rt, claimAttempts); JS_LOCK_GC(rt); /* Reload in case ownercx went away while we blocked on the lock. */ - while ((ownercx = title->ownercx) != NULL) { + while (JSContext *ownercx = title->ownercx) { /* * Avoid selflock if ownercx is dead, or is not running a request, or - * has the same thread as cx. Set title->ownercx to cx so that the - * matching JS_UNLOCK_SCOPE or JS_UNLOCK_OBJ macro call will take the - * fast path around the corresponding js_UnlockTitle or js_UnlockObj - * function call. + * has the same thread as cx, or cx->thread runs the GC (in which case + * all other requests must be suspended), or ownercx->thread runs a GC + * and the GC waits for all requests to finish. Set title->ownercx to + * cx so that the matching JS_UNLOCK_SCOPE or JS_UNLOCK_OBJ macro call + * will take the fast path around the corresponding js_UnlockTitle or + * js_UnlockObj function call. * * If title->u.link is non-null, title has already been inserted on * the rt->titleSharingTodo list, because another thread's context * already wanted to lock title while ownercx was running a request. - * That context must still be in request and cannot be dead. We can - * claim it if its thread matches ours but only if cx itself is in a - * request. - * - * The latter check covers the case when the embedding triggers a call - * to js_GC on a cx outside a request while having ownercx running a - * request on the same thread, and then js_GC calls a mark hook or a - * finalizer accessing the title. In this case we cannot claim the - * title but must share it now as no title-sharing JS_EndRequest will - * follow. + * That context must still be in request and cannot be dead. Moreover, + * the GC can not run at this moment as it must wait until all the + * titles are shared and the threads that want to lock them finish + * their requests. Thus we can claim the title if its thread matches + * ours. */ bool canClaim; if (title->u.link) { JS_ASSERT(js_ValidContextPointer(rt, ownercx)); JS_ASSERT(ownercx->requestDepth > 0); - JS_ASSERT_IF(cx->requestDepth == 0, cx->thread == rt->gcThread); - canClaim = (ownercx->thread == cx->thread && - cx->requestDepth > 0); + JS_ASSERT(!rt->gcRunning); + canClaim = (ownercx->thread == cx->thread); } else { canClaim = (!js_ValidContextPointer(rt, ownercx) || !ownercx->requestDepth || - ownercx->thread == cx->thread); + cx->thread == ownercx->thread || + cx->thread == rt->gcThread || + ownercx->thread->gcWaiting); } if (canClaim) { title->ownercx = cx; @@ -607,14 +606,8 @@ ClaimTitle(JSTitle *title, JSContext *cx) * so that control would unwind properly once these locks became * "thin" or "fat". The engine promotes a title from exclusive to * shared access only when locking, never when holding or unlocking. - * - * Avoid deadlock before any of this title/context cycle detection if - * cx is on the active GC's thread, because in that case, no requests - * will run until the GC completes. Any title wanted by the GC (from - * a finalizer or a mark hook) that can't be claimed must become - * shared. */ - if (rt->gcThread == cx->thread || WillDeadlock(ownercx, cx->thread)) { + if (WillDeadlock(ownercx, cx->thread)) { ShareTitle(cx, title); break; } @@ -625,26 +618,10 @@ ClaimTitle(JSTitle *title, JSContext *cx) * non-null test, and avoid double-insertion bugs. */ if (!title->u.link) { - TITLE_TO_SCOPE(title)->hold(); title->u.link = rt->titleSharingTodo; rt->titleSharingTodo = title; } - /* - * Discount all the requests running on the current thread so a - * possible GC can proceed on another thread while we wait on - * rt->titleSharingDone. - */ - requestDebit = js_DiscountRequestsForGC(cx); - if (title->ownercx != ownercx) { - /* - * js_DiscountRequestsForGC released and reacquired the GC lock, - * and the title was taken or shared. Start over. - */ - js_RecountRequestsAfterGC(rt, requestDebit); - continue; - } - /* * We know that some other thread's context owns title, which is now * linked onto rt->titleSharingTodo, awaiting the end of that other @@ -652,7 +629,7 @@ ClaimTitle(JSTitle *title, JSContext *cx) * But before waiting, we force the operation callback for that other * thread so it can quickly suspend. */ - NudgeThread(ownercx->thread); + NudgeThread(rt, ownercx->thread); JS_ASSERT(!cx->thread->titleToShare); cx->thread->titleToShare = title; @@ -661,21 +638,6 @@ ClaimTitle(JSTitle *title, JSContext *cx) #endif PR_WaitCondVar(rt->titleSharingDone, PR_INTERVAL_NO_TIMEOUT); JS_ASSERT(stat != PR_FAILURE); - - js_RecountRequestsAfterGC(rt, requestDebit); - - /* - * Don't clear titleToShare until after we're through waiting on - * all condition variables protected by rt->gcLock -- that includes - * rt->titleSharingDone *and* rt->gcDone (hidden in the call to - * js_RecountRequestsAfterGC immediately above). - * - * Otherwise, the GC could easily deadlock with another thread that - * owns a title wanted by a finalizer. By keeping cx->titleToShare - * set till here, we ensure that such deadlocks are detected, which - * results in the finalized object's title being shared (it must, of - * course, have other, live objects sharing it). - */ cx->thread->titleToShare = NULL; } @@ -693,24 +655,15 @@ js_ShareWaitingTitles(JSContext *cx) todop = &cx->runtime->titleSharingTodo; shared = false; while ((title = *todop) != NO_TITLE_SHARING_TODO) { - if (title->ownercx != cx) { + if (title->ownercx->thread != cx->thread) { todop = &title->u.link; continue; } *todop = title->u.link; - title->u.link = NULL; /* null u.link for sanity ASAP */ + title->u.link = NULL; /* null u.link for sanity ASAP */ - /* - * If JSScope::drop returns false, we held the last ref to scope. The - * waiting thread(s) must have been killed, after which the GC - * collected the object that held this scope. Unlikely, because it - * requires that the GC ran (e.g., from an operation callback) - * during this request, but possible. - */ - if (TITLE_TO_SCOPE(title)->drop(cx, NULL)) { - FinishSharingTitle(cx, title); /* set ownercx = NULL */ - shared = true; - } + FinishSharingTitle(cx, title); /* set ownercx = NULL */ + shared = true; } if (shared) JS_NOTIFY_ALL_CONDVAR(cx->runtime->titleSharingDone); diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp index ab9be3498ce..07e6a367429 100644 --- a/js/src/jsobj.cpp +++ b/js/src/jsobj.cpp @@ -3860,12 +3860,8 @@ js_InitClass(JSContext *cx, JSObject *obj, JSObject *parent_proto, * * All callers of JSObject::initSharingEmptyScope depend on this. */ - { - JSScope *scope = OBJ_SCOPE(proto)->getEmptyScope(cx, clasp); - if (!scope) - goto bad; - scope->drop(cx, NULL); - } + if (!OBJ_SCOPE(proto)->ensureEmptyScope(cx, clasp)) + goto bad; /* If this is a standard class, cache its prototype. */ if (key != JSProto_Null && !js_SetClassObject(cx, obj, key, ctor)) diff --git a/js/src/jsscope.h b/js/src/jsscope.h index 436dd434650..b2b93601057 100644 --- a/js/src/jsscope.h +++ b/js/src/jsscope.h @@ -285,7 +285,7 @@ struct JSScope : public JSObjectMap static void destroy(JSContext *cx, JSScope *scope); inline void hold(); - inline bool drop(JSContext *cx, JSObject *obj); + inline void drop(JSContext *cx, JSObject *obj); /* * Return an immutable, shareable, empty scope with the same ops as this @@ -296,6 +296,8 @@ struct JSScope : public JSObjectMap */ inline JSEmptyScope *getEmptyScope(JSContext *cx, JSClass *clasp); + inline bool ensureEmptyScope(JSContext *cx, JSClass *clasp); + inline bool canProvideEmptyScope(JSObjectOps *ops, JSClass *clasp); JSScopeProperty *lookup(jsid id); @@ -796,6 +798,22 @@ JSScope::getEmptyScope(JSContext *cx, JSClass *clasp) return createEmptyScope(cx, clasp); } +inline bool +JSScope::ensureEmptyScope(JSContext *cx, JSClass *clasp) +{ + if (emptyScope) { + JS_ASSERT(clasp == emptyScope->clasp); + return true; + } + if (!createEmptyScope(cx, clasp)) + return false; + + /* We are going to have only single ref to the scope. */ + JS_ASSERT(emptyScope->nrefs == 2); + emptyScope->nrefs = 1; + return true; +} + inline void JSScope::hold() { @@ -803,23 +821,23 @@ JSScope::hold() JS_ATOMIC_INCREMENT(&nrefs); } -inline bool +inline void JSScope::drop(JSContext *cx, JSObject *obj) { #ifdef JS_THREADSAFE - /* We are called from only js_ShareWaitingTitles and js_FinalizeObject. */ - JS_ASSERT(!obj || CX_THREAD_IS_RUNNING_GC(cx)); + /* + * We are called only from js_FinalizeObject and can avoid the overhead of + * JS_ATOMIC_DECREMENT. + */ + JS_ASSERT(CX_THREAD_IS_RUNNING_GC(cx)); #endif JS_ASSERT(nrefs > 0); --nrefs; - if (nrefs == 0) { + if (nrefs == 0) destroy(cx, this); - return false; - } - if (object == obj) + else if (object == obj) object = NULL; - return true; } inline bool