[wasm] Split the compilation and instantiation API into sync and async methods.

src/wasm/wasm-module.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 <memory>
 
 #include "src/assembler-inl.h"
 #include "src/base/adapters.h"
 #include "src/base/atomic-utils.h"
 #include "src/code-stubs.h"
@@ skipping 880 matching lines @@
 }
 
 int wasm::GetFunctionCodeOffset(Handle<WasmCompiledModule> compiled_module,
                                 int func_index) {
   return GetFunctionOffsetAndLength(compiled_module, func_index).first;
 }
 
 WasmModule::WasmModule(Zone* owned)
     : owned_zone(owned), pending_tasks(new base::Semaphore(0)) {}
 
-MaybeHandle<WasmCompiledModule> WasmModule::CompileFunctions(
-    Isolate* isolate, Handle<WasmModuleWrapper> module_wrapper,
-    ErrorThrower* thrower, const ModuleWireBytes& wire_bytes,
-    Handle<Script> asm_js_script,
-    Vector<const byte> asm_js_offset_table_bytes) const {
+MaybeHandle<WasmModuleObject> CompileToModuleObject(
+    Isolate* isolate, WasmModule* m, ErrorThrower* thrower,

[Clemens Hammacher, 2017/02/16 20:36:57]

The C++11 way to communicate that we take ownership of the WasmModule* is to
pass it as std::unique_ptr<WasmModule>. But I suppose we don't do this anywhere
else in the code base, so ¯\_(ツ)_/¯

[ahaas, 2017/02/17 13:02:29]

We do use unique_ptr. Do you mean we should use the unique_ptr here to document
the ownership, but use unique_ptr::release later in this function because the
object lifetime is handled by the WasmModuleWrapper?

[Clemens Hammacher, 2017/02/17 13:10:30]

Exactly. The caller gives up ownership, and we either release the pointer, or it
will be deleted automatically.

[titzer, 2017/02/17 13:17:54]

Erg, yeah, that would be bad, because the ownership actually transfers to the
module_wrapper, which is a Managed thing.

+    const ModuleWireBytes& wire_bytes, Handle<Script> asm_js_script,
+    Vector<const byte> asm_js_offset_table_bytes) {
   Factory* factory = isolate->factory();
-
-  MaybeHandle<WasmCompiledModule> nothing;
-
-  WasmInstance temp_instance(this);
+  MaybeHandle<WasmModuleObject> nothing;
+  // The {module_wrapper} will take ownership of the {WasmModule} object,
+  // and it will be destroyed when the GC reclaims the wrapper object.
+  Handle<WasmModuleWrapper> module_wrapper = WasmModuleWrapper::New(isolate, m);
+  WasmInstance temp_instance(m);
   temp_instance.context = isolate->native_context();
-  temp_instance.mem_size = WasmModule::kPageSize * min_mem_pages;
+  temp_instance.mem_size = WasmModule::kPageSize * m->min_mem_pages;
   temp_instance.mem_start = nullptr;
   temp_instance.globals_start = nullptr;
 
   // Initialize the indirect tables with placeholders.
-  int function_table_count = static_cast<int>(function_tables.size());
+  int function_table_count = static_cast<int>(m->function_tables.size());
   Handle<FixedArray> function_tables =
       factory->NewFixedArray(function_table_count, TENURED);
   Handle<FixedArray> signature_tables =
       factory->NewFixedArray(function_table_count, TENURED);
   for (int i = 0; i < function_table_count; ++i) {
     temp_instance.function_tables[i] = factory->NewFixedArray(1, TENURED);
     temp_instance.signature_tables[i] = factory->NewFixedArray(1, TENURED);
     function_tables->set(i, *temp_instance.function_tables[i]);
     signature_tables->set(i, *temp_instance.signature_tables[i]);
   }
 
   HistogramTimerScope wasm_compile_module_time_scope(
       isolate->counters()->wasm_compile_module_time());
 
-  ModuleBytesEnv module_env(this, &temp_instance, wire_bytes);
+  ModuleBytesEnv module_env(m, &temp_instance, wire_bytes);
 
   // The {code_table} array contains import wrappers and functions (which
   // are both included in {functions.size()}, and export wrappers.
   int code_table_size =
-      static_cast<int>(functions.size() + num_exported_functions);
+      static_cast<int>(m->functions.size() + m->num_exported_functions);
   Handle<FixedArray> code_table =
       factory->NewFixedArray(static_cast<int>(code_table_size), TENURED);
 
   // Initialize the code table with the illegal builtin. All call sites will be
   // patched at instantiation.
   Handle<Code> illegal_builtin = isolate->builtins()->Illegal();
-  for (uint32_t i = 0; i < functions.size(); ++i) {
+  for (uint32_t i = 0; i < m->functions.size(); ++i) {
     code_table->set(static_cast<int>(i), *illegal_builtin);
     temp_instance.function_code[i] = illegal_builtin;
   }
 
   isolate->counters()->wasm_functions_per_module()->AddSample(
-      static_cast<int>(functions.size()));
+      static_cast<int>(m->functions.size()));
   if (!FLAG_trace_wasm_decoder && FLAG_wasm_num_compilation_tasks != 0) {
     // Avoid a race condition by collecting results into a second vector.
     std::vector<Handle<Code>> results(temp_instance.function_code);
     CompileInParallel(isolate, &module_env, results, thrower);
     temp_instance.function_code.swap(results);
   } else {
     CompileSequentially(isolate, &module_env, temp_instance.function_code,
                         thrower);
   }
   if (thrower->error()) return nothing;
@@ skipping 34 matching lines @@
   // breakpoints on a (potentially empty) subset of the instances.
 
   Handle<WasmSharedModuleData> shared = WasmSharedModuleData::New(
       isolate, module_wrapper, Handle<SeqOneByteString>::cast(module_bytes),
       script, asm_js_offset_table);
 
   // Create the compiled module object, and populate with compiled functions
   // and information needed at instantiation time. This object needs to be
   // serializable. Instantiation may occur off a deserialized version of this
   // object.
-  Handle<WasmCompiledModule> ret = WasmCompiledModule::New(isolate, shared);
-  ret->set_num_imported_functions(num_imported_functions);
-  ret->set_code_table(code_table);
-  ret->set_min_mem_pages(min_mem_pages);
-  ret->set_max_mem_pages(max_mem_pages);
+  Handle<WasmCompiledModule> compiled_module =
+      WasmCompiledModule::New(isolate, shared);
+  compiled_module->set_num_imported_functions(m->num_imported_functions);
+  compiled_module->set_code_table(code_table);
+  compiled_module->set_min_mem_pages(m->min_mem_pages);
+  compiled_module->set_max_mem_pages(m->max_mem_pages);
   if (function_table_count > 0) {
-    ret->set_function_tables(function_tables);
-    ret->set_signature_tables(signature_tables);
-    ret->set_empty_function_tables(function_tables);
+    compiled_module->set_function_tables(function_tables);
+    compiled_module->set_signature_tables(signature_tables);
+    compiled_module->set_empty_function_tables(function_tables);
   }
 
   // If we created a wasm script, finish it now and make it public to the
   // debugger.
   if (asm_js_script.is_null()) {
-    script->set_wasm_compiled_module(*ret);
+    script->set_wasm_compiled_module(*compiled_module);
     isolate->debug()->OnAfterCompile(script);
   }
 
   // Compile JS->WASM wrappers for exported functions.
   int func_index = 0;
-  for (auto exp : export_table) {
+  for (auto exp : m->export_table) {
     if (exp.kind != kExternalFunction) continue;
     Handle<Code> wasm_code =
         code_table->GetValueChecked<Code>(isolate, exp.index);
     Handle<Code> wrapper_code =
-        compiler::CompileJSToWasmWrapper(isolate, this, wasm_code, exp.index);
-    int export_index = static_cast<int>(functions.size() + func_index);
+        compiler::CompileJSToWasmWrapper(isolate, m, wasm_code, exp.index);
+    int export_index = static_cast<int>(m->functions.size() + func_index);
     code_table->set(export_index, *wrapper_code);
     RecordStats(isolate, *wrapper_code);
     func_index++;
   }
 
-  return ret;
+  DCHECK(!compiled_module.is_null());

[Clemens Hammacher, 2017/02/16 20:36:57]

Seems unnecessary, compiled_module is unconditially assigned as
WasmCompiledModule::New(...).

[titzer, 2017/02/17 13:17:54]

Done.

+  return WasmModuleObject::New(isolate, compiled_module);
 }
 
 static WasmFunction* GetWasmFunctionForImportWrapper(Isolate* isolate,
                                                      Handle<Object> target) {
   if (target->IsJSFunction()) {
     Handle<JSFunction> func = Handle<JSFunction>::cast(target);
     if (func->code()->kind() == Code::JS_TO_WASM_FUNCTION) {
       auto exported = Handle<WasmExportedFunction>::cast(func);
       Handle<WasmInstanceObject> other_instance(exported->instance(), isolate);
       int func_index = exported->function_index();
@@ skipping 88 matching lines @@
   }
 }
 
 // A helper class to simplify instantiating a module from a compiled module.
 // It closes over the {Isolate}, the {ErrorThrower}, the {WasmCompiledModule},
 // etc.
 class WasmInstanceBuilder {
  public:
   WasmInstanceBuilder(Isolate* isolate, ErrorThrower* thrower,
                       Handle<WasmModuleObject> module_object,
-                      Handle<JSReceiver> ffi, Handle<JSArrayBuffer> memory)
+                      MaybeHandle<JSReceiver> ffi,
+                      MaybeHandle<JSArrayBuffer> memory)
       : isolate_(isolate),
         module_(module_object->compiled_module()->module()),
         thrower_(thrower),
         module_object_(module_object),
-        ffi_(ffi),
-        memory_(memory) {}
+        ffi_(ffi.is_null() ? Handle<JSReceiver>::null()

[Clemens Hammacher, 2017/02/16 20:36:57]

Maybe we should also store it as MaybeHandle. That communicates better that it
must be expected to be null.
On the other hand, the redundant check in ToHandleChecked() always bothered me.
Maybe we should add a ToHandleUnchecked method, that just DCHECKs?

[titzer, 2017/02/17 13:17:53]

The only reason I avoided doing that is the noise I anticipated with MaybeHandle
in the code below. We can do that in the future, though.

+                           : ffi.ToHandleChecked()),
+        memory_(memory.is_null() ? Handle<JSArrayBuffer>::null()
+                                 : memory.ToHandleChecked()) {}
 
   // Build an instance, in all of its glory.
   MaybeHandle<WasmInstanceObject> Build() {
     MaybeHandle<WasmInstanceObject> nothing;
 
     // Check that an imports argument was provided, if the module requires it.
     // No point in continuing otherwise.
     if (!module_->import_table.empty() && ffi_.is_null()) {
       thrower_->TypeError(
           "Imports argument must be present and must be an object");
@@ skipping 1073 matching lines @@
       if (!table_instance.table_object.is_null()) {
         // Add the new dispatch table to the WebAssembly.Table object.
         all_dispatch_tables = WasmTableObject::AddDispatchTable(
             isolate_, table_instance.table_object, instance, index,
             table_instance.function_table, table_instance.signature_table);
       }
     }
   }
 };
 
-// Instantiates a WASM module, creating a WebAssembly.Instance from a
-// WebAssembly.Module.
-MaybeHandle<WasmInstanceObject> WasmModule::Instantiate(
-    Isolate* isolate, ErrorThrower* thrower,
-    Handle<WasmModuleObject> wasm_module, Handle<JSReceiver> ffi,
-    Handle<JSArrayBuffer> memory) {
-  WasmInstanceBuilder builder(isolate, thrower, wasm_module, ffi, memory);
-  return builder.Build();
-}
-
 bool wasm::IsWasmInstance(Object* object) {
   return WasmInstanceObject::IsWasmInstanceObject(object);
 }
 
 Handle<Script> wasm::GetScript(Handle<JSObject> instance) {
   WasmCompiledModule* compiled_module =
       WasmInstanceObject::cast(*instance)->compiled_module();
   return handle(compiled_module->script());
 }
 
 bool wasm::IsWasmCodegenAllowed(Isolate* isolate, Handle<Context> context) {
   return isolate->allow_code_gen_callback() == nullptr ||
          isolate->allow_code_gen_callback()(v8::Utils::ToLocal(context));
 }
 
-// TODO(clemensh): origin can be inferred from asm_js_script; remove it.
-MaybeHandle<WasmModuleObject> wasm::CreateModuleObjectFromBytes(
-    Isolate* isolate, const byte* start, const byte* end, ErrorThrower* thrower,
-    ModuleOrigin origin, Handle<Script> asm_js_script,
-    Vector<const byte> asm_js_offset_table_bytes) {
-  MaybeHandle<WasmModuleObject> nothing;
-
-  if (origin != kAsmJsOrigin &&
-      !IsWasmCodegenAllowed(isolate, isolate->native_context())) {
-    thrower->CompileError("Wasm code generation disallowed in this context");
-    return nothing;
-  }
-
-  ModuleResult result = DecodeWasmModule(isolate, start, end, false, origin);
-  if (result.failed()) {
-    if (result.val) delete result.val;
-    thrower->CompileFailed("Wasm decoding failed", result);
-    return nothing;
-  }
-
-  // The {module_wrapper} will take ownership of the {WasmModule} object,
-  // and it will be destroyed when the GC reclaims the wrapper object.
-  Handle<WasmModuleWrapper> module_wrapper =
-      WasmModuleWrapper::New(isolate, const_cast<WasmModule*>(result.val));
-
-  // Compile the functions of the module, producing a compiled module.
-  MaybeHandle<WasmCompiledModule> maybe_compiled_module =
-      result.val->CompileFunctions(isolate, module_wrapper, thrower,
-                                   ModuleWireBytes(start, end), asm_js_script,
-                                   asm_js_offset_table_bytes);
-
-  if (maybe_compiled_module.is_null()) return nothing;
-
-  Handle<WasmCompiledModule> compiled_module =
-      maybe_compiled_module.ToHandleChecked();
-
-  return WasmModuleObject::New(isolate, compiled_module);
-}
-
-bool wasm::ValidateModuleBytes(Isolate* isolate, const byte* start,
-                               const byte* end, ErrorThrower* thrower,
-                               ModuleOrigin origin) {
-  ModuleResult result = DecodeWasmModule(isolate, start, end, true, origin);
-  if (result.val) {
-    delete result.val;
-  } else {
-    DCHECK(!result.ok());
-  }
-  return result.ok();
-}
-
 MaybeHandle<JSArrayBuffer> wasm::GetInstanceMemory(
     Isolate* isolate, Handle<WasmInstanceObject> object) {
   auto instance = Handle<WasmInstanceObject>::cast(object);
   if (instance->has_memory_buffer()) {
     return Handle<JSArrayBuffer>(instance->memory_buffer(), isolate);
   }
   return MaybeHandle<JSArrayBuffer>();
 }
 
 void SetInstanceMemory(Handle<WasmInstanceObject> instance,
@@ skipping 486 matching lines @@
   Handle<FixedArray> storage = factory->NewFixedArray(num_custom_sections);
   JSArray::SetContent(array_object, storage);
   array_object->set_length(Smi::FromInt(num_custom_sections));
 
   for (int i = 0; i < num_custom_sections; i++) {
     storage->set(i, *matching_sections[i]);
   }
 
   return array_object;
 }
+
+bool wasm::SyncValidate(Isolate* isolate, ErrorThrower* thrower,
+                        const ModuleWireBytes& bytes) {
+  if (bytes.start() == nullptr || bytes.length() == 0) return false;
+  ModuleResult result =
+      DecodeWasmModule(isolate, bytes.start(), bytes.end(), true, kWasmOrigin);
+  if (result.val) {
+    delete result.val;
+  } else {
+    DCHECK(!result.ok());

[Clemens Hammacher, 2017/02/16 20:36:57]

"DCHECK_IMPLIES(result.ok(), result.val);" before the if?

[ahaas, 2017/02/17 13:02:29]

The check here is the other way around: if !result.val, then !result.ok(), so it
would have to be a DCHECK_IMPLIES(!result.val, !result.ok());

[Clemens Hammacher, 2017/02/17 13:10:30]

It's semantically equivalent, and my version reads easier IMO ;)

[titzer, 2017/02/17 13:17:54]

Initially I added the DCHECK, but now I just removed it. The rationale is that
that is basically a dynamic check of the contract of DecodeWasmModule, which
shouldn't be put into all callers, because we might miss some callers, and if
the check fails, the bug is actually in DecodeWasmModule, not in the caller.

+  }
+  return result.ok();
+}
+
+MaybeHandle<WasmModuleObject> wasm::SyncCompileTranslatedAsmJs(
+    Isolate* isolate, ErrorThrower* thrower, const ModuleWireBytes& bytes,
+    Handle<Script> asm_js_script,
+    Vector<const byte> asm_js_offset_table_bytes) {
+  MaybeHandle<WasmModuleObject> nothing;
+
+  ModuleResult result = DecodeWasmModule(isolate, bytes.start(), bytes.end(),
+                                         false, kAsmJsOrigin);
+  if (result.failed()) {
+    if (result.val) delete result.val;

[Clemens Hammacher, 2017/02/16 20:36:57]

This pattern repeats several times in this file. We could make ModuleResult a
Result<std::unique_ptr<const WasmModule*>>, so if noone takes out the module,
it's deleted automatically.

[ahaas, 2017/02/17 13:02:29]

I agree, storing a std::unique_ptr in the result would make sense.

[titzer, 2017/02/17 13:17:53]

Added TODO.

+    thrower->CompileFailed("Wasm decoding failed", result);
+    return nothing;
+  }
+
+  return CompileToModuleObject(isolate, const_cast<WasmModule*>(result.val),
+                               thrower, bytes, asm_js_script,
+                               asm_js_offset_table_bytes);
+}
+
+MaybeHandle<WasmModuleObject> wasm::SyncCompile(Isolate* isolate,
+                                                ErrorThrower* thrower,
+                                                const ModuleWireBytes& bytes) {
+  MaybeHandle<WasmModuleObject> nothing;
+
+  if (!IsWasmCodegenAllowed(isolate, isolate->native_context())) {
+    thrower->CompileError("Wasm code generation disallowed in this context");
+    return nothing;
+  }
+
+  ModuleResult result =
+      DecodeWasmModule(isolate, bytes.start(), bytes.end(), false, kWasmOrigin);
+  if (result.failed()) {
+    if (result.val) delete result.val;
+    thrower->CompileFailed("Wasm decoding failed", result);
+    return nothing;
+  }
+
+  return CompileToModuleObject(isolate, const_cast<WasmModule*>(result.val),
+                               thrower, bytes, Handle<Script>(),
+                               Vector<const byte>());
+}
+
+MaybeHandle<WasmInstanceObject> wasm::SyncInstantiate(
+    Isolate* isolate, ErrorThrower* thrower,
+    Handle<WasmModuleObject> module_object, MaybeHandle<JSReceiver> imports,
+    MaybeHandle<JSArrayBuffer> memory) {
+  WasmInstanceBuilder builder(isolate, thrower, module_object, imports, memory);
+  return builder.Build();
+}
+
+void RejectPromise(Isolate* isolate, ErrorThrower* thrower,
+                   Handle<JSPromise> promise) {
+  v8::Local<v8::Promise::Resolver> resolver =
+      v8::Utils::Convert<i::JSPromise, v8::Promise::Resolver>(promise);

[Clemens Hammacher, 2017/02/16 20:36:57]

This is the only location outside of api.h that uses Utils::Convert directly.
I think you can replace it by
v8::Utils::PromiseToLocal(promise).As<v8::Promise::Resolver>();

[titzer, 2017/02/17 13:17:53]

Done.

+  Handle<Context> context(isolate->context(), isolate);
+  resolver->Reject(v8::Utils::ToLocal(context),
+                   v8::Utils::ToLocal(thrower->Reify()));

[ahaas, 2017/02/17 13:02:29]

I think a DCHECK(thrower->error()); would be interesting here.

+}
+
+void ResolvePromise(Isolate* isolate, Handle<JSPromise> promise,
+                    Handle<Object> result) {
+  v8::Local<v8::Promise::Resolver> resolver =
+      v8::Utils::Convert<i::JSPromise, v8::Promise::Resolver>(promise);

[Clemens Hammacher, 2017/02/16 20:36:57]

Same here.

[titzer, 2017/02/17 13:17:53]

Done.

+  Handle<Context> context(isolate->context(), isolate);
+  resolver->Resolve(v8::Utils::ToLocal(context), v8::Utils::ToLocal(result));
+}
+
+void wasm::AsyncCompile(Isolate* isolate, Handle<JSPromise> promise,
+                        const ModuleWireBytes& bytes) {
+  ErrorThrower thrower(isolate, "");

[Clemens Hammacher, 2017/02/16 20:36:57]

Better pass null pointer here. Otherwise ": " will be prepended to each error:
(in ErrorThrower::Format):
 39   if (context_ != nullptr) {                                                
    
 40     str << context_ << ": ";                                                
    
 41   }                                                                         
    

[ahaas, 2017/02/17 13:02:29]

Why does the thrower not get a proper name?

[titzer, 2017/02/17 13:17:53]

Good catch. Done.

+  MaybeHandle<WasmModuleObject> module_object =
+      SyncCompile(isolate, &thrower, bytes);
+  if (thrower.error()) {
+    RejectPromise(isolate, &thrower, promise);
+    return;
+  }
+  ResolvePromise(isolate, promise, module_object.ToHandleChecked());
+}
+
+void wasm::AsyncInstantiate(Isolate* isolate, Handle<JSPromise> promise,
+                            Handle<WasmModuleObject> module_object,
+                            MaybeHandle<JSReceiver> imports) {
+  ErrorThrower thrower(isolate, "");

[Clemens Hammacher, 2017/02/16 20:36:57]

Same here.

[titzer, 2017/02/17 13:17:53]

Done.

+  MaybeHandle<WasmInstanceObject> instance_object = SyncInstantiate(
+      isolate, &thrower, module_object, imports, Handle<JSArrayBuffer>::null());
+  if (thrower.error()) {
+    RejectPromise(isolate, &thrower, promise);
+    return;
+  }
+  ResolvePromise(isolate, promise, instance_object.ToHandleChecked());
+}
+
+void wasm::AsyncCompileAndInstantiate(Isolate* isolate,
+                                      Handle<JSPromise> promise,
+                                      const ModuleWireBytes& bytes,
+                                      void* unused,
+                                      MaybeHandle<JSReceiver> imports) {
+  ErrorThrower thrower(isolate, "");

[Clemens Hammacher, 2017/02/16 20:36:57]

And here.

[titzer, 2017/02/17 13:17:53]

Done.

+
+  // Compile the module.
+  MaybeHandle<WasmModuleObject> module_object =
+      SyncCompile(isolate, &thrower, bytes);
+  if (thrower.error()) {
+    RejectPromise(isolate, &thrower, promise);
+    return;
+  }
+
+  AsyncCompileAndInstantiate(isolate, promise, module_object.ToHandleChecked(),
+                             imports);
+}
+
+void wasm::AsyncCompileAndInstantiate(Isolate* isolate,

[ahaas, 2017/02/17 13:02:29]

Where is the compilation happening in this function? And why is the result
different than the result of SyncInstantiate?

[titzer, 2017/02/17 13:17:54]

Dude, you're absolutely right. This method was redundant in the API wasn't even
called from wasm-js.cc. I've just inlined it into the (one) caller in this file.

+                                      Handle<JSPromise> promise,
+                                      Handle<WasmModuleObject> module,
+                                      MaybeHandle<JSReceiver> imports) {
+  ErrorThrower thrower(isolate, "");

[Clemens Hammacher, 2017/02/16 20:36:57]

And here.

[titzer, 2017/02/17 13:17:54]

Done.

+
+  // Instantiate the module.
+  MaybeHandle<WasmInstanceObject> instance_object = SyncInstantiate(
+      isolate, &thrower, module, imports, Handle<JSArrayBuffer>::null());
+  if (thrower.error()) {
+    RejectPromise(isolate, &thrower, promise);
+    return;
+  }
+
+  Handle<JSFunction> object_function =
+      Handle<JSFunction>(isolate->native_context()->object_function(), isolate);
+  Handle<JSObject> ret =
+      isolate->factory()->NewJSObject(object_function, TENURED);
+  Handle<String> module_property_name =
+      isolate->factory()->InternalizeUtf8String("module");
+  Handle<String> instance_property_name =
+      isolate->factory()->InternalizeUtf8String("instance");
+  JSObject::AddProperty(ret, module_property_name, module, NONE);
+  JSObject::AddProperty(ret, instance_property_name,
+                        instance_object.ToHandleChecked(), NONE);
+
+  ResolvePromise(isolate, promise, ret);
+}