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

src/wasm/wasm-module.cc

Index: src/wasm/wasm-module.cc
diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc
index 3021fb55b9b67d917ae25093b62f298302c93779..b167a37ef9a16488a141c1b7c80bd84cdd526217 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -898,23 +898,23 @@ int wasm::GetFunctionCodeOffset(Handle<WasmCompiledModule> compiled_module,
 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 =
@@ -929,25 +929,25 @@ MaybeHandle<WasmCompiledModule> WasmModule::CompileFunctions(
   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);
@@ -1002,39 +1002,41 @@ MaybeHandle<WasmCompiledModule> WasmModule::CompileFunctions(
   // 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,
@@ -1143,13 +1145,16 @@ 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() {
@@ -2243,16 +2248,6 @@ class WasmInstanceBuilder {
   }
 };
 
-// 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);
 }
@@ -2268,57 +2263,6 @@ bool wasm::IsWasmCodegenAllowed(Isolate* isolate, Handle<Context> context) {
          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);
@@ -2825,3 +2769,156 @@ Handle<JSArray> wasm::GetCustomSections(Isolate* isolate,
 
   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);
+}