[wasm] Store function names in the wasm object

[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

Tests will follow in https://codereview.chromium.org/1916403002.

R=titzer@chromium.org, mstarzinger@chromium.org, machenbach@chromium.org

Committed: https://crrev.com/9fd2650ae0c7bf01b381d485997f632740ecc887
Cr-Commit-Position: refs/heads/master@{#35905}

[Clemens Hammacher, 2016-04-22 11:07:01]

Description was changed from

==========
[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

R=titzer@chromium.org
==========

to

==========
[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

R=titzer@chromium.org, mstarzinger@chromium.org
==========

[Clemens Hammacher, 2016-04-22 11:07:01]

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

[Michael Starzinger, 2016-04-22 11:59:35]

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:1320: Factory* factory = info->isolate()->factory();
Can we move all of this into the WASM compiler?

https://codereview.chromium.org/1912103002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1912103002/diff/20001/src/objects.h#newcode438
src/objects.h:438: V(JS_WASM_TYPE)                                              
\
As discussed offline: Let's try to get by without adding an instance type for
the WASM module.

[titzer, 2016-04-22 13:21:08]

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.h
File src/compiler/pipeline.h (right):

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.h...
src/compiler/pipeline.h:41: Handle<Object> wasm_obj = Handle<Object>(), uint32_t
func_index = 0);
Let's not have defaults for these last parameters.

https://codereview.chromium.org/1912103002/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1912103002/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:3903: void ByteArray::get(int index, byte* buffer, int length)
{
This shouldn't be called "get", but more like "copy_out".

https://codereview.chromium.org/1912103002/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:3903: void ByteArray::get(int index, byte* buffer, int length)
{
Let's move this out of line into the .cc file; it's not performance critical.

https://codereview.chromium.org/1912103002/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:3921: void ByteArray::set(int index, const byte* buffer, int
length) {
This shouldn't be called "set", but more like "copy_in"

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

https://codereview.chromium.org/1912103002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:253: // The entries 1 to N contain offsets into the
seconds part of this array.
s/seconds/second/

https://codereview.chromium.org/1912103002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:261: Handle<Object> BuildFunctionNamesArray(Isolate*
isolate, WasmModule* module) {
Can we factor out the encoding and decoding logic for wasm names into a separate
file with its own unittests?

https://codereview.chromium.org/1912103002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:275: for (int func_index = 0; func_index !=
num_funcs_int; ++func_index) {
Would prefer a C++ foreach-loop with a separate index variable, if possible.

[Clemens Hammacher, 2016-04-22 14:38:40]

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:1320: Factory* factory = info->isolate()->factory();
On 2016/04/22 at 11:59:35, Michael Starzinger wrote:
> Can we move all of this into the WASM compiler?

Yes we can :)
It also saves us the check whether we have a wasm_obj, since this is distinct
for each caller.

But now, GenerateWASMCode is identical to GenerateCodeForTesting again (except
for the schedule parameter, which is always NULL for wasm).
Should be keep it anyway?

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.h
File src/compiler/pipeline.h (right):

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.h...
src/compiler/pipeline.h:41: Handle<Object> wasm_obj = Handle<Object>(), uint32_t
func_index = 0);
On 2016/04/22 at 13:21:07, titzer wrote:
> Let's not have defaults for these last parameters.

OK. They were only used from the tests anyway.

https://codereview.chromium.org/1912103002/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1912103002/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:3903: void ByteArray::get(int index, byte* buffer, int length)
{
On 2016/04/22 at 13:21:08, titzer wrote:
> Let's move this out of line into the .cc file; it's not performance critical.

It's not so easy to move it out, since it uses the macros only defined in
objects-inl.h. But they are not bigger than the surrounding methods, so it
shouldn't be a real problem either.

> This shouldn't be called "get", but more like "copy_out".

Renamed them to copy_in and copy_out.

https://codereview.chromium.org/1912103002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1912103002/diff/20001/src/objects.h#newcode438
src/objects.h:438: V(JS_WASM_TYPE)                                              
\
On 2016/04/22 at 11:59:35, Michael Starzinger wrote:
> As discussed offline: Let's try to get by without adding an instance type for
the WASM module.

Definitely! :)

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

https://codereview.chromium.org/1912103002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:261: Handle<Object> BuildFunctionNamesArray(Isolate*
isolate, WasmModule* module) {
On 2016/04/22 at 13:21:08, titzer wrote:
> Can we factor out the encoding and decoding logic for wasm names into a
separate file with its own unittests?

OK, will work on that on Monday and then update this CL.

https://codereview.chromium.org/1912103002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:275: for (int func_index = 0; func_index !=
num_funcs_int; ++func_index) {
On 2016/04/22 at 13:21:08, titzer wrote:
> Would prefer a C++ foreach-loop with a separate index variable, if possible.

C++ foreach does not work, since we increase func_index further in the function
body. This allows us to only iterate the functions once.

[titzer, 2016-04-25 11:15:05]

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1912103002/diff/20001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:1320: Factory* factory = info->isolate()->factory();
On 2016/04/22 14:38:40, Clemens Hammacher wrote:
> On 2016/04/22 at 11:59:35, Michael Starzinger wrote:
> > Can we move all of this into the WASM compiler?
> 
> Yes we can :)
> It also saves us the check whether we have a wasm_obj, since this is distinct
> for each caller.
> 
> But now, GenerateWASMCode is identical to GenerateCodeForTesting again (except
> for the schedule parameter, which is always NULL for wasm).
> Should be keep it anyway?

Yes, let's keep it to convey the intent, and they may diverge again in the
future. If you want to factor out a common helper routine, that's fine.

[Clemens Hammacher, 2016-04-26 12:45:03]

Description was changed from

==========
[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

R=titzer@chromium.org, mstarzinger@chromium.org
==========

to

==========
[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

R=titzer@chromium.org, mstarzinger@chromium.org, machenbach@chromium.org
==========

[Clemens Hammacher, 2016-04-26 12:45:03]

clemensh@chromium.org changed reviewers:
+ machenbach@chromium.org

[Clemens Hammacher, 2016-04-26 12:46:33]

Everything changed as recommended.

+machenbach for the change in tools/gyp/v8.gyp.

PTAL.

[Michael Achenbach, 2016-04-26 12:48:27]

lgtm on gyp

[Michael Starzinger, 2016-04-26 13:03:19]

LGTM on the plumbing outside of WASM from my end.

[Clemens Hammacher, 2016-04-26 13:16:11]

Description was changed from

==========
[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

R=titzer@chromium.org, mstarzinger@chromium.org, machenbach@chromium.org
==========

to

==========
[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

Tests will follow in https://codereview.chromium.org/1916403002.

R=titzer@chromium.org, mstarzinger@chromium.org, machenbach@chromium.org
==========

[Clemens Hammacher, 2016-04-28 14:41:35]

@titzer: LGTY now?

[titzer, 2016-04-29 11:46:17]

On 2016/04/28 14:41:35, Clemens Hammacher wrote:
> @titzer: LGTY now?

lgtm

[Clemens Hammacher, 2016-04-29 12:12:54]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-04-29 12:12:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, machenbach@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/1912103002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-29 12:13:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1912103002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1912103002/80001

[commit-bot: I haz the power, 2016-04-29 12:25:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-01 22:03:47]

Description was changed from

==========
[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

Tests will follow in https://codereview.chromium.org/1916403002.

R=titzer@chromium.org, mstarzinger@chromium.org, machenbach@chromium.org
==========

to

==========
[wasm] Store function names in the wasm object

We now store the wasm object and the function index in the
deoptimization data of the wasm Code object, and store an array with
function names in the wasm object.

This will make both the wasm module as well as the respective function
name available from the code object.

Tests will follow in https://codereview.chromium.org/1916403002.

R=titzer@chromium.org, mstarzinger@chromium.org, machenbach@chromium.org

Committed: https://crrev.com/9fd2650ae0c7bf01b381d485997f632740ecc887
Cr-Commit-Position: refs/heads/master@{#35905}
==========

[commit-bot: I haz the power, 2016-05-01 22:03:48]

Patchset 5 (id:??) landed as
https://crrev.com/9fd2650ae0c7bf01b381d485997f632740ecc887
Cr-Commit-Position: refs/heads/master@{#35905}