From b53141eca31d34d7def3e20074c4802258fe5d1e Mon Sep 17 00:00:00 2001 From: Clemens Hammacher Date: Fri, 14 Jul 2017 15:58:25 +0200 Subject: [PATCH] Reland "[wasm] Don't store global handles in the interpreter" This is a reland of 5648aad553e09144197fbb40cf9314d057bd52ba. Previous compile error should be fixed by disabling strict aliasing assumptions on gyp: https://chromium-review.googlesource.com/c/571806 Original change's description: > [wasm] Don't store global handles in the interpreter > > Storing global handles in the interpreter is dangerous, because the > global handles are strong roots into the heap. The interpreter itself is > referenced from the heap via a Managed. Hence the interpreter keeps the > instance alive, while the instance keeps the Managed alive. So the GC > will never collect them. > > This CL refactors this to only store the handle to the instance object > while executing in the interpreter, and clearing it when returning. > It also removes the cache of import wrappers, as it should not be > performance critical, but keeps lots of objects alive. If it turns out > to be performance critical, we will have to reintroduce such a cache > stored in the WasmDebugInfo object. > > R=titzer@chromium.org > CC=ahaas@chromium.org > > Bug: chromium:610330 > Change-Id: I54b489dadc16685887c0c1a98da6fd0df5ad7cbb > Reviewed-on: https://chromium-review.googlesource.com/567058 > Reviewed-by: Ben Titzer > Commit-Queue: Clemens Hammacher > Cr-Commit-Position: refs/heads/master@{#46629} TBR=titzer@chromium.org Bug: chromium:610330 Change-Id: Ic7836b1b1a044a89f2138f0c76f92acd3a1b2f2b Reviewed-on: https://chromium-review.googlesource.com/570578 Commit-Queue: Clemens Hammacher Reviewed-by: Clemens Hammacher Cr-Commit-Position: refs/heads/master@{#46679} --- src/wasm/wasm-debug.cc | 11 +- src/wasm/wasm-interpreter.cc | 139 +++++++++++-------------- src/wasm/wasm-interpreter.h | 20 ++-- src/wasm/wasm-objects.h | 10 +- test/cctest/wasm/wasm-run-utils.h | 2 + test/common/wasm/wasm-module-runner.cc | 1 + 6 files changed, 87 insertions(+), 96 deletions(-) diff --git a/src/wasm/wasm-debug.cc b/src/wasm/wasm-debug.cc index 390ff357da..1c69b4af77 100644 --- a/src/wasm/wasm-debug.cc +++ b/src/wasm/wasm-debug.cc @@ -144,9 +144,6 @@ class InterpreterHandle { WasmInstanceObject* instance = debug_info->wasm_instance(); - // Store a global handle to the wasm instance in the interpreter. - interpreter_.SetInstanceObject(instance); - // Set memory start pointer and size. instance_.mem_start = nullptr; instance_.mem_size = 0; @@ -193,7 +190,8 @@ class InterpreterHandle { // Returns true if exited regularly, false if a trap/exception occured and was // not handled inside this activation. In the latter case, a pending exception // will have been set on the isolate. - bool Execute(Address frame_pointer, uint32_t func_index, + bool Execute(Handle instance_object, + Address frame_pointer, uint32_t func_index, uint8_t* arg_buffer) { DCHECK_GE(module()->functions.size(), func_index); FunctionSig* sig = module()->functions[func_index].sig; @@ -222,6 +220,8 @@ class InterpreterHandle { uint32_t activation_id = StartActivation(frame_pointer); + WasmInterpreter::HeapObjectsScope heap_objects_scope(&interpreter_, + instance_object); WasmInterpreter::Thread* thread = interpreter_.GetThread(0); thread->InitFrame(&module()->functions[func_index], wasm_args.start()); bool finished = false; @@ -705,8 +705,9 @@ void WasmDebugInfo::PrepareStep(StepAction step_action) { bool WasmDebugInfo::RunInterpreter(Address frame_pointer, int func_index, uint8_t* arg_buffer) { DCHECK_LE(0, func_index); + Handle instance(wasm_instance()); return GetInterpreterHandle(this)->Execute( - frame_pointer, static_cast(func_index), arg_buffer); + instance, frame_pointer, static_cast(func_index), arg_buffer); } std::vector> WasmDebugInfo::GetInterpretedStack( diff --git a/src/wasm/wasm-interpreter.cc b/src/wasm/wasm-interpreter.cc index 2199dda8cf..20e34c4be9 100644 --- a/src/wasm/wasm-interpreter.cc +++ b/src/wasm/wasm-interpreter.cc @@ -936,21 +936,14 @@ class CodeMap { Zone* zone_; const WasmModule* module_; ZoneVector interpreter_code_; - // Global handle to the wasm instance. + // This handle is set and reset by the SetInstanceObject() / + // ClearInstanceObject() method, which is used by the HeapObjectsScope. Handle instance_; - // Global handle to array of unwrapped imports. - Handle imported_functions_; - // Map from WASM_TO_JS wrappers to unwrapped imports (indexes into - // imported_functions_). - IdentityMap unwrapped_imports_; public: CodeMap(Isolate* isolate, const WasmModule* module, const uint8_t* module_start, Zone* zone) - : zone_(zone), - module_(module), - interpreter_code_(zone), - unwrapped_imports_(isolate->heap(), ZoneAllocationPolicy(zone)) { + : zone_(zone), module_(module), interpreter_code_(zone) { if (module == nullptr) return; interpreter_code_.reserve(module->functions.size()); for (const WasmFunction& function : module->functions) { @@ -964,37 +957,28 @@ class CodeMap { } } - ~CodeMap() { - // Destroy the global handles. - // Cast the location, not the handle, because the handle cast might access - // the object behind the handle. - GlobalHandles::Destroy(reinterpret_cast(instance_.location())); - GlobalHandles::Destroy( - reinterpret_cast(imported_functions_.location())); + void SetInstanceObject(Handle instance) { + DCHECK(instance_.is_null()); + instance_ = instance; } + void ClearInstanceObject() { instance_ = Handle::null(); } + const WasmModule* module() const { return module_; } bool has_instance() const { return !instance_.is_null(); } - Handle instance() const { + WasmInstanceObject* instance() const { DCHECK(has_instance()); - return instance_; + return *instance_; } MaybeHandle maybe_instance() const { - return has_instance() ? instance_ : MaybeHandle(); - } - - void SetInstanceObject(WasmInstanceObject* instance) { - // Only set the instance once (otherwise we have to destroy the global - // handle first). - DCHECK(instance_.is_null()); - DCHECK_EQ(instance->module(), module_); - instance_ = instance->GetIsolate()->global_handles()->Create(instance); + return has_instance() ? handle(instance()) + : MaybeHandle(); } Code* GetImportedFunction(uint32_t function_index) { - DCHECK(!instance_.is_null()); + DCHECK(has_instance()); DCHECK_GT(module_->num_imported_functions, function_index); - FixedArray* code_table = instance_->compiled_module()->ptr_to_code_table(); + FixedArray* code_table = instance()->compiled_module()->ptr_to_code_table(); return Code::cast(code_table->get(static_cast(function_index))); } @@ -1051,48 +1035,6 @@ class CodeMap { code->side_table = nullptr; Preprocess(code); } - - // Returns a callable object if the imported function has a JS-compatible - // signature, or a null handle otherwise. - Handle GetCallableObjectForJSImport(Isolate* isolate, - Handle code) { - DCHECK_EQ(Code::WASM_TO_JS_FUNCTION, code->kind()); - int* unwrapped_index = unwrapped_imports_.Find(code); - if (unwrapped_index) { - return handle( - HeapObject::cast(imported_functions_->get(*unwrapped_index)), - isolate); - } - Handle called_obj = UnwrapWasmToJSWrapper(isolate, code); - if (!called_obj.is_null()) { - // Cache the unwrapped callable object. - if (imported_functions_.is_null()) { - // This is the first call to an imported function. Allocate the - // FixedArray to cache unwrapped objects. - constexpr int kInitialCacheSize = 8; - Handle new_imported_functions = - isolate->factory()->NewFixedArray(kInitialCacheSize, TENURED); - // First entry: Number of occupied slots. - new_imported_functions->set(0, Smi::kZero); - imported_functions_ = - isolate->global_handles()->Create(*new_imported_functions); - } - int this_idx = Smi::ToInt(imported_functions_->get(0)) + 1; - if (this_idx == imported_functions_->length()) { - Handle new_imported_functions = - isolate->factory()->CopyFixedArrayAndGrow(imported_functions_, - this_idx / 2, TENURED); - // Update the existing global handle: - *imported_functions_.location() = *new_imported_functions; - } - DCHECK_GT(imported_functions_->length(), this_idx); - DCHECK(imported_functions_->get(this_idx)->IsUndefined(isolate)); - imported_functions_->set(0, Smi::FromInt(this_idx)); - imported_functions_->set(this_idx, *called_obj); - unwrapped_imports_.Set(code, this_idx); - } - return called_obj; - } }; Handle WasmValToNumber(Factory* factory, WasmVal val, @@ -2140,7 +2082,7 @@ class ThreadImpl { DCHECK_EQ(2, deopt_data->length()); WasmInstanceObject* target_instance = WasmInstanceObject::cast(WeakCell::cast(deopt_data->get(0))->value()); - if (target_instance != *codemap()->instance()) { + if (target_instance != codemap()->instance()) { // TODO(wasm): Implement calling functions of other instances/modules. UNIMPLEMENTED(); } @@ -2150,8 +2092,7 @@ class ThreadImpl { codemap()->GetCode(target_func_idx)}; } - Handle target = - codemap()->GetCallableObjectForJSImport(isolate, code); + Handle target = UnwrapWasmToJSWrapper(isolate, code); if (target.is_null()) { isolate->Throw(*isolate->factory()->NewTypeError( @@ -2372,6 +2313,37 @@ const InterpretedFrameImpl* ToImpl(const InterpretedFrame* frame) { return reinterpret_cast(frame); } +//============================================================================ +// Implementation details of the heap objects scope. +//============================================================================ +class HeapObjectsScopeImpl { + public: + HeapObjectsScopeImpl(CodeMap* codemap, Handle instance) + : codemap_(codemap), needs_reset(!codemap_->has_instance()) { + if (needs_reset) { + instance_ = handle(*instance); + codemap_->SetInstanceObject(instance_); + } else { + DCHECK_EQ(*instance, codemap_->instance()); + return; + } + } + + ~HeapObjectsScopeImpl() { + if (!needs_reset) return; + DCHECK_EQ(*instance_, codemap_->instance()); + codemap_->ClearInstanceObject(); + // Clear the handle, such that anyone who accidentally copied them will + // notice. + *instance_.location() = nullptr; + } + + private: + CodeMap* codemap_; + Handle instance_; + bool needs_reset; +}; + } // namespace //============================================================================ @@ -2512,10 +2484,6 @@ bool WasmInterpreter::SetTracing(const WasmFunction* function, bool enabled) { return false; } -void WasmInterpreter::SetInstanceObject(WasmInstanceObject* instance) { - internals_->codemap_.SetInstanceObject(instance); -} - int WasmInterpreter::GetThreadCount() { return 1; // only one thread for now. } @@ -2590,6 +2558,19 @@ WasmVal InterpretedFrame::GetStackValue(int index) const { return ToImpl(this)->GetStackValue(index); } +//============================================================================ +// Public API of the heap objects scope. +//============================================================================ +WasmInterpreter::HeapObjectsScope::HeapObjectsScope( + WasmInterpreter* interpreter, Handle instance) { + static_assert(sizeof(data) == sizeof(HeapObjectsScopeImpl), "Size mismatch"); + new (data) HeapObjectsScopeImpl(&interpreter->internals_->codemap_, instance); +} + +WasmInterpreter::HeapObjectsScope::~HeapObjectsScope() { + reinterpret_cast(data)->~HeapObjectsScopeImpl(); +} + } // namespace wasm } // namespace internal } // namespace v8 diff --git a/src/wasm/wasm-interpreter.h b/src/wasm/wasm-interpreter.h index a4a9320c3a..c11ebb2ecd 100644 --- a/src/wasm/wasm-interpreter.h +++ b/src/wasm/wasm-interpreter.h @@ -140,6 +140,19 @@ class InterpretedFrame { // An interpreter capable of executing WebAssembly. class V8_EXPORT_PRIVATE WasmInterpreter { public: + // Open a HeapObjectsScope before running any code in the interpreter which + // needs access to the instance object or needs to call to JS functions. + class V8_EXPORT_PRIVATE HeapObjectsScope { + public: + HeapObjectsScope(WasmInterpreter* interpreter, + Handle instance); + ~HeapObjectsScope(); + + private: + char data[3 * sizeof(void*)]; // must match sizeof(HeapObjectsScopeImpl). + DISALLOW_COPY_AND_ASSIGN(HeapObjectsScope); + }; + // State machine for a Thread: // +---------Run()/Step()--------+ // V | @@ -236,13 +249,6 @@ class V8_EXPORT_PRIVATE WasmInterpreter { // Enable or disable tracing for {function}. Return the previous state. bool SetTracing(const WasmFunction* function, bool enabled); - // Set the associated wasm instance object. - // If the instance object has been set, some tables stored inside it are used - // instead of the tables stored in the WasmModule struct. This allows to call - // back and forth between the interpreter and outside code (JS or wasm - // compiled) without repeatedly copying information. - void SetInstanceObject(WasmInstanceObject*); - //========================================================================== // Thread iteration and inspection. //========================================================================== diff --git a/src/wasm/wasm-objects.h b/src/wasm/wasm-objects.h index 81de738508..b7c92dfd56 100644 --- a/src/wasm/wasm-objects.h +++ b/src/wasm/wasm-objects.h @@ -592,11 +592,11 @@ class WasmDebugInfo : public FixedArray { DECL_GETTER(wasm_instance, WasmInstanceObject) DECL_OPTIONAL_ACCESSORS(locals_names, FixedArray) - enum { // -- - kInstanceIndex, - kInterpreterHandleIndex, - kInterpretedFunctionsIndex, - kLocalsNamesIndex, + enum { + kInstanceIndex, // instance object. + kInterpreterHandleIndex, // managed object containing the interpreter. + kInterpretedFunctionsIndex, // array of interpreter entry code objects. + kLocalsNamesIndex, // array of array of local names. kFieldCount }; diff --git a/test/cctest/wasm/wasm-run-utils.h b/test/cctest/wasm/wasm-run-utils.h index ccd6ba6584..a1aa4acf23 100644 --- a/test/cctest/wasm/wasm-run-utils.h +++ b/test/cctest/wasm/wasm-run-utils.h @@ -800,6 +800,8 @@ class WasmRunner : public WasmRunnerBase { thread->Reset(); std::array args{{WasmVal(p)...}}; thread->InitFrame(function(), args.data()); + WasmInterpreter::HeapObjectsScope heap_objects_scope( + interpreter(), module().instance_object()); if (thread->Run() == WasmInterpreter::FINISHED) { WasmVal val = thread->GetReturnValue(); possible_nondeterminism_ |= thread->PossibleNondeterminism(); diff --git a/test/common/wasm/wasm-module-runner.cc b/test/common/wasm/wasm-module-runner.cc index 2036ebf1e3..28ece8a9e7 100644 --- a/test/common/wasm/wasm-module-runner.cc +++ b/test/common/wasm/wasm-module-runner.cc @@ -93,6 +93,7 @@ int32_t InterpretWasmModule(Isolate* isolate, WasmInterpreter* interpreter = WasmDebugInfo::SetupForTesting(instance, nullptr); + WasmInterpreter::HeapObjectsScope heap_objects_scope(interpreter, instance); WasmInterpreter::Thread* thread = interpreter->GetThread(0); thread->Reset(); thread->InitFrame(&(instance->module()->functions[function_index]), args);