Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695813005:
[wasm] Split the compilation and instantiation API into sync and async methods. (Closed)
Description[wasm] Split the compilation and instantiation API into sync and async methods.
This makes it easier to implement asynchronous compilation by hiding all the implementation details of both synchronous and asynchronous compilation within wasm-module.cc, whereas before the code in wasm-js.cc actually implemented asynchronous compilation in terms of synchronous.
BUG=
Review-Url: https://codereview.chromium.org/2695813005
Cr-Commit-Position: refs/heads/master@{#43310}
Committed: https://chromium.googlesource.com/v8/v8/+/df834f3ff293d2c5e342335a71d17f0af6b0f648
Patch Set 1 #Patch Set 2 : Make it work #Patch Set 3 : Remove unused variable #Patch Set 4 : [wasm] Split the compilation and instantiation API into sync and async methods. #
Total comments: 65
Patch Set 5 : Address review comments #Patch Set 6 : Remove Handle and unused parameter. #Patch Set 7 : [wasm] Split the compilation and instantiation API into sync and async methods. #
Messages
Total messages: 43 (32 generated)
The CQ bit was checked by titzer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by titzer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by titzer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by titzer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [wasm] Split the compilation and instantiation API into sync and async methods. BUG= ========== to ========== [wasm] Split the compilation and instantiation API into sync and async methods. This makes it easier to implement asynchronous compilation by hiding all the implementation details of both synchronous and asynchronous compilation within wasm-module.cc, whereas before the code in wasm-js.cc actually implemented asynchronous compilation in terms of synchronous. BUG= ==========
titzer@chromium.org changed reviewers: + ahaas@chromium.org, clemensh@chromium.org
Nice refactoring, I like it! https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:40: return !has_brand.IsNothing() && has_brand.ToChecked(); return has_brand.FromMaybe(false); https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:61: return nothing; You really don't like "return {};"? What does {} represent if not "nothing"? ;) (also everywhere below) https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:67: i::Handle<i::Symbol>(i_context->wasm_module_sym()), this can we shortened to i::handle(i->context...) https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:177: HandleScope scope(isolate); What does this line do? It's also not clang-formatted BTW. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:205: // WebAssembly.Module.imports(module) -> Array<Export> exports, not imports. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:243: if (!thrower.error()) { "if (thrower.error()) return;" would be more consistent. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:264: if (!instance_object.is_null()) { if (thrower.error()) return; https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:272: // WebAssembly.Instance} That line break is annoying. Can you indent it a bit? https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:902: Isolate* isolate, WasmModule* m, ErrorThrower* thrower, 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 ¯\_(ツ)_/¯ https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1038: DCHECK(!compiled_module.is_null()); Seems unnecessary, compiled_module is unconditially assigned as WasmCompiledModule::New(...). https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1154: ffi_(ffi.is_null() ? Handle<JSReceiver>::null() 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? https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2781: DCHECK(!result.ok()); "DCHECK_IMPLIES(result.ok(), result.val);" before the if? https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2795: if (result.val) delete result.val; 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. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2839: v8::Utils::Convert<i::JSPromise, v8::Promise::Resolver>(promise); 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>(); https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2848: v8::Utils::Convert<i::JSPromise, v8::Promise::Resolver>(promise); Same here. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2855: ErrorThrower thrower(isolate, ""); Better pass null pointer here. Otherwise ": " will be prepended to each error: (in ErrorThrower::Format): 39 if (context_ != nullptr) { 40 str << context_ << ": "; 41 } https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2868: ErrorThrower thrower(isolate, ""); Same here. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2883: ErrorThrower thrower(isolate, ""); And here. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2901: ErrorThrower thrower(isolate, ""); And here. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:255: : handle_(Handle<Object>::null()), module_bytes_(module_bytes) {} This is default-initialized, you can skip it here. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:257: : handle_(Handle<Object>::null()), Same here. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:306: Handle<Object> handle_; This field seems to be unused. If I am wrong, can you then add a comment what this is and what it's used for? https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:484: void* unused, MaybeHandle<JSReceiver> imports); Wut? "void* unused"? Why is this needed? The compiler should be able to differentiate the two functions on the type of the third parameter.
https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:43: static bool BrandCheck(ErrorThrower* thrower, i::Handle<i::Object> value, nit: having thrower as the third parameter would make the code easier to read. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:122: if (!args[1]->IsObject()) { Independent question: why is this function called "IsObject" when what it actually does is "Utils::OpenHandle(this)->IsJSReceiver()". https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:131: void WebAssemblyCompile(const v8::FunctionCallbackInfo<v8::Value>& args) { I would prefer a function name which refers to the asynchronous nature of this function. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:174: void WebAssemblyModule(const v8::FunctionCallbackInfo<v8::Value>& args) { And a name which points to the synchronous nature here. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:200: if (thrower.error()) return; Is it okay that the actual error reason is lost here? Does it get lost? https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:213: if (thrower.error()) return; same here.
https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:902: Isolate* isolate, WasmModule* m, ErrorThrower* thrower, On 2017/02/16 at 20:36:57, Clemens Hammacher wrote: > 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 ¯\_(ツ)_/¯ 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? https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2781: DCHECK(!result.ok()); On 2017/02/16 at 20:36:57, Clemens Hammacher wrote: > "DCHECK_IMPLIES(result.ok(), result.val);" before the if? 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()); https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2795: if (result.val) delete result.val; On 2017/02/16 at 20:36:57, Clemens Hammacher wrote: > 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. I agree, storing a std::unique_ptr in the result would make sense. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2842: v8::Utils::ToLocal(thrower->Reify())); I think a DCHECK(thrower->error()); would be interesting here. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2855: ErrorThrower thrower(isolate, ""); Why does the thrower not get a proper name? https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2897: void wasm::AsyncCompileAndInstantiate(Isolate* isolate, Where is the compilation happening in this function? And why is the result different than the result of SyncInstantiate? https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:254: ModuleWireBytes(Vector<const byte> module_bytes) I think this constructor should use the explicit keyword, see https://google.github.io/styleguide/cppguide.html#Implicit_Conversions
https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:902: Isolate* isolate, WasmModule* m, ErrorThrower* thrower, On 2017/02/17 at 13:02:29, ahaas wrote: > On 2017/02/16 at 20:36:57, Clemens Hammacher wrote: > > 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 ¯\_(ツ)_/¯ > > 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? Exactly. The caller gives up ownership, and we either release the pointer, or it will be deleted automatically. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2781: DCHECK(!result.ok()); On 2017/02/17 at 13:02:29, ahaas wrote: > On 2017/02/16 at 20:36:57, Clemens Hammacher wrote: > > "DCHECK_IMPLIES(result.ok(), result.val);" before the if? > > 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()); It's semantically equivalent, and my version reads easier IMO ;)
https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:40: return !has_brand.IsNothing() && has_brand.ToChecked(); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > return has_brand.FromMaybe(false); Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:43: static bool BrandCheck(ErrorThrower* thrower, i::Handle<i::Object> value, On 2017/02/17 12:19:04, ahaas wrote: > nit: having thrower as the third parameter would make the code easier to read. Done. I thought we were more consistent about putting the ErrorThrower earlier in the parameter list, but alas, we aren't. I think this file is more the other way, so, suggestion accepted. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:61: return nothing; On 2017/02/16 20:36:57, Clemens Hammacher wrote: > You really don't like "return {};"? What does {} represent if not "nothing"? ;) > (also everywhere below) Done. Here and elsewhere in this file. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:67: i::Handle<i::Symbol>(i_context->wasm_module_sym()), On 2017/02/16 20:36:57, Clemens Hammacher wrote: > this can we shortened to i::handle(i->context...) Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:122: if (!args[1]->IsObject()) { On 2017/02/17 12:19:04, ahaas wrote: > Independent question: why is this function called "IsObject" when what it > actually does is "Utils::OpenHandle(this)->IsJSReceiver()". Don't have an answer to that :-/ https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:131: void WebAssemblyCompile(const v8::FunctionCallbackInfo<v8::Value>& args) { On 2017/02/17 12:19:04, ahaas wrote: > I would prefer a function name which refers to the asynchronous nature of this > function. The name here is chosen to reflect the JavaScript function name "WebAssembly.compile", as are the others. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:174: void WebAssemblyModule(const v8::FunctionCallbackInfo<v8::Value>& args) { On 2017/02/17 12:19:04, ahaas wrote: > And a name which points to the synchronous nature here. Same as above. It's named to reflect the JS API. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:177: HandleScope scope(isolate); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > What does this line do? It's also not clang-formatted BTW. Not sure which line you mean, as the lines seem pretty straightforward. One opens a HandleScope and the other creates an on-stack ErrorThrower. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:200: if (thrower.error()) return; On 2017/02/17 12:19:04, ahaas wrote: > Is it okay that the actual error reason is lost here? Does it get lost? The destructor of ErrorThrower will schedule it to be thrown in the isolate (see wasm-result.cc:93) https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:205: // WebAssembly.Module.imports(module) -> Array<Export> On 2017/02/16 20:36:57, Clemens Hammacher wrote: > exports, not imports. Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:213: if (thrower.error()) return; On 2017/02/17 12:19:04, ahaas wrote: > same here. Acknowledged. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:243: if (!thrower.error()) { On 2017/02/16 20:36:57, Clemens Hammacher wrote: > "if (thrower.error()) return;" would be more consistent. Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:264: if (!instance_object.is_null()) { On 2017/02/16 20:36:57, Clemens Hammacher wrote: > if (thrower.error()) return; Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:272: // WebAssembly.Instance} On 2017/02/16 20:36:57, Clemens Hammacher wrote: > That line break is annoying. Can you indent it a bit? Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:902: Isolate* isolate, WasmModule* m, ErrorThrower* thrower, On 2017/02/16 20:36:57, Clemens Hammacher wrote: > 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 ¯\_(ツ)_/¯ Erg, yeah, that would be bad, because the ownership actually transfers to the module_wrapper, which is a Managed thing. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1038: DCHECK(!compiled_module.is_null()); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > Seems unnecessary, compiled_module is unconditially assigned as > WasmCompiledModule::New(...). Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1154: ffi_(ffi.is_null() ? Handle<JSReceiver>::null() On 2017/02/16 20:36:57, Clemens Hammacher wrote: > 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? 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. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2781: DCHECK(!result.ok()); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > "DCHECK_IMPLIES(result.ok(), result.val);" before the if? 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. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2795: if (result.val) delete result.val; On 2017/02/16 20:36:57, Clemens Hammacher wrote: > 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. Added TODO. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2839: v8::Utils::Convert<i::JSPromise, v8::Promise::Resolver>(promise); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > 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>(); Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2848: v8::Utils::Convert<i::JSPromise, v8::Promise::Resolver>(promise); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > Same here. Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2855: ErrorThrower thrower(isolate, ""); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > Better pass null pointer here. Otherwise ": " will be prepended to each error: > (in ErrorThrower::Format): > 39 if (context_ != nullptr) { > > 40 str << context_ << ": "; > > 41 } > Good catch. Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2868: ErrorThrower thrower(isolate, ""); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > Same here. Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2883: ErrorThrower thrower(isolate, ""); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > And here. Done. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2897: void wasm::AsyncCompileAndInstantiate(Isolate* isolate, On 2017/02/17 13:02:29, ahaas wrote: > Where is the compilation happening in this function? And why is the result > different than the result of SyncInstantiate? 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. https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2901: ErrorThrower thrower(isolate, ""); On 2017/02/16 20:36:57, Clemens Hammacher wrote: > And here. Done.
The CQ bit was checked by titzer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by titzer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2695813005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:177: HandleScope scope(isolate); On 2017/02/17 at 13:17:53, titzer wrote: > On 2017/02/16 20:36:57, Clemens Hammacher wrote: > > What does this line do? It's also not clang-formatted BTW. > > Not sure which line you mean, as the lines seem pretty straightforward. One opens a HandleScope and the other creates an on-stack ErrorThrower. Never mind, Rietviel was missing some lines when showing the "context", so it seemed as if the Scope is left immediately after that line.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
The CQ bit was checked by titzer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by titzer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clemensh@chromium.org, ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2695813005/#ps120001 (title: "[wasm] Split the compilation and instantiation API into sync and async methods.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487587136437610,
"parent_rev": "db624fc4d7a210af88b32ecce46bc83699b96a5c", "commit_rev":
"df834f3ff293d2c5e342335a71d17f0af6b0f648"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Split the compilation and instantiation API into sync and async methods. This makes it easier to implement asynchronous compilation by hiding all the implementation details of both synchronous and asynchronous compilation within wasm-module.cc, whereas before the code in wasm-js.cc actually implemented asynchronous compilation in terms of synchronous. BUG= ========== to ========== [wasm] Split the compilation and instantiation API into sync and async methods. This makes it easier to implement asynchronous compilation by hiding all the implementation details of both synchronous and asynchronous compilation within wasm-module.cc, whereas before the code in wasm-js.cc actually implemented asynchronous compilation in terms of synchronous. BUG= Review-Url: https://codereview.chromium.org/2695813005 Cr-Commit-Position: refs/heads/master@{#43310} Committed: https://chromium.googlesource.com/v8/v8/+/df834f3ff293d2c5e342335a71d17f0af6b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/df834f3ff293d2c5e342335a71d17f0af6b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
