[wasm] Allocate a single script per wasm module

[wasm] Allocate a single script per wasm module

Before, we allocated one script per function per instance, and each
script referenced the wasm instance and the function index. Now we only
allocate one script per compiled wasm module, so the script also only
references this WasmCompiledModule, which causes changes to many interfaces.

Instead of fixing the disassemble API only used via debug.js, I decided
to drop it for now. Some later CL will reintroduce it via
DebugInterface.

BUG=v8:5530, chromium:659715
R=yangguo@chromium.org, titzer@chromium.org
CC=jgruber@chromium.org

Committed: https://crrev.com/32077e01fb7f98c8cd6d9f95abbedec6cdeda469
Cr-Commit-Position: refs/heads/master@{#41004}

[Clemens Hammacher, 2016-11-10 20:12:29]

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

[commit-bot: I haz the power, 2016-11-10 20:12:32]

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, 2016-11-10 20:17:27]

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

[commit-bot: I haz the power, 2016-11-10 20:17:28]

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

[Clemens Hammacher, 2016-11-10 21:09:11]

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

[commit-bot: I haz the power, 2016-11-10 21:09:21]

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, 2016-11-10 21:37:00]

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

[commit-bot: I haz the power, 2016-11-10 21:37:00]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-11-15 07:42:26]

lgtm, but I have some concerns. Also, I did not thoroughly review the wasm part.

https://codereview.chromium.org/2493823003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2493823003/diff/20001/src/objects.cc#newcode1...
src/objects.cc:13426: if (type() == Script::TYPE_WASM) {
InitLineEnds is called from a number of places, which then expect the line_ends
array to hold a FixedArray. Does that work correctly with wasm scripts?

By having the logic for position computation for wasm functions in a separate
place, we duplicate some code (the binary search below, for example). Would it
make sense to somehow unify the line_ends array with wasm's function offset
table?

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

https://codereview.chromium.org/2493823003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:770: // If the found entry does not contains the given
position, return false.
Can this actually happen, if we check upfront that position is less or equal the
last functions end offset? I'd rather do that and change this code to an
assertion.

[Clemens Hammacher, 2016-11-15 12:58:40]

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

[commit-bot: I haz the power, 2016-11-15 12:58:50]

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, 2016-11-15 13:00:20]

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

[commit-bot: I haz the power, 2016-11-15 13:00:21]

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/...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16200)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16166)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12519)

[Clemens Hammacher, 2016-11-15 15:03:06]

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

[commit-bot: I haz the power, 2016-11-15 15:03:17]

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

[Clemens Hammacher, 2016-11-15 15:04:06]

