From 149843723d5697bbab036d38b3e6bb75d4ca8287 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Sun, 10 May 2020 16:21:25 +0200 Subject: [PATCH] [wasm][debug] Make recompilation isolate-independent Passing an isolate to {RecompileNativeModule} feels wrong, since compilation and the generated code are totally isolate-independent. In fact, the isolate is only used for updating counters. Instead of passing the counters instead, this CL just refactors the code to support a nullptr for the counters everywhere (some code paths already supported that). The few recompilation would not make a significant difference in the histograms anyway, and even have the risk of skewing the data. Drive-by 1: Rename {TierUp} to {StartTierUp} and update comments. Drive-by 2: Remove non-actionable TODO. R=thibaudm@chromium.org Bug: v8:10359 Change-Id: Ic027f939bbc55398b90784922130fe1fe5573b0c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2187638 Reviewed-by: Thibaud Michaud Commit-Queue: Clemens Backes Cr-Commit-Position: refs/heads/master@{#67708} --- src/compiler/wasm-compiler.cc | 7 ++++--- src/runtime/runtime-test.cc | 4 ++-- src/wasm/function-compiler.cc | 18 +++++++++++------- src/wasm/module-compiler.cc | 5 +++-- src/wasm/module-compiler.h | 2 +- src/wasm/wasm-code-manager.cc | 8 ++++---- src/wasm/wasm-code-manager.h | 11 +++++++---- src/wasm/wasm-engine.cc | 4 ++-- test/cctest/wasm/wasm-run-utils.h | 2 +- 9 files changed, 35 insertions(+), 26 deletions(-) diff --git a/src/compiler/wasm-compiler.cc b/src/compiler/wasm-compiler.cc index cf4a586cc3..38b186ca02 100644 --- a/src/compiler/wasm-compiler.cc +++ b/src/compiler/wasm-compiler.cc @@ -6788,9 +6788,10 @@ wasm::WasmCompilationResult ExecuteTurbofanWasmCompilation( &info, wasm_engine, mcgraph, call_descriptor, source_positions, node_origins, func_body, env->module, func_index); - // TODO(bradnelson): Improve histogram handling of size_t. - counters->wasm_compile_function_peak_memory_bytes()->AddSample( - static_cast(mcgraph->graph()->zone()->allocation_size())); + if (counters) { + counters->wasm_compile_function_peak_memory_bytes()->AddSample( + static_cast(mcgraph->graph()->zone()->allocation_size())); + } auto result = info.ReleaseWasmCompilationResult(); CHECK_NOT_NULL(result); // Compilation expected to succeed. DCHECK_EQ(wasm::ExecutionTier::kTurbofan, result->result_tier); diff --git a/src/runtime/runtime-test.cc b/src/runtime/runtime-test.cc index b69068cc5a..16a75dce69 100644 --- a/src/runtime/runtime-test.cc +++ b/src/runtime/runtime-test.cc @@ -1383,7 +1383,7 @@ RUNTIME_FUNCTION(Runtime_WasmTierDownModule) { DCHECK_EQ(1, args.length()); CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0); auto* native_module = instance->module_object().native_module(); - native_module->TierDown(isolate); + native_module->TierDown(); CHECK(!native_module->compilation_state()->failed()); return ReadOnlyRoots(isolate).undefined_value(); } @@ -1393,7 +1393,7 @@ RUNTIME_FUNCTION(Runtime_WasmTierUpModule) { DCHECK_EQ(1, args.length()); CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0); auto* native_module = instance->module_object().native_module(); - native_module->TierUp(isolate); + native_module->StartTierUp(); CHECK(!native_module->compilation_state()->failed()); return ReadOnlyRoots(isolate).undefined_value(); } diff --git a/src/wasm/function-compiler.cc b/src/wasm/function-compiler.cc index f1feae7a33..6b25520d84 100644 --- a/src/wasm/function-compiler.cc +++ b/src/wasm/function-compiler.cc @@ -130,7 +130,7 @@ WasmCompilationResult WasmCompilationUnit::ExecuteCompilation( counters, detected); } - if (result.succeeded()) { + if (result.succeeded() && counters) { counters->wasm_generated_code_size()->Increment( result.code_desc.instr_size); counters->wasm_reloc_size()->Increment(result.code_desc.reloc_size); @@ -163,12 +163,16 @@ WasmCompilationResult WasmCompilationUnit::ExecuteFunctionCompilation( wasm::FunctionBody func_body{func->sig, func->code.offset(), code.begin(), code.end()}; - auto size_histogram = SELECT_WASM_COUNTER(counters, env->module->origin, wasm, - function_size_bytes); - size_histogram->AddSample(static_cast(func_body.end - func_body.start)); - auto timed_histogram = SELECT_WASM_COUNTER(counters, env->module->origin, - wasm_compile, function_time); - TimedHistogramScope wasm_compile_function_time_scope(timed_histogram); + base::Optional wasm_compile_function_time_scope; + if (counters) { + auto size_histogram = SELECT_WASM_COUNTER(counters, env->module->origin, + wasm, function_size_bytes); + size_histogram->AddSample( + static_cast(func_body.end - func_body.start)); + auto timed_histogram = SELECT_WASM_COUNTER(counters, env->module->origin, + wasm_compile, function_time); + wasm_compile_function_time_scope.emplace(timed_histogram); + } if (FLAG_trace_wasm_compiler) { PrintF("Compiling wasm function %d with %s\n", func_index_, diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc index 84472ea194..9f6e91c73e 100644 --- a/src/wasm/module-compiler.cc +++ b/src/wasm/module-compiler.cc @@ -1466,7 +1466,7 @@ std::shared_ptr CompileToNativeModule( return native_module; } -void RecompileNativeModule(Isolate* isolate, NativeModule* native_module, +void RecompileNativeModule(NativeModule* native_module, TieringState tiering_state) { // Install a callback to notify us once background recompilation finished. auto recompilation_finished_semaphore = std::make_shared(0); @@ -1484,8 +1484,9 @@ void RecompileNativeModule(Isolate* isolate, NativeModule* native_module, // We only wait for tier down. Tier up can happen in the background. if (tiering_state == kTieredDown) { // The main thread contributes to the compilation. + constexpr Counters* kNoCounters = nullptr; while (ExecuteCompilationUnits( - compilation_state->background_compile_token(), isolate->counters(), + compilation_state->background_compile_token(), kNoCounters, kMainThreadTaskId, kBaselineOnly)) { // Continue executing compilation units. } diff --git a/src/wasm/module-compiler.h b/src/wasm/module-compiler.h index b0019af5d5..a3fc4037a2 100644 --- a/src/wasm/module-compiler.h +++ b/src/wasm/module-compiler.h @@ -44,7 +44,7 @@ std::shared_ptr CompileToNativeModule( std::shared_ptr module, const ModuleWireBytes& wire_bytes, Handle* export_wrappers_out); -void RecompileNativeModule(Isolate* isolate, NativeModule* native_module, +void RecompileNativeModule(NativeModule* native_module, TieringState new_tiering_state); V8_EXPORT_PRIVATE diff --git a/src/wasm/wasm-code-manager.cc b/src/wasm/wasm-code-manager.cc index 173a4889cd..d6e8d17753 100644 --- a/src/wasm/wasm-code-manager.cc +++ b/src/wasm/wasm-code-manager.cc @@ -1853,17 +1853,17 @@ bool NativeModule::IsTieredDown() { return tiering_state_ == kTieredDown; } -void NativeModule::TierDown(Isolate* isolate) { +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; - RecompileNativeModule(isolate, this, kTieredDown); + RecompileNativeModule(this, kTieredDown); } -void NativeModule::TierUp(Isolate* isolate) { +void NativeModule::StartTierUp() { // Do not tier up asm.js. if (module()->origin != kWasmOrigin) return; @@ -1873,7 +1873,7 @@ void NativeModule::TierUp(Isolate* isolate) { tiering_state_ = kTieredUp; } - RecompileNativeModule(isolate, this, kTieredUp); + RecompileNativeModule(this, kTieredUp); } void NativeModule::FreeCode(Vector codes) { diff --git a/src/wasm/wasm-code-manager.h b/src/wasm/wasm-code-manager.h index fa933ac9aa..4162bfe022 100644 --- a/src/wasm/wasm-code-manager.h +++ b/src/wasm/wasm-code-manager.h @@ -600,10 +600,13 @@ class V8_EXPORT_PRIVATE NativeModule final { bool SetTieredDown(); bool IsTieredDown(); - // Sets the flag, triggers recompilation of all methods to tier down or up, - // waits for that to complete. - void TierDown(Isolate* isolate); - void TierUp(Isolate* isolate); + // Set the flag to keep this module tiered down, trigger recompilation of all + // functions, and wait for recompilation to complete. + void TierDown(); + + // Clear the flag to keep this module tiered down and trigger recompilation + // of all functions. Does not wait for completion of recompilation. + void StartTierUp(); // Free a set of functions of this module. Uncommits whole pages if possible. // The given vector must be ordered by the instruction start address, and all diff --git a/src/wasm/wasm-engine.cc b/src/wasm/wasm-engine.cc index a31edff6b9..685965f7a9 100644 --- a/src/wasm/wasm-engine.cc +++ b/src/wasm/wasm-engine.cc @@ -626,7 +626,7 @@ void WasmEngine::TierDownAllModulesPerIsolate(Isolate* isolate) { } } for (auto* native_module : native_modules) { - native_module->TierDown(isolate); + native_module->TierDown(); } } @@ -640,7 +640,7 @@ void WasmEngine::TierUpAllModulesPerIsolate(Isolate* isolate) { } } for (auto* native_module : native_modules) { - native_module->TierUp(isolate); + native_module->StartTierUp(); } } diff --git a/test/cctest/wasm/wasm-run-utils.h b/test/cctest/wasm/wasm-run-utils.h index cff11695f1..f5a143de0d 100644 --- a/test/cctest/wasm/wasm-run-utils.h +++ b/test/cctest/wasm/wasm-run-utils.h @@ -221,7 +221,7 @@ class TestingModuleBuilder { void SetExecutable() { native_module_->SetExecutable(true); } void TierDown() { - native_module_->TierDown(isolate_); + native_module_->TierDown(); execution_tier_ = ExecutionTier::kLiftoff; }