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

src/wasm/wasm-js.cc

Index: src/wasm/wasm-js.cc
diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc
index 36ffed223da101b0ba8cd44bc8b63c1f837f69ea..8c0024762d1478fbe17c5f10b0768e0b5d38083d 100644
--- a/src/wasm/wasm-js.cc
+++ b/src/wasm/wasm-js.cc
@@ -34,13 +34,11 @@ 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) {
@@ -229,13 +227,15 @@ void WebAssemblyInstance(const v8::FunctionCallbackInfo<v8::Value>& args) {
   }
 
   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 =
@@ -244,6 +244,16 @@ void WebAssemblyInstance(const v8::FunctionCallbackInfo<v8::Value>& args) {
     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()));
@@ -552,31 +562,15 @@ void WebAssemblyMemoryGrow(const v8::FunctionCallbackInfo<v8::Value>& args) {
 
   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);
 }
@@ -920,11 +914,42 @@ void WasmJs::SetWasmMemoryInstance(Isolate* isolate,
                                    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