https://codereview.chromium.org/2493823003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2493823003/diff/20001/src/objects.cc#newcode1...
src/objects.cc:13426: if (type() == Script::TYPE_WASM) {
On 2016/11/15 at 07:42:25, Yang wrote:
> InitLineEnds is called from a number of places, which then expect the
line_ends array to hold a FixedArray. Does that work correctly with wasm
scripts?
> 
> By having the logic for position computation for wasm functions in a separate
place, we duplicate some code (the binary search below, for example). Would it
make sense to somehow unify the line_ends array with wasm's function offset
table?

I would like to keep wasm's function offset table "internal" to wasm, and not
expose it to the script. Generating a line_ends array and reusing the existing
logic is problematic, since it makes several assumptions, e.g. that function N
starts one character after the end of function N-1. We would also have to encode
a negative column offset of the script in order to account for the offset of
function 0.
So I think keeping the translation at wasm makes more sense.

With this CL
https://codereview.chromium.org/2451853002
all other usages of InitLineEnds are gone.
I added a DCHECK to InitLineEnds to ensure (and document) that it is never
called for wasm scripts.

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

https://codereview.chromium.org/2493823003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:770: // If the found entry does not contains the given
position, return false.
On 2016/11/15 at 07:42:26, Yang wrote:
> Can this actually happen, if we check upfront that position is less or equal
the last functions end offset? I'd rather do that and change this code to an
assertion.

There is a slight gap between functions, precisely the bytes used to encode the
length of the function code. For details:
https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#function-...

[titzer, 2016-11-15 15:27:36]

On 2016/11/15 15:04:06, Clemens Hammacher wrote:
> https://codereview.chromium.org/2493823003/diff/20001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/2493823003/diff/20001/src/objects.cc#newcode1...
> src/objects.cc:13426: if (type() == Script::TYPE_WASM) {
> On 2016/11/15 at 07:42:25, Yang wrote:
> > InitLineEnds is called from a number of places, which then expect the
> line_ends array to hold a FixedArray. Does that work correctly with wasm
> scripts?
> > 
> > By having the logic for position computation for wasm functions in a
separate
> place, we duplicate some code (the binary search below, for example). Would it
> make sense to somehow unify the line_ends array with wasm's function offset
> table?
> 
> I would like to keep wasm's function offset table "internal" to wasm, and not
> expose it to the script. Generating a line_ends array and reusing the existing
> logic is problematic, since it makes several assumptions, e.g. that function N
> starts one character after the end of function N-1. We would also have to
encode
> a negative column offset of the script in order to account for the offset of
> function 0.
> So I think keeping the translation at wasm makes more sense.
> 
> With this CL
> https://codereview.chromium.org/2451853002
> all other usages of InitLineEnds are gone.
> I added a DCHECK to InitLineEnds to ensure (and document) that it is never
> called for wasm scripts.
> 
> https://codereview.chromium.org/2493823003/diff/20001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2493823003/diff/20001/src/wasm/wasm-module.cc...
> src/wasm/wasm-module.cc:770: // If the found entry does not contains the given
> position, return false.
> On 2016/11/15 at 07:42:26, Yang wrote:
> > Can this actually happen, if we check upfront that position is less or equal
> the last functions end offset? I'd rather do that and change this code to an
> assertion.
> 
> There is a slight gap between functions, precisely the bytes used to encode
the
> length of the function code. For details:
>
https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#function-...

lgtm, i like it!

[commit-bot: I haz the power, 2016-11-15 15:29:54]

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

[commit-bot: I haz the power, 2016-11-15 15:29:55]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-11-15 17:00:21]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-11-15 17:00:22]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2493823003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-15 17:00:32]

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, 2016-11-15 17:05:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-17 22:34:44]

Description was changed from

==========
[wasm] Allocate a single script per wasm module

Before, we allocated one script per function per instance, and each
script referenced the wasm instance and the function index. Now we only
allocate one script per compiled wasm module, so the script also only
references this WasmCompiledModule, which causes changes to many interfaces.

Instead of fixing the disassemble API only used via debug.js, I decided
to drop it for now. Some later CL will reintroduce it via
DebugInterface.

BUG=v8:5530,chromium:659715
R=yangguo@chromium.org, titzer@chromium.org
CC=jgruber@chromium.org
==========

to

==========
[wasm] Allocate a single script per wasm module

Before, we allocated one script per function per instance, and each
script referenced the wasm instance and the function index. Now we only
allocate one script per compiled wasm module, so the script also only
references this WasmCompiledModule, which causes changes to many interfaces.

Instead of fixing the disassemble API only used via debug.js, I decided
to drop it for now. Some later CL will reintroduce it via
DebugInterface.

BUG=v8:5530,chromium:659715
R=yangguo@chromium.org, titzer@chromium.org
CC=jgruber@chromium.org

Committed: https://crrev.com/32077e01fb7f98c8cd6d9f95abbedec6cdeda469
Cr-Commit-Position: refs/heads/master@{#41004}
==========

[commit-bot: I haz the power, 2016-11-17 22:34:45]

Patchset 4 (id:??) landed as
https://crrev.com/32077e01fb7f98c8cd6d9f95abbedec6cdeda469
Cr-Commit-Position: refs/heads/master@{#41004}