[wasm] Refactor call site patching

[wasm] Refactor call site patching

For debugging, we are patching call sites to not call other
WASM_FUNCTIONs, but call WASM_TO_INTERPRETER stubs instead. When later
re-instantiating / cloning this code, the old logic for patching call
sites would miss those calls.
This CL changes the way we patch callsites by getting the called
function index per callsite from the bytecode. This requires iterating
both the source position table and the relocation table at the same
time to determine the byte position for each call.
Instead of looking up the functions to be replaced in a std::map, we now
get the function directly from a FixedArray. This reduces the complexity
from O(n*n*log(n)) to O(m), where n is the total number of functions and
m is the total byte code length (note that each function is patched
individually, so we set up the map n times before).
Constant factor are unclear though.

BUG=v8:5822
R=titzer@chromium.org

Review-Url: https://codereview.chromium.org/2627613002
Cr-Commit-Position: refs/heads/master@{#42259}
Committed: https://chromium.googlesource.com/v8/v8/+/cfc2e5e180f416e24fcbf532f32ded47d38a49ff

[Clemens Hammacher, 2017-01-10 12:33:33]

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

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

Dry run: 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-01-10 13:27:53]

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

[commit-bot: I haz the power, 2017-01-10 13:27:54]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[Clemens Hammacher, 2017-01-10 14:25:42]

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

[commit-bot: I haz the power, 2017-01-10 14:25:45]

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

[Clemens Hammacher, 2017-01-10 14:27:52]

Description was changed from

==========
[wasm] Refactor call site patching

For debugging, we are patching call sites to not call other
WASM_FUNCTIONs, but call WASM_TO_INTERPRETER stubs instead. When later
re-instantiating / cloning this code, the old logic for patching call
sites would miss those calls.
This CL changes the way we patch callsites by getting the called
function index per callsite from the bytecode. This requires iterating
both the source position table and the relocation table at the same
time to determine the byte position for each call.
Instead of looking up the functions to be replaced in a std::map, we now
get the function directly from a FixedArray. This reduces the complexity
from O(n*n*log(n)) to O(m), where n is the total number of functions and
m is the total byte code length (note that each function is patched in
individually, so we set up the map n times before).
Constant factor are unclear though.

BUG=v8:5822
R=titzer@chromium.org
==========

to

==========
[wasm] Refactor call site patching

For debugging, we are patching call sites to not call other
WASM_FUNCTIONs, but call WASM_TO_INTERPRETER stubs instead. When later
re-instantiating / cloning this code, the old logic for patching call
sites would miss those calls.
This CL changes the way we patch callsites by getting the called
function index per callsite from the bytecode. This requires iterating
both the source position table and the relocation table at the same
time to determine the byte position for each call.
Instead of looking up the functions to be replaced in a std::map, we now
get the function directly from a FixedArray. This reduces the complexity
from O(n*n*log(n)) to O(m), where n is the total number of functions and
m is the total byte code length (note that each function is patched
individually, so we set up the map n times before).
Constant factor are unclear though.

BUG=v8:5822
R=titzer@chromium.org
==========

[commit-bot: I haz the power, 2017-01-10 14:54:41]

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

[commit-bot: I haz the power, 2017-01-10 14:54:42]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-11 18:02:48]

https://codereview.chromium.org/2627613002/diff/20001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2627613002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:449: int ExtractDirectCallIndex(const byte* pc) {
Can you use a decoder for this? That way we don't need to introduce another LEB
decoder function in leb-helper.h.

[Clemens Hammacher, 2017-01-11 18:15:55]

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

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

Dry run: 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-01-11 18:22:51]

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

[commit-bot: I haz the power, 2017-01-11 18:22:52]

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

[Clemens Hammacher, 2017-01-11 18:27:50]

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

[commit-bot: I haz the power, 2017-01-11 18:28:01]

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

[Clemens Hammacher, 2017-01-11 18:28:43]

