From c582eb4ee2e80b6888312d3532028aae98861abe Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Thu, 15 Sep 2022 18:58:51 +0200 Subject: [PATCH] [wasm] Use a single source of truth for feedback vector size The number of feedback vector slots is currently stored in the {WasmFunction}, returned in the {WasmCompilationResult}, and implicitly stored as the size of the {call_targets} vector in {FunctionTypeFeedback}. This CL uses the latter as the source of truth, encapsulated in a new {NumFeedbackSlots} function. This can be updated when adding new kinds of feedback that need additional slots. For now, the implementation of {NumFeedbackSlots} requires taking a mutex, which we can hopefully avoid when productionizing speculative inlining. We also take the mutex on every Liftoff compilation, which adds synchronization between concurrent compilation which we previously tried very hard to avoid (because it introduced significant overhead for eager compilation). As a nice side-effect, this CL reduces the per-function overhead by 8 bytes, independent of enabled features. R=jkummerow@chromium.org Bug: v8:13209 Change-Id: I2fe5f7fe73154328032a3f0961e88d068c5d07ae Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3899299 Reviewed-by: Jakob Kummerow Commit-Queue: Clemens Backes Cr-Commit-Position: refs/heads/main@{#83253} --- src/wasm/baseline/liftoff-compiler.cc | 7 ------- src/wasm/function-compiler.h | 1 - src/wasm/module-compiler.cc | 7 ++++--- src/wasm/module-decoder-impl.h | 2 -- src/wasm/module-instantiate.cc | 3 +-- src/wasm/wasm-code-manager.cc | 10 ---------- src/wasm/wasm-module.cc | 12 ++++++++++++ src/wasm/wasm-module.h | 7 +++---- test/cctest/wasm/wasm-run-utils.cc | 1 - test/common/wasm/wasm-interpreter.cc | 1 - .../unittests/wasm/function-body-decoder-unittest.cc | 1 - 11 files changed, 20 insertions(+), 32 deletions(-) diff --git a/src/wasm/baseline/liftoff-compiler.cc b/src/wasm/baseline/liftoff-compiler.cc index 120e325b66..eaed311111 100644 --- a/src/wasm/baseline/liftoff-compiler.cc +++ b/src/wasm/baseline/liftoff-compiler.cc @@ -556,12 +556,6 @@ class LiftoffCompiler { return __ GetTotalFrameSlotCountForGC(); } - int GetFeedbackVectorSlots() const { - // The number of call instructions is capped by max function size. - static_assert(kV8MaxWasmFunctionSize < std::numeric_limits::max() / 2); - return static_cast(encountered_call_instructions_.size()) * 2; - } - void unsupported(FullDecoder* decoder, LiftoffBailoutReason reason, const char* detail) { DCHECK_NE(kSuccess, reason); @@ -7709,7 +7703,6 @@ WasmCompilationResult ExecuteLiftoffCompilation( if (auto* debug_sidetable = compiler_options.debug_sidetable) { *debug_sidetable = debug_sidetable_builder->GenerateDebugSideTable(); } - result.feedback_vector_slots = compiler->GetFeedbackVectorSlots(); if (V8_UNLIKELY(v8_flags.trace_wasm_compilation_times)) { base::TimeDelta time = base::TimeTicks::Now() - start_time; diff --git a/src/wasm/function-compiler.h b/src/wasm/function-compiler.h index 6d2d17cca2..babf57f66e 100644 --- a/src/wasm/function-compiler.h +++ b/src/wasm/function-compiler.h @@ -57,7 +57,6 @@ struct WasmCompilationResult { ExecutionTier result_tier; Kind kind = kFunction; ForDebugging for_debugging = kNoDebugging; - int feedback_vector_slots = 0; }; class V8_EXPORT_PRIVATE WasmCompilationUnit final { diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc index 55806d5141..6d8fbd5270 100644 --- a/src/wasm/module-compiler.cc +++ b/src/wasm/module-compiler.cc @@ -1247,13 +1247,14 @@ bool CompileLazy(Isolate* isolate, Handle instance, } // Allocate feedback vector if needed. - if (result.feedback_vector_slots > 0) { + int feedback_vector_slots = NumFeedbackSlots(module, func_index); + if (feedback_vector_slots > 0) { DCHECK(v8_flags.wasm_speculative_inlining); // We have to save the native_module on the stack, in case the allocation // triggers a GC and we need the module to scan WasmCompileLazy stack frame. *out_native_module = native_module; - Handle vector = isolate->factory()->NewFixedArrayWithZeroes( - result.feedback_vector_slots); + Handle vector = + isolate->factory()->NewFixedArrayWithZeroes(feedback_vector_slots); instance->feedback_vectors().set( declared_function_index(module, func_index), *vector); } diff --git a/src/wasm/module-decoder-impl.h b/src/wasm/module-decoder-impl.h index 72c777e490..37cc426a04 100644 --- a/src/wasm/module-decoder-impl.h +++ b/src/wasm/module-decoder-impl.h @@ -787,7 +787,6 @@ class ModuleDecoderTemplate : public Decoder { import->index, // func_index 0, // sig_index {0, 0}, // code - 0, // feedback slots true, // imported false, // exported false}); // declared @@ -884,7 +883,6 @@ class ModuleDecoderTemplate : public Decoder { func_index, // func_index 0, // sig_index {0, 0}, // code - 0, // feedback slots false, // imported false, // exported false}); // declared diff --git a/src/wasm/module-instantiate.cc b/src/wasm/module-instantiate.cc index ef0ec3cdac..05a611fcff 100644 --- a/src/wasm/module-instantiate.cc +++ b/src/wasm/module-instantiate.cc @@ -744,8 +744,7 @@ MaybeHandle InstanceBuilder::Build() { instance->set_feedback_vectors(*vectors); for (int i = 0; i < num_functions; i++) { int func_index = module_->num_imported_functions + i; - int slots = - base::Relaxed_Load(&module_->functions[func_index].feedback_slots); + int slots = NumFeedbackSlots(module_, func_index); if (slots == 0) continue; if (v8_flags.trace_wasm_speculative_inlining) { PrintF("[Function %d (declared %d): allocating %d feedback slots]\n", diff --git a/src/wasm/wasm-code-manager.cc b/src/wasm/wasm-code-manager.cc index 47770f2d9f..23db8f4caa 100644 --- a/src/wasm/wasm-code-manager.cc +++ b/src/wasm/wasm-code-manager.cc @@ -2336,16 +2336,6 @@ std::vector> NativeModule::AddCompiledCode( for (auto& result : results) { DCHECK(result.succeeded()); total_code_space += RoundUp(result.code_desc.instr_size); - if (result.result_tier == ExecutionTier::kLiftoff) { - int index = result.func_index; - int* slots = &module()->functions[index].feedback_slots; -#if DEBUG - int current_value = base::Relaxed_Load(slots); - DCHECK(current_value == 0 || - current_value == result.feedback_vector_slots); -#endif - base::Relaxed_Store(slots, result.feedback_vector_slots); - } } base::Vector code_space; NativeModule::JumpTablesRef jump_tables; diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc index 219ccf0d45..8596325428 100644 --- a/src/wasm/wasm-module.cc +++ b/src/wasm/wasm-module.cc @@ -678,4 +678,16 @@ size_t GetWireBytesHash(base::Vector wire_bytes) { kZeroHashSeed); } +int NumFeedbackSlots(const WasmModule* module, int func_index) { + if (!v8_flags.wasm_speculative_inlining) return 0; + // TODO(clemensb): Avoid the mutex once this ships, or at least switch to a + // shared mutex. + base::MutexGuard type_feedback_guard{&module->type_feedback.mutex}; + auto it = module->type_feedback.feedback_for_function.find(func_index); + if (it == module->type_feedback.feedback_for_function.end()) return 0; + // The number of call instructions is capped by max function size. + static_assert(kV8MaxWasmFunctionSize < std::numeric_limits::max() / 2); + return static_cast(2 * it->second.call_targets.size()); +} + } // namespace v8::internal::wasm diff --git a/src/wasm/wasm-module.h b/src/wasm/wasm-module.h index d7952052da..7df8668bf6 100644 --- a/src/wasm/wasm-module.h +++ b/src/wasm/wasm-module.h @@ -63,10 +63,6 @@ struct WasmFunction { uint32_t func_index; // index into the function table. uint32_t sig_index; // index into the signature table. WireBytesRef code; // code of this function. - // Required number of slots in a feedback vector. Marked {mutable} because - // this is computed late (by Liftoff compilation), when the rest of the - // {WasmFunction} is typically considered {const}. - mutable int feedback_slots; bool imported; bool exported; bool declared; @@ -783,6 +779,9 @@ size_t PrintSignature(base::Vector buffer, const wasm::FunctionSig*, V8_EXPORT_PRIVATE size_t GetWireBytesHash(base::Vector wire_bytes); +// Get the required number of feedback slots for a function. +int NumFeedbackSlots(const WasmModule* module, int func_index); + } // namespace v8::internal::wasm #endif // V8_WASM_WASM_MODULE_H_ diff --git a/test/cctest/wasm/wasm-run-utils.cc b/test/cctest/wasm/wasm-run-utils.cc index 2380ff3140..6eb4b9cea3 100644 --- a/test/cctest/wasm/wasm-run-utils.cc +++ b/test/cctest/wasm/wasm-run-utils.cc @@ -158,7 +158,6 @@ uint32_t TestingModuleBuilder::AddFunction(const FunctionSig* sig, index, // func_index 0, // sig_index {0, 0}, // code - 0, // feedback slots false, // imported false, // exported false}); // declared diff --git a/test/common/wasm/wasm-interpreter.cc b/test/common/wasm/wasm-interpreter.cc index 98a68d6b9f..b58f1a6556 100644 --- a/test/common/wasm/wasm-interpreter.cc +++ b/test/common/wasm/wasm-interpreter.cc @@ -4240,7 +4240,6 @@ ControlTransferMap WasmInterpreter::ComputeControlTransfersForTesting( 0, // func_index 0, // sig_index {0, 0}, // code - 0, // feedback slots false, // imported false, // exported false}; // declared diff --git a/test/unittests/wasm/function-body-decoder-unittest.cc b/test/unittests/wasm/function-body-decoder-unittest.cc index 75756093a3..6079f735ea 100644 --- a/test/unittests/wasm/function-body-decoder-unittest.cc +++ b/test/unittests/wasm/function-body-decoder-unittest.cc @@ -183,7 +183,6 @@ class TestModuleBuilder { static_cast(mod.functions.size()), // func_index sig_index, // sig_index {0, 0}, // code - 0, // feedback slots false, // import false, // export declared}); // declared