[wasm] WebAssembly.Memory object can be referenced by multiple Instance objects.

src/wasm/wasm-js.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/api-natives.h"
 #include "src/api.h"
 #include "src/asmjs/asm-js.h"
 #include "src/asmjs/asm-typer.h"
 #include "src/asmjs/asm-wasm-builder.h"
 #include "src/assert-scope.h"
@@ skipping 16 matching lines @@
 
 namespace v8 {
 
 static const int kWasmTableArrayFieldIndex = 0;
 static const int kWasmTableMaximumFieldIndex = 1;
 static const int kWasmTableDispatchTablesFieldIndex = 2;
 
 enum WasmMemoryObjectData {
   kWasmMemoryBuffer,
   kWasmMemoryMaximum,
-  kWasmMemoryInstanceObject
+  kWasmMemoryInstancesLink,

[titzer, 2016/11/07 19:34:07]

I think this might be a bit simpler if we just used a regular FixedArray here
and only grow it when we run out of slots, instead of trying to manage a weak
list.

[gdeepti, 2016/11/09 01:26:17]

Do you mean using a FixedArray to store weak links to the instances, and grow
the array when we run out of slots for instances, i.e, check for empty slots
that could have been garbage collected and grow if needed? I'm not sure what the
optimal size of the array in this case would be. 

Experimenting with it a little bit, I've found the weak list to be a simpler
approach because of the book keeping overhead. I'm probably missing something
here - could you elaborate on why this would be better as a FixedArray?   

[bradnelson, 2016/11/09 01:40:11]

So this is currently keeping a doubly-linked (strong) chain of wrappers, with
only the link to the instance being weak. The instances keep a link to their
wrapper, so that deletion is direct.

Doing this with a FixedArray of weak references to the instances would mean more
bookkeeping around edge cases I would think?

+  kWasmMemoryInternalFieldCount
 };
 
-enum WasmInternalFieldCountData {
-  kWasmTableInternalFieldCount = 3,
-  kWasmMemoryInternalFieldCount = 3
-};
+enum WasmInternalFieldCountData { kWasmTableInternalFieldCount = 3 };
 
 namespace {
 i::Handle<i::String> v8_str(i::Isolate* isolate, const char* str) {
   return isolate->factory()->NewStringFromAsciiChecked(str);
 }
 Local<String> v8_str(Isolate* isolate, const char* str) {
   return Utils::ToLocal(v8_str(reinterpret_cast<i::Isolate*>(isolate), str));
 }
 
 struct RawBuffer {
@@ skipping 168 matching lines @@
   i::Handle<i::JSObject> i_obj =
       i::Handle<i::JSObject>::cast(v8::Utils::OpenHandle(*obj));
 
   i::Handle<i::JSReceiver> ffi = i::Handle<i::JSObject>::null();
   if (args.Length() > 1 && args[1]->IsObject()) {
     Local<Object> obj = Local<Object>::Cast(args[1]);
     ffi = i::Handle<i::JSReceiver>::cast(v8::Utils::OpenHandle(*obj));
   }
 
   i::Handle<i::JSArrayBuffer> memory = i::Handle<i::JSArrayBuffer>::null();
+  i::Handle<i::Object> mem_obj;
   if (args.Length() > 2 && args[2]->IsObject()) {
     Local<Object> obj = Local<Object>::Cast(args[2]);
-    i::Handle<i::Object> mem_obj = v8::Utils::OpenHandle(*obj);
+    mem_obj = v8::Utils::OpenHandle(*obj);
     if (i::WasmJs::IsWasmMemoryObject(i_isolate, mem_obj)) {
       memory = i::WasmJs::GetWasmMemoryArrayBuffer(i_isolate, mem_obj);
     } else {
       thrower.TypeError("Argument 2 must be a WebAssembly.Memory");
+      return;
     }
   }
   i::MaybeHandle<i::JSObject> instance =
       i::wasm::WasmModule::Instantiate(i_isolate, &thrower, i_obj, ffi, memory);
   if (instance.is_null()) {
     if (!thrower.error()) thrower.RuntimeError("Could not instantiate module");
     return;
   }
+
+  // TODO(gdeepti): Seems like the right place for this would be in instantiate
+  // flow, fix to use memory object as an external memory import when passed in
+  // as an argument.
+  if (args.Length() > 2 && args[2]->IsObject()) {

[Mircea Trofin, 2016/11/09 18:57:09]

there's another place we check if args.Length() > 2, etc, just above. Cache the
result? After that, you can probably avoid mem_obj being first declared and then
set.

[gdeepti, 2016/11/16 05:34:09]

This is no longer required after refactoring, removed.

+    i::WasmJs::SetWasmMemoryInstance(i_isolate, mem_obj,
+                                     instance.ToHandleChecked());
+    internal::wasm::SetInstanceMemoryObject(instance.ToHandleChecked(),
+                                            mem_obj);
+  }
   DCHECK(!i_isolate->has_pending_exception());
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();
   return_value.Set(Utils::ToLocal(instance.ToHandleChecked()));
 }
 
 bool GetIntegerProperty(v8::Isolate* isolate, ErrorThrower* thrower,
                         Local<Context> context, Local<v8::Object> object,
                         Local<String> property, int* result, int lower_bound,
                         int upper_bound) {
   v8::MaybeLocal<v8::Value> maybe = object->Get(context, property);
@@ skipping 288 matching lines @@
   }
   if (args.Length() < 1) {
     v8::Local<v8::Value> e = v8::Exception::TypeError(
         v8_str(isolate, "Argument 0 required, must be numeric value of pages"));
     isolate->ThrowException(e);
     return;
   }
 
   uint32_t delta = args[0]->Uint32Value(context).FromJust();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
-  i::Handle<i::JSObject> receiver =
-      i::Handle<i::JSObject>::cast(Utils::OpenHandle(*args.This()));
-  i::Handle<i::Object> instance_object(
-      receiver->GetInternalField(kWasmMemoryInstanceObject), i_isolate);
-  i::Handle<i::JSObject> instance(
-      i::Handle<i::JSObject>::cast(instance_object));
-
-  // TODO(gdeepti) Implement growing memory when shared by different
-  // instances.
-  int32_t ret = internal::wasm::GrowInstanceMemory(i_isolate, instance, delta);
+  i::Handle<i::Object> receiver =
+      i::Handle<i::Object>::cast(Utils::OpenHandle(*args.This()));
+  int32_t ret = i::wasm::GrowWebAssemblyMemory(i_isolate, receiver, delta);
   if (ret == -1) {
     v8::Local<v8::Value> e = v8::Exception::Error(
         v8_str(isolate, "Unable to grow instance memory."));
     isolate->ThrowException(e);
     return;
   }
-  i::MaybeHandle<i::JSArrayBuffer> buffer =
-      internal::wasm::GetInstanceMemory(i_isolate, instance);
-  if (buffer.is_null()) {
-    v8::Local<v8::Value> e = v8::Exception::Error(
-        v8_str(isolate, "WebAssembly.Memory buffer object not set."));
-    isolate->ThrowException(e);
-    return;
-  }
-  receiver->SetInternalField(kWasmMemoryBuffer, *buffer.ToHandleChecked());
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();
   return_value.Set(ret);
 }
 
 void WebAssemblyMemoryGetBuffer(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
   if (!BrandCheck(isolate, Utils::OpenHandle(*args.This()),
@@ skipping 323 matching lines @@
   if (max_mem->IsUndefined(isolate)) return wasm::WasmModule::kV8MaxPages;
   uint32_t max_pages = Smi::cast(max_mem)->value();
   return max_pages;
 }
 
 void WasmJs::SetWasmMemoryInstance(Isolate* isolate,
                                    Handle<Object> memory_object,
                                    Handle<JSObject> instance) {
   if (!memory_object->IsUndefined(isolate)) {
     DCHECK(IsWasmMemoryObject(isolate, memory_object));
-    // TODO(gdeepti): This should be a weak list of instance objects
-    // for instances that share memory.
+    Object* instance_link = JSObject::cast(*memory_object)
+                                ->GetInternalField(kWasmMemoryInstancesLink);
+    Handle<wasm::WasmInstanceWrapper> instance_wrapper;
+    if (instance_link->IsUndefined(isolate)) {
+      instance_wrapper = wasm::WasmInstanceWrapper::New(isolate, instance);
+    } else {
+      Handle<wasm::WasmInstanceWrapper> current_wrapper =
+          handle(wasm::WasmInstanceWrapper::cast(instance_link));
+      DCHECK(
+          wasm::WasmInstanceWrapper::IsWasmInstanceWrapper(*current_wrapper));
+      DCHECK(!current_wrapper->has_previous());
+      instance_wrapper = wasm::WasmInstanceWrapper::New(isolate, instance);
+      instance_wrapper->set_next_wrapper(*current_wrapper);
+      current_wrapper->set_previous_wrapper(*instance_wrapper);
+    }
     JSObject::cast(*memory_object)
-        ->SetInternalField(kWasmMemoryInstanceObject, *instance);
+        ->SetInternalField(kWasmMemoryInstancesLink, *instance_wrapper);
+    const int kWasmMemInstanceWrapper = 5;

[Mircea Trofin, 2016/11/09 18:57:09]

This is fragile, because this value here ("5") needs to be kept in sync with the
corresponding value on the wasm-module side of things. Better expose an API on
that side.

[gdeepti, 2016/11/16 05:34:09]

Done.

+    instance->SetInternalField(kWasmMemInstanceWrapper, *instance_wrapper);
   }
 }
+
+void WasmJs::ResetWasmMemoryInstance(Isolate* isolate,
+                                     Handle<Object> memory_object) {
+  Handle<Object> undefined = isolate->factory()->undefined_value();
+  JSObject::cast(*memory_object)
+      ->SetInternalField(kWasmMemoryInstancesLink, *undefined);
+}
+
+Handle<Object> WasmJs::GetWasmMemoryInstanceWrapper(Isolate* isolate,
+                                                    Handle<Object> value) {
+  DCHECK(IsWasmMemoryObject(isolate, value));
+  Handle<Object> obj(
+      JSObject::cast(*value)->GetInternalField(kWasmMemoryInstancesLink),
+      isolate);
+  return obj;
+}
 }  // namespace internal
 }  // namespace v8