https://codereview.chromium.org/2627613002/diff/20001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2627613002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:449: int ExtractDirectCallIndex(const byte* pc) {
On 2017/01/11 at 18:02:48, titzer wrote:
> Can you use a decoder for this? That way we don't need to introduce another
LEB decoder function in leb-helper.h.

Done.

FYI: I have to write it as wasm::Decoder, because arm64 defines another Decoder
somewhere, so it's ambiguous:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/buil...

[titzer, 2017-01-11 18:34:04]

lgtm if we add some more comments

https://codereview.chromium.org/2627613002/diff/60001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2627613002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:474: for (int i = start; i < num_wasm_functions; ++i) {
Can you add a comment here about what we are doing (i.e. we are iterating over
the source position table to find the source position, which allows us to decode
the bytecode to find the function index? It could be confusing for someone
stumbling across this code in the future.

[commit-bot: I haz the power, 2017-01-11 18:57:30]

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

[commit-bot: I haz the power, 2017-01-11 18:57:31]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-12 08:54:41]

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

[commit-bot: I haz the power, 2017-01-12 08:54:50]

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

[Clemens Hammacher, 2017-01-12 09:03:12]

https://codereview.chromium.org/2627613002/diff/60001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2627613002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:474: for (int i = start; i < num_wasm_functions; ++i) {
On 2017/01/11 at 18:34:04, titzer wrote:
> Can you add a comment here about what we are doing (i.e. we are iterating over
the source position table to find the source position, which allows us to decode
the bytecode to find the function index? It could be confusing for someone
stumbling across this code in the future.

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-12 09:38:10]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2017-01-12 09:38:11]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2627613002/#ps80001
(title: "Add comments")

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

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-01-12 09:40:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484213890835270,
"parent_rev": "31887804107bf5c103d915f5c601cfaaf1cd7cb6", "commit_rev":
"cfc2e5e180f416e24fcbf532f32ded47d38a49ff"}

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

Description was changed from

==========
[wasm] Refactor call site patching

For debugging, we are patching call sites to not call other
WASM_FUNCTIONs, but call WASM_TO_INTERPRETER stubs instead. When later
re-instantiating / cloning this code, the old logic for patching call
sites would miss those calls.
This CL changes the way we patch callsites by getting the called
function index per callsite from the bytecode. This requires iterating
both the source position table and the relocation table at the same
time to determine the byte position for each call.
Instead of looking up the functions to be replaced in a std::map, we now
get the function directly from a FixedArray. This reduces the complexity
from O(n*n*log(n)) to O(m), where n is the total number of functions and
m is the total byte code length (note that each function is patched
individually, so we set up the map n times before).
Constant factor are unclear though.

BUG=v8:5822
R=titzer@chromium.org
==========

to

==========
[wasm] Refactor call site patching

For debugging, we are patching call sites to not call other
WASM_FUNCTIONs, but call WASM_TO_INTERPRETER stubs instead. When later
re-instantiating / cloning this code, the old logic for patching call
sites would miss those calls.
This CL changes the way we patch callsites by getting the called
function index per callsite from the bytecode. This requires iterating
both the source position table and the relocation table at the same
time to determine the byte position for each call.
Instead of looking up the functions to be replaced in a std::map, we now
get the function directly from a FixedArray. This reduces the complexity
from O(n*n*log(n)) to O(m), where n is the total number of functions and
m is the total byte code length (note that each function is patched
individually, so we set up the map n times before).
Constant factor are unclear though.

BUG=v8:5822
R=titzer@chromium.org

Review-Url: https://codereview.chromium.org/2627613002
Cr-Commit-Position: refs/heads/master@{#42259}
Committed:
https://chromium.googlesource.com/v8/v8/+/cfc2e5e180f416e24fcbf532f32ded47d38...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/cfc2e5e180f416e24fcbf532f32ded47d38...