Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2717973003:
[wasm] Exit loop once wasm code in JS_TO_WASM is found (Closed)
|
Created:
3 years, 9 months ago by Clemens Hammacher Modified:
3 years, 9 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Exit loop once wasm code in JS_TO_WASM is found
This is a minor performance optimization. Instead of iterating the
relocation information till the end, we exit the loop once we found the
call to wasm code.
R=titzer@chromium.org, ahaas@chromium.org
Review-Url: https://codereview.chromium.org/2717973003
Cr-Commit-Position: refs/heads/master@{#43534}
Committed: https://chromium.googlesource.com/v8/v8/+/9fd418b96a67f3c83e36dcc0fa274ab71d21bf56
Patch Set 1 #Patch Set 2 : Minor fix #
Total comments: 6
Patch Set 3 : Address comments #
Dependent Patchsets: Messages
Total messages: 26 (17 generated)
The CQ bit was checked by clemensh@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_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
The CQ bit was checked by clemensh@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.
Ping
https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:863: Code* UnwrapImportWrapper(Object* target) { any reason for dropping the static qualifier? https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:863: Code* UnwrapImportWrapper(Object* target) { I think it'd be better if you left this as using handles.
https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:863: Code* UnwrapImportWrapper(Object* target) { On 2017/02/28 at 15:07:31, titzer wrote: > any reason for dropping the static qualifier? No, this is a mistake. This CL (https://codereview.chromium.org/2714373003) will wrap it in an anonymous namespace, but without this, we should re-add the static. > I think it'd be better if you left this as using handles. Hm, any reason for this? I know that the performance impact is minimal, but it exists (handle(obj) also needs to gather the isolate first by obj->GetIsolate()). Handles generally make code less readable IMO.
lgtm once Ben's comments are resolved. https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:872: target->kind() != Code::WASM_TO_JS_FUNCTION) What is the reason why code->builtin_index() != Builtins::kIllegal is not included here?
On 2017/02/28 15:14:52, Clemens Hammacher wrote: > https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc > File src/wasm/wasm-module.cc (right): > > https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc... > src/wasm/wasm-module.cc:863: Code* UnwrapImportWrapper(Object* target) { > On 2017/02/28 at 15:07:31, titzer wrote: > > any reason for dropping the static qualifier? > > No, this is a mistake. This CL (https://codereview.chromium.org/2714373003) will > wrap it in an anonymous namespace, but without this, we should re-add the > static. > > > I think it'd be better if you left this as using handles. > > Hm, any reason for this? I know that the performance impact is minimal, but it > exists (handle(obj) also needs to gather the isolate first by > obj->GetIsolate()). > Handles generally make code less readable IMO. It's more future-proof and we could police things better, e.g. if it were to ever end up running concurrently. There's a bit of a back story here, too. V8 struggled for a long time with a lot of unhandlified code, and we had a major fixit to get rid of them. Unhandlified code tended to be brittle and prone to GC bugs. So, default to handles unless there is a really good reason for doing so. And, yeah, the ugliness is annoying :(
Description was changed from ========== [wasm] Exit loop once wasm code in JS_TO_WASM is found This is a minor performance optimization. Instead of iterating the relocation information till the end, we exit the loop once we found the call to wasm code. Also, unhandlify the UnwrapImportWrapper function. RelocIterator is not gc-safe anyway, and there is no allocation happening in that function. R=titzer@chromium.org, ahaas@chromium.org ========== to ========== [wasm] Exit loop once wasm code in JS_TO_WASM is found This is a minor performance optimization. Instead of iterating the relocation information till the end, we exit the loop once we found the call to wasm code. R=titzer@chromium.org, ahaas@chromium.org ==========
The CQ bit was checked by clemensh@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.
Thanks for clarifying. All done. https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:863: Code* UnwrapImportWrapper(Object* target) { On 2017/02/28 at 15:07:31, titzer wrote: > I think it'd be better if you left this as using handles. Done. https://codereview.chromium.org/2717973003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:872: target->kind() != Code::WASM_TO_JS_FUNCTION) On 2017/02/28 at 18:40:35, ahaas wrote: > What is the reason why code->builtin_index() != Builtins::kIllegal is not included here? The import wrapper we are unwrapping here belongs to another module, which must be fully compiled at that stage, so kIllegal cannot occur here. If it ever would, the DCHECK(!it.done()) would fail.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2717973003/#ps40001 (title: "Address comments")
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": 40001, "attempt_start_ts": 1488449965617770,
"parent_rev": "c5c570f0c0dd638b59406c0d1eae0bca96fd6f0c", "commit_rev":
"9fd418b96a67f3c83e36dcc0fa274ab71d21bf56"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Exit loop once wasm code in JS_TO_WASM is found This is a minor performance optimization. Instead of iterating the relocation information till the end, we exit the loop once we found the call to wasm code. R=titzer@chromium.org, ahaas@chromium.org ========== to ========== [wasm] Exit loop once wasm code in JS_TO_WASM is found This is a minor performance optimization. Instead of iterating the relocation information till the end, we exit the loop once we found the call to wasm code. R=titzer@chromium.org, ahaas@chromium.org Review-Url: https://codereview.chromium.org/2717973003 Cr-Commit-Position: refs/heads/master@{#43534} Committed: https://chromium.googlesource.com/v8/v8/+/9fd418b96a67f3c83e36dcc0fa274ab71d2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/9fd418b96a67f3c83e36dcc0fa274ab71d2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
