[wasm] Acquire the context for TrapIf from a wasm frame

[wasm] Acquire the context for TrapIf from a wasm frame

This is the first step to reduce the size of the out-of-line code of
TrapIf. Instead of passing the context to the runtime call as a
parameter, we pass Smi::kZero to the runtime call and then get the
actual context from the WasmFrame on the stack.

BUG=v8:5908
R=titzer@chromium.org, clemensh@chromium.org

Review-Url: https://codereview.chromium.org/2664273002
Cr-Commit-Position: refs/heads/master@{#42853}
Committed: https://chromium.googlesource.com/v8/v8/+/87d309dadd062c1c4d279505aa5e36df12026016

[ahaas, 2017-01-31 19:06:33]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 19:06:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[titzer, 2017-01-31 19:16:07]

titzer@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[titzer, 2017-01-31 19:16:08]

lgtm, but might want to get opinion from mstarzinger@

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:62:
isolate->set_context(*WasmCompiledFrame::cast(it.frame())
We should be careful here. I assume that Throw() below will reset the context,
but I'm not entirely sure.

[Clemens Hammacher, 2017-01-31 19:20:34]

LGTM, this should already save us a big amount of code space!

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:61: DCHECK(it.frame()->is_wasm());
If this is just DCHECKs, we can actually skip them, because
StackFrameIterator::frame already DCHECKs !done(), and WasmCompiledFrame::cast
checks the frame type.

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:62:
isolate->set_context(*WasmCompiledFrame::cast(it.frame())
I am still not sure if it's really needed, but to be on the save side, we should
use a SaveContext here.

[commit-bot: I haz the power, 2017-01-31 19:33:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:33:41]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2017-02-01 11:36:53]

LGTM.

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:55: Object* ThrowRuntimeError(Isolate* isolate, int
message_id, int byte_offset,
We could add a DCHECK_NULL(isolate->context()) here to ensure callers adhere to
the convention. We do something similar in Runtime_NotifyDeoptimized already,
where we also call {Isolate::set_context} in a similar fashion.

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:62:
isolate->set_context(*WasmCompiledFrame::cast(it.frame())
On 2017/01/31 19:16:08, titzer wrote:
> We should be careful here. I assume that Throw() below will reset the context,
> but I'm not entirely sure.

AFAICT the {Throw} method itself should not change the current context. What
will change the current context though is the CEntryStub during the stack
unwinding after this function returns. This unwinding itself should not inspect
the current context. So at first glance this looks safe to me.

[ahaas, 2017-02-01 11:46:12]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 11:46:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2017-02-01 11:48:51]

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:55: Object* ThrowRuntimeError(Isolate* isolate, int
message_id, int byte_offset,
On 2017/02/01 at 11:36:52, Michael Starzinger wrote:
> We could add a DCHECK_NULL(isolate->context()) here to ensure callers adhere
to the convention. We do something similar in Runtime_NotifyDeoptimized already,
where we also call {Isolate::set_context} in a similar fashion.

Done.

https://codereview.chromium.org/2664273002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:61: DCHECK(it.frame()->is_wasm());
On 2017/01/31 at 19:20:33, Clemens Hammacher wrote:
> If this is just DCHECKs, we can actually skip them, because
StackFrameIterator::frame already DCHECKs !done(), and WasmCompiledFrame::cast
checks the frame type.

I removed the DCHECK(!it.done()); and added
CHECK(it.frame()->is_wasm_compiled());

[Clemens Hammacher, 2017-02-01 11:54:09]

lgtm

[commit-bot: I haz the power, 2017-02-01 12:02:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 12:02:10]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[ahaas, 2017-02-01 12:57:22]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2017-02-01 12:57:22]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2664273002/#ps20001
(title: "Add DCHECK_NULL(isolate->context());")

[commit-bot: I haz the power, 2017-02-01 12:57:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 13:22:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485953842305610,
"parent_rev": "58611d0115ff4031e26aefaf7f571f4b3a26cf07", "commit_rev":
"87d309dadd062c1c4d279505aa5e36df12026016"}

[commit-bot: I haz the power, 2017-02-01 13:23:03]

Description was changed from

==========
[wasm] Acquire the context for TrapIf from a wasm frame

This is the first step to reduce the size of the out-of-line code of
TrapIf. Instead of passing the context to the runtime call as a
parameter, we pass Smi::kZero to the runtime call and then get the
actual context from the WasmFrame on the stack.

BUG=v8:5908
R=titzer@chromium.org, clemensh@chromium.org
==========

to

==========
[wasm] Acquire the context for TrapIf from a wasm frame

This is the first step to reduce the size of the out-of-line code of
TrapIf. Instead of passing the context to the runtime call as a
parameter, we pass Smi::kZero to the runtime call and then get the
actual context from the WasmFrame on the stack.

BUG=v8:5908
R=titzer@chromium.org, clemensh@chromium.org

Review-Url: https://codereview.chromium.org/2664273002
Cr-Commit-Position: refs/heads/master@{#42853}
Committed:
https://chromium.googlesource.com/v8/v8/+/87d309dadd062c1c4d279505aa5e36df120...
==========

[commit-bot: I haz the power, 2017-02-01 13:23:04]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/87d309dadd062c1c4d279505aa5e36df120...