[wasm][debug] Fix tier down for multiple isolates

If multiple isolates are using the same module, we need to keep it
tiered down as long as any isolate still has a debugger open.
Also, we cannot short-cut the {NativeModule::TierDown} method, since the
previously triggered tier down might not have finished yet.
For now, each isolate starts an independent tier down (i.e. a full
recompilation). We could optimize this later by skipping functions that
are already tiered down, or are already scheduled for tier down, but we
still need to wait for tier-down to finish on each isolate.

R=thibaudm@chromium.org

Bug: v8:10359
Change-Id: I7ea6a6f5d3977e48718ac5bc94f9831541f6173f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2190758
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67716}

src/wasm/wasm-code-manager.cc

@@ -1838,14 +1838,12 @@ std::vector<std::unique_ptr<WasmCode>> NativeModule::AddCompiledCode(
   return generated_code;
 }
 
-bool NativeModule::SetTieredDown() {
+void NativeModule::SetTieredDown() {
   // Do not tier down asm.js.
-  if (module()->origin != kWasmOrigin) return true;
+  if (module()->origin != kWasmOrigin) return;
 
   base::MutexGuard lock(&allocation_mutex_);
-  if (tiering_state_ == kTieredDown) return false;
   tiering_state_ = kTieredDown;
-  return true;
 }
 
 bool NativeModule::IsTieredDown() {
@@ -1857,9 +1855,7 @@ void NativeModule::TierDown() {
   // Do not tier down asm.js.
   if (module()->origin != kWasmOrigin) return;
 
-  // Set the module to tiered down state; return if it is already in that state.
-  if (!SetTieredDown()) return;
-
+  SetTieredDown();
   RecompileNativeModule(this, kTieredDown);
 }
 

src/wasm/wasm-code-manager.h

@@ -595,9 +595,11 @@ class V8_EXPORT_PRIVATE NativeModule final {
   V8_WARN_UNUSED_RESULT std::vector<std::unique_ptr<WasmCode>> AddCompiledCode(
       Vector<WasmCompilationResult>);
 
-  // Set to tiered down state. Returns {true} if this caused a change, {false}
-  // otherwise.
-  bool SetTieredDown();
+  // Set the module to tiered down state. Triggering recompilation (if
+  // necessary) has to be done by the caller.
+  void SetTieredDown();
+
+  // Check whether this modules is tiered down for debugging.
   bool IsTieredDown();
 
   // Set the flag to keep this module tiered down, trigger recompilation of all

src/wasm/wasm-engine.cc

@@ -631,15 +631,20 @@ void WasmEngine::TierDownAllModulesPerIsolate(Isolate* isolate) {
 }
 
 void WasmEngine::TierUpAllModulesPerIsolate(Isolate* isolate) {
-  std::vector<NativeModule*> native_modules;
-  {
   base::MutexGuard lock(&mutex_);
   isolates_[isolate]->keep_tiered_down = false;
+  auto test_keep_tiered_down = [this](NativeModule* native_module) {
+    DCHECK_EQ(1, native_modules_.count(native_module));
+    for (auto* isolate : native_modules_[native_module]->isolates) {
+      DCHECK_EQ(1, isolates_.count(isolate));
+      if (isolates_[isolate]->keep_tiered_down) return true;
+    }
+    return false;
+  };
   for (auto* native_module : isolates_[isolate]->native_modules) {
-      native_modules.push_back(native_module);
-    }
-  }
-  for (auto* native_module : native_modules) {
+    // Only start tier-up if no other isolate needs this modules in tiered down
+    // state.
+    if (test_keep_tiered_down(native_module)) continue;
     native_module->StartTierUp();
   }
 }
@@ -1001,6 +1006,7 @@ std::shared_ptr<NativeModule> WasmEngine::MaybeGetNativeModule(
     ModuleOrigin origin, Vector<const uint8_t> wire_bytes, Isolate* isolate) {
   std::shared_ptr<NativeModule> native_module =
       native_module_cache_.MaybeGetNativeModule(origin, wire_bytes);
+  bool tier_down_module = false;
   if (native_module) {
     base::MutexGuard guard(&mutex_);
     auto& native_module_info = native_modules_[native_module.get()];
@@ -1009,7 +1015,10 @@ std::shared_ptr<NativeModule> WasmEngine::MaybeGetNativeModule(
     }
     native_module_info->isolates.insert(isolate);
     isolates_[isolate]->native_modules.insert(native_module.get());
+    tier_down_module = isolates_[isolate]->keep_tiered_down;
   }
+  // Potentially tier down after releasing the mutex.
+  if (tier_down_module) native_module->TierDown();
   return native_module;
 }
 
@@ -1025,11 +1034,17 @@ bool WasmEngine::UpdateNativeModuleCache(
 
   if (prev == native_module->get()) return true;
 
+  bool tier_down_module = false;
+  {
     base::MutexGuard guard(&mutex_);
     DCHECK_EQ(1, native_modules_.count(native_module->get()));
     native_modules_[native_module->get()]->isolates.insert(isolate);
     DCHECK_EQ(1, isolates_.count(isolate));
     isolates_[isolate]->native_modules.insert(native_module->get());
+    tier_down_module = isolates_[isolate]->keep_tiered_down;
+  }
+  // Potentially tier down after releasing the mutex.
+  if (tier_down_module) native_module->get()->TierDown();
   return false;
 }
 

test/debugger/debug/wasm/debug-enabled-tier-down-wasm.js

@@ -23,7 +23,7 @@ function checkTieredDown(instance) {
   }
 }
 
-function checkTieredUp(instance) {
+function waitForTieredUp(instance) {
   // Busy waiting until all functions are tiered up.
   let num_liftoff_functions = 0;
   while (true) {
@@ -37,6 +37,8 @@ function checkTieredUp(instance) {
   }
 }
 
+// In the 'isolates' test, this test runs in parallel to itself on two isolates.
+// All checks below should still hold.
 const instance = create_builder().instantiate();
 const Debug = new DebugWrapper();
 Debug.enable();
@@ -44,8 +46,9 @@ checkTieredDown(instance);
 const newInstance = create_builder(num_functions*2).instantiate();
 checkTieredDown(newInstance);
 Debug.disable();
-checkTieredUp(instance);
-checkTieredUp(newInstance);
+// Eventually the instances will be completely tiered up again.
+waitForTieredUp(instance);
+waitForTieredUp(newInstance);
 
 // Async.
 async function testTierDownToLiftoffAsync() {
@@ -55,8 +58,8 @@ async function testTierDownToLiftoffAsync() {
   const newAsyncInstance = await create_builder(num_functions*3).asyncInstantiate();
   checkTieredDown(newAsyncInstance);
   Debug.disable();
-  checkTieredUp(asyncInstance);
-  checkTieredUp(newAsyncInstance);
+  waitForTieredUp(asyncInstance);
+  waitForTieredUp(newAsyncInstance);
 }
 
 assertPromiseResult(testTierDownToLiftoffAsync());

test/debugger/debugger.status

@@ -152,16 +152,6 @@
   'debug/wasm/frame-inspection': [SKIP],
 }],
 
-##############################################################################
-['isolates', {
-  # WebAssembly debugging does not work reliably when multiple isolates are
-  # involved (https://crbug.com/v8/10359).
-  # (this list might need to be extended by more debugging tests as they
-  # start flaking)
-  'debug/wasm/*': [SKIP],
-  'regress/regress-crbug-1032042': [SKIP],
-}],  # 'isolates'
-
 ################################################################################
 ['variant == stress_snapshot', {
   '*': [SKIP],  # only relevant for mjsunit tests.