Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208703002:
[wasm] Allow import function to be any kind of callables. (Closed)
Description[wasm] Allow import function to be any kind of callables.
With this CL all kinds of Callable can imported into wasm. Please take a special look at the context that is used now in the WasmToJSWrapper.
BUG=633895
TEST=mjsunit/wasm/ffi.js
Committed: https://crrev.com/8d4910893cd1ffe2aa802e2079fb059b5d6ef1b9
Cr-Commit-Position: refs/heads/master@{#38569}
Patch Set 1 #Patch Set 2 : Create a native context at the beginning of all tests in test-run-wasm-js.cc #
Total comments: 8
Patch Set 3 : Comments. #
Total comments: 4
Patch Set 4 : Fix the problem in wasm-module.cc, and refactor the code in wasm-compiler.cc #
Total comments: 2
Patch Set 5 : Fix the receiver of native functions, and its test. #Patch Set 6 : Rebase. #
Messages
Total messages: 46 (29 generated)
The CQ bit was checked by ahaas@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...
Description was changed from ========== [wasm] Allow import function to be any kind of callables. BUG=633895 ========== to ========== [wasm] Allow import function to be any kind of callables. With this CL all kinds of Callable can imported into wasm. Please take a special look at the context that is used now in the WasmToJSWrapper. BUG=633895 TEST=mjsunit/wasm/ffi.js ==========
ahaas@chromium.org changed reviewers: + bmeurer@chromium.org, titzer@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2734: // Use the Call builtin. Always using the builtin does increase the overhead of calling WASM->JS, even in the case of an arity match. We actually started to measure that with one of the native benchmarks ported to WASM. Can you add a TODO to reintroduce the fast case?
On 2016/08/03 16:33:50, titzer wrote: > https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... > File src/compiler/wasm-compiler.cc (right): > > https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... > src/compiler/wasm-compiler.cc:2734: // Use the Call builtin. > Always using the builtin does increase the overhead of calling WASM->JS, even in > the case of an arity match. We actually started to measure that with one of the > native benchmarks ported to WASM. > > Can you add a TODO to reintroduce the fast case? lgtm otherwise
> src/compiler/wasm-compiler.cc:2734: // Use the Call builtin. > Always using the builtin does increase the overhead of calling WASM->JS, even in > the case of an arity match. We actually started to measure that with one of the > native benchmarks ported to WASM. > > Can you add a TODO to reintroduce the fast case? And add a comment to that TODO that the fast case was missing the class constructor and the native function checks.
LGTM once comments/nits addressed. https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2726: Node* context = HeapConstant(isolate->native_context()); Uhm, that looks weird. Are you really sure that the current context is the context you want here? Can we somehow guard this, i.e. add some DCHECKs that this is what you want? Because we had tons of bugs in the past where someone was incorrectly using the current context from the TLS instead of the correct context. https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2742: args[pos++] = jsgraph()->Constant(target); // JS function. Nit: Update comment. https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2754: args[pos++] = ToJS(param, context, sig->GetParam(i)); The ToJS should be (and seems to be) completely context independent; I'd suggest to remove the context parameter here because it's misleading and looks like a bug on first sight. https://codereview.chromium.org/2208703002/diff/20001/test/mjsunit/wasm/ffi.js File test/mjsunit/wasm/ffi.js (right): https://codereview.chromium.org/2208703002/diff/20001/test/mjsunit/wasm/ffi.j... test/mjsunit/wasm/ffi.js:59: testCallFFI(proxy_sub, check_FOREIGN_SUB); I'd like to see appropriate tests for all the special cases, i.e. bound functions, class constructors, special callables (propably requires a cctest in test-api.cc), and also native functions, i.e. our EcmaScript builtins making sure they get undefined for receiver and not some global proxy.
The CQ bit was checked by ahaas@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...
Benedikt, PTAL. I added tests for all kinds of callables you mentioned. Additionally I reintroduced the optimization for JSFunctions that I removed in the original CL. https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2726: Node* context = HeapConstant(isolate->native_context()); On 2016/08/03 at 17:14:43, Benedikt Meurer wrote: > Uhm, that looks weird. Are you really sure that the current context is the context you want here? Can we somehow guard this, i.e. add some DCHECKs that this is what you want? Because we had tons of bugs in the past where someone was incorrectly using the current context from the TLS instead of the correct context. Done. https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2734: // Use the Call builtin. On 2016/08/03 at 16:33:49, titzer wrote: > Always using the builtin does increase the overhead of calling WASM->JS, even in the case of an arity match. We actually started to measure that with one of the native benchmarks ported to WASM. > > Can you add a TODO to reintroduce the fast case? I reintroduced this optimization now for the case where target is a JSFunction. https://codereview.chromium.org/2208703002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2742: args[pos++] = jsgraph()->Constant(target); // JS function. On 2016/08/03 at 17:14:43, Benedikt Meurer wrote: > Nit: Update comment. Done.
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...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
mtrofin@chromium.org changed reviewers: + clarkchenwang@google.com, mtrofin@chromium.org
Please see comments. Also +clarkchenwang for the changes to wasm-module.cc (it's his elision code) https://codereview.chromium.org/2208703002/diff/40001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2208703002/diff/40001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2558: Handle<JSFunction> function; For readability/maintainability, could you perhaps pull in the if statement all the variables that depend on function being a valid handle, and assign them there? We reduce the risk of someone attempting to use function in incorrect cases; we also encapsulate the decisions about receiver's value and the heap constant that is to be used if (call_direct) in that one place. In fact, you can do away with having the call_direct bool altogether. https://codereview.chromium.org/2208703002/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2208703002/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:714: imports.push_back(code); why the 2 imports.push_back(code)?
The CQ bit was checked by ahaas@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/2208703002/diff/40001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2208703002/diff/40001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2558: Handle<JSFunction> function; On 2016/08/09 at 14:54:07, Mircea Trofin wrote: > For readability/maintainability, could you perhaps pull in the if statement all the variables that depend on function being a valid handle, and assign them there? We reduce the risk of someone attempting to use function in incorrect cases; we also encapsulate the decisions about receiver's value and the heap constant that is to be used if (call_direct) in that one place. > > In fact, you can do away with having the call_direct bool altogether. I refactored the code now and think that it's more readable than before. https://codereview.chromium.org/2208703002/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2208703002/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:714: imports.push_back(code); On 2016/08/09 at 14:54:07, Mircea Trofin wrote: > why the 2 imports.push_back(code)? Thanks for spotting this! I removed one push_back.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/09 18:00:09, ahaas wrote: > https://codereview.chromium.org/2208703002/diff/40001/src/compiler/wasm-compi... > File src/compiler/wasm-compiler.cc (right): > > https://codereview.chromium.org/2208703002/diff/40001/src/compiler/wasm-compi... > src/compiler/wasm-compiler.cc:2558: Handle<JSFunction> function; > On 2016/08/09 at 14:54:07, Mircea Trofin wrote: > > For readability/maintainability, could you perhaps pull in the if statement > all the variables that depend on function being a valid handle, and assign them > there? We reduce the risk of someone attempting to use function in incorrect > cases; we also encapsulate the decisions about receiver's value and the heap > constant that is to be used if (call_direct) in that one place. > > > > In fact, you can do away with having the call_direct bool altogether. > > I refactored the code now and think that it's more readable than before. > > https://codereview.chromium.org/2208703002/diff/40001/src/wasm/wasm-module.cc > File src/wasm/wasm-module.cc (right): > > https://codereview.chromium.org/2208703002/diff/40001/src/wasm/wasm-module.cc... > src/wasm/wasm-module.cc:714: imports.push_back(code); > On 2016/08/09 at 14:54:07, Mircea Trofin wrote: > > why the 2 imports.push_back(code)? > > Thanks for spotting this! I removed one push_back. Lgtm
https://codereview.chromium.org/2208703002/diff/60001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2208703002/diff/60001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2568: if (is_sloppy(function->shared()->language_mode())) { This is wrong. You also need to check for natives here. See JSTypedLowering::ReduceJSCallFunction for example.
The CQ bit was checked by ahaas@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/2208703002/diff/60001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2208703002/diff/60001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2568: if (is_sloppy(function->shared()->language_mode())) { On 2016/08/10 at 03:29:02, Benedikt Meurer wrote: > This is wrong. You also need to check for natives here. See JSTypedLowering::ReduceJSCallFunction for example. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM^4
The CQ bit was checked by ahaas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, mtrofin@chromium.org Link to the patchset: https://codereview.chromium.org/2208703002/#ps80001 (title: "Fix the receiver of native functions, and its test.")
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
Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/12308)
The CQ bit was checked by ahaas@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 ahaas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, titzer@chromium.org, mtrofin@chromium.org Link to the patchset: https://codereview.chromium.org/2208703002/#ps100001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Allow import function to be any kind of callables. With this CL all kinds of Callable can imported into wasm. Please take a special look at the context that is used now in the WasmToJSWrapper. BUG=633895 TEST=mjsunit/wasm/ffi.js ========== to ========== [wasm] Allow import function to be any kind of callables. With this CL all kinds of Callable can imported into wasm. Please take a special look at the context that is used now in the WasmToJSWrapper. BUG=633895 TEST=mjsunit/wasm/ffi.js Committed: https://crrev.com/8d4910893cd1ffe2aa802e2079fb059b5d6ef1b9 Cr-Commit-Position: refs/heads/master@{#38569} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8d4910893cd1ffe2aa802e2079fb059b5d6ef1b9 Cr-Commit-Position: refs/heads/master@{#38569} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
