Bug 1290156 - Remove the GCRuntime DEBUG-only lockOwner field; r=terrence
☠☠ backed out by 8786fe9a1993 ☠ ☠
authorNick Fitzgerald <fitzgen@gmail.com>
Mon, 01 Aug 2016 09:45:30 -0700
changeset 351149 573529c879adbdfa560d00b70528e522d32bee97
parent 351148 51e8405321ae1a3bc7bedf8bd12e3cd5d25dc8be
child 351150 1bc0a14de00a86e2a198e40c1eb7a75442d60ce5
push id1324
push usermtabara@mozilla.com
push dateMon, 16 Jan 2017 13:07:44 +0000
treeherdermozilla-release@a01c49833940 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersterrence
bugs1290156
milestone50.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 1290156 - Remove the GCRuntime DEBUG-only lockOwner field; r=terrence The checks that the `lockOwner` member was used for are redundant now that `js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.
js/src/gc/Allocator.cpp
js/src/gc/GCRuntime.h
js/src/jsgc.cpp
js/src/jsgc.h
js/src/vm/Runtime.cpp
js/src/vm/Runtime.h
--- a/js/src/gc/Allocator.cpp
+++ b/js/src/gc/Allocator.cpp
@@ -385,17 +385,16 @@ ArenaLists::allocateFromArena(JS::Zone* 
         return nullptr;
 
     // Although our chunk should definitely have enough space for another arena,
     // there are other valid reasons why Chunk::allocateArena() may fail.
     arena = rt->gc.allocateArena(chunk, zone, thingKind, maybeLock.ref());
     if (!arena)
         return nullptr;
 
-    MOZ_ASSERT(!maybeLock->wasUnlocked());
     MOZ_ASSERT(al.isCursorAtEnd());
     al.insertBeforeCursor(arena);
 
     return allocateFromArenaInner(zone, arena, thingKind);
 }
 
 inline TenuredCell*
 ArenaLists::allocateFromArenaInner(JS::Zone* zone, Arena* arena, AllocKind kind)
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -696,42 +696,24 @@ class GCRuntime
     void waitBackgroundSweepOrAllocEnd() {
         helperState.waitBackgroundSweepEnd();
         allocTask.cancel(GCParallelTask::CancelAndWait);
     }
 
     void requestMinorGC(JS::gcreason::Reason reason);
 
 #ifdef DEBUG
-
     bool onBackgroundThread() { return helperState.onBackgroundThread(); }
-
-    bool currentThreadOwnsGCLock() {
-        return lockOwner == PR_GetCurrentThread();
-    }
-
 #endif // DEBUG
 
-    void assertCanLock() {
-        MOZ_ASSERT(!currentThreadOwnsGCLock());
-    }
-
     void lockGC() {
         lock.lock();
-#ifdef DEBUG
-        MOZ_ASSERT(!lockOwner);
-        lockOwner = PR_GetCurrentThread();
-#endif
     }
 
     void unlockGC() {
-#ifdef DEBUG
-        MOZ_ASSERT(lockOwner == PR_GetCurrentThread());
-        lockOwner = nullptr;
-#endif
         lock.unlock();
     }
 
 #ifdef DEBUG
     bool isAllocAllowed() { return noGCOrAllocationCheck == 0; }
     void disallowAlloc() { ++noGCOrAllocationCheck; }
     void allowAlloc() {
         MOZ_ASSERT(!isAllocAllowed());
@@ -1370,19 +1352,16 @@ class GCRuntime
 
     size_t noGCOrAllocationCheck;
     size_t noNurseryAllocationCheck;
 #endif
 
     /* Synchronize GC heap access between main thread and GCHelperState. */
     friend class js::AutoLockGC;
     js::Mutex lock;
-#ifdef DEBUG
-    mozilla::Atomic<PRThread*> lockOwner;
-#endif
 
     BackgroundAllocTask allocTask;
     BackgroundDecommitTask decommitTask;
     GCHelperState helperState;
 
     /*
      * During incremental sweeping, this field temporarily holds the arenas of
      * the current AllocKind being swept in order of increasing free space.
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -3272,86 +3272,76 @@ js::GetCPUCount()
 void
 GCHelperState::finish()
 {
     // Wait for any lingering background sweeping to finish.
     waitBackgroundSweepEnd();
 }
 
 GCHelperState::State
-GCHelperState::state()
-{
-    MOZ_ASSERT(rt->gc.currentThreadOwnsGCLock());
+GCHelperState::state(const AutoLockGC&)
+{
     return state_;
 }
 
 void
-GCHelperState::setState(State state)
-{
-    MOZ_ASSERT(rt->gc.currentThreadOwnsGCLock());
+GCHelperState::setState(State state, const AutoLockGC&)
+{
     state_ = state;
 }
 
 void
-GCHelperState::startBackgroundThread(State newState)
-{
-    MOZ_ASSERT(!thread && state() == IDLE && newState != IDLE);
-    setState(newState);
+GCHelperState::startBackgroundThread(State newState, const AutoLockGC& lock)
+{
+    MOZ_ASSERT(!thread && state(lock) == IDLE && newState != IDLE);
+    setState(newState, lock);
 
     {
         AutoEnterOOMUnsafeRegion noOOM;
         if (!HelperThreadState().gcHelperWorklist().append(this))
             noOOM.crash("Could not add to pending GC helpers list");
     }
 
     HelperThreadState().notifyAll(GlobalHelperThreadState::PRODUCER);
 }
 
 void
 GCHelperState::waitForBackgroundThread(js::AutoLockGC& lock)
 {
-    MOZ_ASSERT(CurrentThreadCanAccessRuntime(rt));
-
-#ifdef DEBUG
-    rt->gc.lockOwner = nullptr;
-#endif
     done.wait(lock.guard());
-#ifdef DEBUG
-    rt->gc.lockOwner = PR_GetCurrentThread();
-#endif
 }
 
 void
 GCHelperState::work()
 {
     MOZ_ASSERT(CanUseExtraThreads());
 
     AutoLockGC lock(rt);
 
     MOZ_ASSERT(!thread);
     thread = PR_GetCurrentThread();
 
     TraceLoggerThread* logger = TraceLoggerForCurrentThread();
 
-    switch (state()) {
+    switch (state(lock)) {
 
       case IDLE:
         MOZ_CRASH("GC helper triggered on idle state");
         break;
 
       case SWEEPING: {
         AutoTraceLog logSweeping(logger, TraceLogger_GCSweeping);
         doSweep(lock);
-        MOZ_ASSERT(state() == SWEEPING);
+        MOZ_ASSERT(state(lock) == SWEEPING);
         break;
       }
 
     }
 
-    setState(IDLE);
+    setState(IDLE, lock);
     thread = nullptr;
 
     done.notify_all();
 }
 
 void
 GCRuntime::queueZonesForBackgroundSweep(ZoneList& zones)
 {
@@ -3384,25 +3374,25 @@ GCRuntime::freeAllLifoBlocksAfterMinorGC
     blocksToFreeAfterMinorGC.transferFrom(lifo);
 }
 
 void
 GCHelperState::maybeStartBackgroundSweep(const AutoLockGC& lock)
 {
     MOZ_ASSERT(CanUseExtraThreads());
 
-    if (state() == IDLE)
-        startBackgroundThread(SWEEPING);
+    if (state(lock) == IDLE)
+        startBackgroundThread(SWEEPING, lock);
 }
 
 void
 GCHelperState::waitBackgroundSweepEnd()
 {
     AutoLockGC lock(rt);
-    while (state() == SWEEPING)
+    while (state(lock) == SWEEPING)
         waitForBackgroundThread(lock);
     if (!rt->gc.isIncrementalGCInProgress())
         rt->gc.assertBackgroundSweepingFinished();
 }
 
 void
 GCHelperState::doSweep(AutoLockGC& lock)
 {
--- a/js/src/jsgc.h
+++ b/js/src/jsgc.h
@@ -860,21 +860,21 @@ class GCHelperState
     js::ConditionVariable done;
 
     // Activity for the helper to do, protected by the GC lock.
     State state_;
 
     // Thread which work is being performed on, or null.
     PRThread* thread;
 
-    void startBackgroundThread(State newState);
+    void startBackgroundThread(State newState, const js::AutoLockGC&);
     void waitForBackgroundThread(js::AutoLockGC& lock);
 
-    State state();
-    void setState(State state);
+    State state(const AutoLockGC&);
+    void setState(State state, const AutoLockGC&);
 
     friend class js::gc::ArenaLists;
 
     static void freeElementsAndArray(void** array, void** end) {
         MOZ_ASSERT(array <= end);
         for (void** p = array; p != end; ++p)
             js_free(*p);
         js_free(array);
--- a/js/src/vm/Runtime.cpp
+++ b/js/src/vm/Runtime.cpp
@@ -900,17 +900,16 @@ JSRuntime::assertCanLock(RuntimeLock whi
     switch (which) {
       case ExclusiveAccessLock:
         MOZ_ASSERT(exclusiveAccessOwner != PR_GetCurrentThread());
         MOZ_FALLTHROUGH;
       case HelperThreadStateLock:
         MOZ_ASSERT(!HelperThreadState().isLocked());
         MOZ_FALLTHROUGH;
       case GCLock:
-        gc.assertCanLock();
         break;
       default:
         MOZ_CRASH();
     }
 }
 
 void
 js::AssertCurrentThreadCanLock(RuntimeLock which)
--- a/js/src/vm/Runtime.h
+++ b/js/src/vm/Runtime.h
@@ -1417,63 +1417,42 @@ FreeOp::appendJitPoisonRange(const jit::
  * passed a non-const reference to this class.
  */
 class MOZ_RAII AutoLockGC
 {
   public:
     explicit AutoLockGC(JSRuntime* rt
                         MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
       : runtime_(rt)
-#ifdef DEBUG
-      , wasUnlocked_(false)
-#endif
     {
         MOZ_GUARD_OBJECT_NOTIFIER_INIT;
         lock();
     }
 
     ~AutoLockGC() {
         unlock();
     }
 
     void lock() {
         MOZ_ASSERT(lockGuard_.isNothing());
         lockGuard_.emplace(runtime_->gc.lock);
-#ifdef DEBUG
-        runtime_->gc.lockOwner = PR_GetCurrentThread();
-#endif
     }
 
     void unlock() {
         MOZ_ASSERT(lockGuard_.isSome());
-#ifdef DEBUG
-        runtime_->gc.lockOwner = nullptr;
-#endif
         lockGuard_.reset();
-#ifdef DEBUG
-        wasUnlocked_ = true;
-#endif
     }
 
     js::LockGuard<js::Mutex>& guard() {
         return lockGuard_.ref();
     }
 
-#ifdef DEBUG
-    bool wasUnlocked() {
-        return wasUnlocked_;
-    }
-#endif
-
   private:
     JSRuntime* runtime_;
     mozilla::Maybe<js::LockGuard<js::Mutex>> lockGuard_;
-#ifdef DEBUG
-    bool wasUnlocked_;
-#endif
     MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
 
     AutoLockGC(const AutoLockGC&) = delete;
     AutoLockGC& operator=(const AutoLockGC&) = delete;
 };
 
 class MOZ_RAII AutoUnlockGC
 {