From 902f48bddad82337349ea3bc5029b38d9ffec11a Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Mon, 11 May 2020 16:10:07 +0200 Subject: [PATCH] [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 Reviewed-by: Thibaud Michaud Cr-Commit-Position: refs/heads/master@{#67716} --- src/wasm/wasm-code-manager.cc | 10 ++--- src/wasm/wasm-code-manager.h | 8 ++-- src/wasm/wasm-engine.cc | 41 +++++++++++++------ .../wasm/debug-enabled-tier-down-wasm.js | 13 +++--- test/debugger/debugger.status | 10 ----- 5 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/wasm/wasm-code-manager.cc b/src/wasm/wasm-code-manager.cc index d6e8d17753..2df8eadffd 100644 --- a/src/wasm/wasm-code-manager.cc +++ b/src/wasm/wasm-code-manager.cc @@ -1838,14 +1838,12 @@ std::vector> 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); } diff --git a/src/wasm/wasm-code-manager.h b/src/wasm/wasm-code-manager.h index 4162bfe022..c9d617362f 100644 --- a/src/wasm/wasm-code-manager.h +++ b/src/wasm/wasm-code-manager.h @@ -595,9 +595,11 @@ class V8_EXPORT_PRIVATE NativeModule final { V8_WARN_UNUSED_RESULT std::vector> AddCompiledCode( Vector); - // 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 diff --git a/src/wasm/wasm-engine.cc b/src/wasm/wasm-engine.cc index 685965f7a9..9a1cbd10e0 100644 --- a/src/wasm/wasm-engine.cc +++ b/src/wasm/wasm-engine.cc @@ -631,15 +631,20 @@ void WasmEngine::TierDownAllModulesPerIsolate(Isolate* isolate) { } void WasmEngine::TierUpAllModulesPerIsolate(Isolate* isolate) { - std::vector native_modules; - { - base::MutexGuard lock(&mutex_); - isolates_[isolate]->keep_tiered_down = false; - for (auto* native_module : isolates_[isolate]->native_modules) { - native_modules.push_back(native_module); + 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; } - } - for (auto* native_module : native_modules) { + return false; + }; + for (auto* native_module : isolates_[isolate]->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 WasmEngine::MaybeGetNativeModule( ModuleOrigin origin, Vector wire_bytes, Isolate* isolate) { std::shared_ptr 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 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; - 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()); + 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; } diff --git a/test/debugger/debug/wasm/debug-enabled-tier-down-wasm.js b/test/debugger/debug/wasm/debug-enabled-tier-down-wasm.js index 20dc1e1c5e..d2d1c5480f 100644 --- a/test/debugger/debug/wasm/debug-enabled-tier-down-wasm.js +++ b/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()); diff --git a/test/debugger/debugger.status b/test/debugger/debugger.status index 71d0381196..6c2f06538c 100644 --- a/test/debugger/debugger.status +++ b/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.