Revert "[wasm] Don't store global handles in the interpreter"

This reverts commit 5648aad5.

Reason for revert: Compile error on mips:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder/builds/10732

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 <titzer@chromium.org>
> Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#46629}

TBR=titzer@chromium.org,clemensh@chromium.org

Change-Id: Ifadfb885f937f37bb3eab4732a97f20ff40c2583
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:610330
Reviewed-on: https://chromium-review.googlesource.com/569962Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46630}

src/wasm/wasm-debug.cc

@@ -144,6 +144,9 @@ 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;
@@ -190,8 +193,7 @@ 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(Handle<WasmInstanceObject> instance_object,
-               Address frame_pointer, uint32_t func_index,
+  bool Execute(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;
@@ -220,8 +222,6 @@ 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,9 +705,8 @@ 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<WasmInstanceObject> instance(wasm_instance());
   return GetInterpreterHandle(this)->Execute(
-      instance, frame_pointer, static_cast<uint32_t>(func_index), arg_buffer);
+      frame_pointer, static_cast<uint32_t>(func_index), arg_buffer);
 }
 
 std::vector<std::pair<uint32_t, int>> WasmDebugInfo::GetInterpretedStack(

src/wasm/wasm-interpreter.cc

@@ -936,14 +936,21 @@ class CodeMap {
   Zone* zone_;
   const WasmModule* module_;
   ZoneVector<InterpreterCode> interpreter_code_;
-  // This handle is set and reset by the SetInstanceObject() /
-  // ClearInstanceObject() method, which is used by the HeapObjectsScope.
+  // Global handle to the wasm instance.
   Handle<WasmInstanceObject> instance_;
+  // Global handle to array of unwrapped imports.
+  Handle<FixedArray> imported_functions_;
+  // Map from WASM_TO_JS wrappers to unwrapped imports (indexes into
+  // imported_functions_).
+  IdentityMap<int, ZoneAllocationPolicy> unwrapped_imports_;
 
  public:
   CodeMap(Isolate* isolate, const WasmModule* module,
           const uint8_t* module_start, Zone* zone)
-      : zone_(zone), module_(module), interpreter_code_(zone) {
+      : zone_(zone),
+        module_(module),
+        interpreter_code_(zone),
+        unwrapped_imports_(isolate->heap(), ZoneAllocationPolicy(zone)) {
     if (module == nullptr) return;
     interpreter_code_.reserve(module->functions.size());
     for (const WasmFunction& function : module->functions) {
@@ -957,28 +964,37 @@ class CodeMap {
     }
   }
 
-  void SetInstanceObject(Handle<WasmInstanceObject> instance) {
-    DCHECK(instance_.is_null());
-    instance_ = instance;
+  ~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<Object**>(instance_.location()));
+    GlobalHandles::Destroy(
+        reinterpret_cast<Object**>(imported_functions_.location()));
   }
 
-  void ClearInstanceObject() { instance_ = Handle<WasmInstanceObject>::null(); }
-
   const WasmModule* module() const { return module_; }
   bool has_instance() const { return !instance_.is_null(); }
-  WasmInstanceObject* instance() const {
+  Handle<WasmInstanceObject> instance() const {
     DCHECK(has_instance());
-    return *instance_;
+    return instance_;
   }
   MaybeHandle<WasmInstanceObject> maybe_instance() const {
-    return has_instance() ? handle(instance())
-                          : MaybeHandle<WasmInstanceObject>();
+    return has_instance() ? instance_ : MaybeHandle<WasmInstanceObject>();
+  }
+
+  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);
   }
 
   Code* GetImportedFunction(uint32_t function_index) {
-    DCHECK(has_instance());
+    DCHECK(!instance_.is_null());
     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<int>(function_index)));
   }
 
@@ -1035,6 +1051,48 @@ 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<HeapObject> GetCallableObjectForJSImport(Isolate* isolate,
+                                                  Handle<Code> 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<HeapObject> 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<FixedArray> 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<FixedArray> 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<Object> WasmValToNumber(Factory* factory, WasmVal val,
@@ -2082,7 +2140,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();
       }
@@ -2092,7 +2150,8 @@ class ThreadImpl {
               codemap()->GetCode(target_func_idx)};
     }
 
-    Handle<HeapObject> target = UnwrapWasmToJSWrapper(isolate, code);
+    Handle<HeapObject> target =
+        codemap()->GetCallableObjectForJSImport(isolate, code);
 
     if (target.is_null()) {
       isolate->Throw(*isolate->factory()->NewTypeError(
@@ -2313,37 +2372,6 @@ const InterpretedFrameImpl* ToImpl(const InterpretedFrame* frame) {
   return reinterpret_cast<const InterpretedFrameImpl*>(frame);
 }
 
-//============================================================================
-// Implementation details of the heap objects scope.
-//============================================================================
-class HeapObjectsScopeImpl {
- public:
-  HeapObjectsScopeImpl(CodeMap* codemap, Handle<WasmInstanceObject> 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<WasmInstanceObject> instance_;
-  bool needs_reset;
-};
-
 }  // namespace
 
 //============================================================================
@@ -2484,6 +2512,10 @@ 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.
 }
@@ -2558,19 +2590,6 @@ WasmVal InterpretedFrame::GetStackValue(int index) const {
   return ToImpl(this)->GetStackValue(index);
 }
 
-//============================================================================
-// Public API of the heap objects scope.
-//============================================================================
-WasmInterpreter::HeapObjectsScope::HeapObjectsScope(
-    WasmInterpreter* interpreter, Handle<WasmInstanceObject> instance) {
-  static_assert(sizeof(data) == sizeof(HeapObjectsScopeImpl), "Size mismatch");
-  new (data) HeapObjectsScopeImpl(&interpreter->internals_->codemap_, instance);
-}
-
-WasmInterpreter::HeapObjectsScope::~HeapObjectsScope() {
-  reinterpret_cast<HeapObjectsScopeImpl*>(data)->~HeapObjectsScopeImpl();
-}
-
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

src/wasm/wasm-interpreter.h

@@ -140,19 +140,6 @@ 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<WasmInstanceObject> 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                             |
@@ -249,6 +236,13 @@ 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.
   //==========================================================================

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,              // instance object.
-    kInterpreterHandleIndex,     // managed object containing the interpreter.
-    kInterpretedFunctionsIndex,  // array of interpreter entry code objects.
-    kLocalsNamesIndex,           // array of array of local names.
+  enum {  // --
+    kInstanceIndex,
+    kInterpreterHandleIndex,
+    kInterpretedFunctionsIndex,
+    kLocalsNamesIndex,
     kFieldCount
   };
 

test/cctest/wasm/wasm-run-utils.h

@@ -800,8 +800,6 @@ class WasmRunner : public WasmRunnerBase {
     thread->Reset();
     std::array<WasmVal, sizeof...(p)> 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();

test/common/wasm/wasm-module-runner.cc

@@ -93,7 +93,6 @@ 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);