Bug 1608462 - Simplify locking for ScriptPreloader::MaybeFinishOffThreadDecode. r=kmag,Gankro,decoder
authorTed Campbell <tcampbell@mozilla.com>
Mon, 14 Dec 2020 15:23:34 +0000
changeset 560611 99f2216bb7ebc744317bda8a5b6c3806dfda4bb4
parent 560610 2f43fcc8980c13eacfd1d490a1bfdcec692ec5af
child 560612 2497e2dd2ae83a5bce8a5fe49861f53ed164f759
push id38032
push usercsabou@mozilla.com
push dateTue, 15 Dec 2020 09:29:54 +0000
treeherdermozilla-central@f805f27183c3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, Gankro, decoder
bugs1608462
milestone85.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 1608462 - Simplify locking for ScriptPreloader::MaybeFinishOffThreadDecode. r=kmag,Gankro,decoder Be consistent about always calling `MaybeFinishOffThreadDecode` without holding the lock to simplify code. This lets us remove a TSAN deadlock exception. Differential Revision: https://phabricator.services.mozilla.com/D99402
js/xpconnect/loader/ScriptPreloader.cpp
mozglue/build/TsanOptions.cpp
--- a/js/xpconnect/loader/ScriptPreloader.cpp
+++ b/js/xpconnect/loader/ScriptPreloader.cpp
@@ -906,37 +906,36 @@ JSScript* ScriptPreloader::WaitForCached
   // most one batch of buffered scripts, and occasionally under-running that
   // buffer.
   MaybeFinishOffThreadDecode();
 
   if (!script->mReadyToExecute) {
     LOG(Info, "Must wait for async script load: %s\n", script->mURL.get());
     auto start = TimeStamp::Now();
 
-    mMonitor.AssertNotCurrentThreadOwns();
-    MonitorAutoLock mal(mMonitor);
-
-    // Check for finished operations again *after* locking, or we may race
-    // against mToken being set between our last check and the time we
-    // entered the mutex.
-    MaybeFinishOffThreadDecode();
-
-    if (!script->mReadyToExecute &&
-        script->mSize < MAX_MAINTHREAD_DECODE_SIZE) {
+    // If script is small enough, we'd rather recompile on main-thread than wait
+    // for a decode task to complete.
+    if (script->mSize < MAX_MAINTHREAD_DECODE_SIZE) {
       LOG(Info, "Script is small enough to recompile on main thread\n");
 
       script->mReadyToExecute = true;
       Telemetry::ScalarAdd(
           Telemetry::ScalarID::SCRIPT_PRELOADER_MAINTHREAD_RECOMPILE, 1);
     } else {
-      while (!script->mReadyToExecute) {
-        mal.Wait();
+      MonitorAutoLock mal(mMonitor);
 
-        MonitorAutoUnlock mau(mMonitor);
-        MaybeFinishOffThreadDecode();
+      // Process script batches until our target is found.
+      while (!script->mReadyToExecute) {
+        if (mToken) {
+          MonitorAutoUnlock mau(mMonitor);
+          MaybeFinishOffThreadDecode();
+        } else {
+          MOZ_ASSERT(!mParsingScripts.empty());
+          mal.Wait();
+        }
       }
     }
 
     double waitedMS = (TimeStamp::Now() - start).ToMilliseconds();
     Telemetry::Accumulate(Telemetry::SCRIPT_PRELOADER_WAIT_TIME, int(waitedMS));
     LOG(Debug, "Waited %fms\n", waitedMS);
   }
 
@@ -965,33 +964,38 @@ void ScriptPreloader::OffThreadDecodeCal
         NewRunnableMethod("ScriptPreloader::DoFinishOffThreadDecode", cache,
                           &ScriptPreloader::DoFinishOffThreadDecode));
   }
 }
 
 void ScriptPreloader::FinishPendingParses(MonitorAutoLock& aMal) {
   mMonitor.AssertCurrentThreadOwns();
 
+  // Clear out scripts that we have not issued batch for yet.
   mPendingScripts.clear();
 
-  MaybeFinishOffThreadDecode();
-
-  // Loop until all pending decode operations finish.
+  // Process any pending decodes that are in flight.
   while (!mParsingScripts.empty()) {
-    aMal.Wait();
-    MaybeFinishOffThreadDecode();
+    if (mToken) {
+      MonitorAutoUnlock mau(mMonitor);
+      MaybeFinishOffThreadDecode();
+    } else {
+      aMal.Wait();
+    }
   }
 }
 
 void ScriptPreloader::DoFinishOffThreadDecode() {
   mFinishDecodeRunnablePending = false;
   MaybeFinishOffThreadDecode();
 }
 
 void ScriptPreloader::MaybeFinishOffThreadDecode() {
+  mMonitor.AssertNotCurrentThreadOwns();
+
   if (!mToken) {
     return;
   }
 
   auto cleanup = MakeScopeExit([&]() {
     mToken = nullptr;
     mParsingSources.clear();
     mParsingScripts.clear();
--- a/mozglue/build/TsanOptions.cpp
+++ b/mozglue/build/TsanOptions.cpp
@@ -233,19 +233,16 @@ extern "C" const char* __tsan_default_su
          "race:nsSocketTransport::Close\n"
          "race:nsSocketTransport::OnSocketDetached\n"
          "race:nsSocketTransport::OnMsgInputClosed\n"
          "race:nsSocketTransport::OpenOutputStream\n"
 
          // Bug 1607138
          "race:gXPCOMThreadsShutDown\n"
 
-         // Bug 1608462
-         "deadlock:ScriptPreloader::OffThreadDecodeCallback\n"
-
          // Bug 1615017
          "race:CacheFileMetadata::SetHash\n"
          "race:CacheFileMetadata::OnDataWritten\n"
 
          // Bug 1615123
          "race:_dl_deallocate_tls\n"
          "race:__libc_memalign\n"