[wasm] Refactor import handling for 0xC.

[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=

Committed: https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Committed: https://crrev.com/e97ca6ec479e3941dfbd9bdd36a637956dddbe00
Cr-Original-Commit-Position: refs/heads/master@{#40033}
Cr-Commit-Position: refs/heads/master@{#40050}

[titzer, 2016-10-04 15:56:41]

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

[commit-bot: I haz the power, 2016-10-04 15:56:47]

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-10-04 16:00:37]

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

[commit-bot: I haz the power, 2016-10-04 16:00:38]

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

[Mircea Trofin, 2016-10-04 20:06:45]

mtrofin@chromium.org changed reviewers:
+ mtrofin@chromium.org

[Mircea Trofin, 2016-10-04 20:06:46]

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

https://codereview.chromium.org/2390113003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1098: class WasmModuleInstantiateHelper {
How about "WasmInstantiationPipeline"?

[titzer, 2016-10-05 12:28:01]

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

[commit-bot: I haz the power, 2016-10-05 12:28:12]

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-10-05 13:17:10]

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

[commit-bot: I haz the power, 2016-10-05 13:17:11]

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

[titzer, 2016-10-05 15:47:33]

Description was changed from

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=
==========

to

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=
==========

[titzer, 2016-10-05 15:47:33]

titzer@chromium.org changed reviewers:
+ ahaas@chromium.org, bradnelson@chromium.org

[titzer, 2016-10-05 15:47:54]

On 2016/10/04 20:06:46, Mircea Trofin wrote:
> https://codereview.chromium.org/2390113003/diff/20001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2390113003/diff/20001/src/wasm/wasm-module.cc...
> src/wasm/wasm-module.cc:1098: class WasmModuleInstantiateHelper {
> How about "WasmInstantiationPipeline"?

Hmm, it's not really a pipeline, as it only does one thing.

[Mircea Trofin, 2016-10-05 16:21:09]

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

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:109: byte* raw_buffer_ptr(MaybeHandle<JSArrayBuffer>
buffer, int offset) {
The name strikes me as too generic. How about GetRawPointerToOffset?

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1005: WasmCompiledModule::New(isolate, min_mem_pages,
globals_size, origin);
Weird, this isn't a change. Git cl format did this?

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1141: class WasmModuleInstantiateHelper {
If not *Pipeline, how about WasmModuleInstanceBuilder?

I would love to avoid "Helper" or "Utility" names, they communicate very little
about the API.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1143: Isolate* isolate_;
since this is a class, could you make these private and add accessors? It would
also help with maintainability (I can more easily understand where things are
mutated by searching where set_blah_blah is called, rather than all uses of
blah_blah).

also, by the looks of it, a few things are read-only, e.g. isolate_, thrower_...
and a few things are built internally and only offered to the user to be read:
module_object_, memory_..

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1310: double num;
double num = 0.0

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1348: for (int index = 0; index < imports->length();
index++) {
++index - for consistency with the file

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1359: if (thrower_->error()) return 0;
should it return -1, because error?

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1370: return 0;
same here

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1396: return 0;
same here

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1417: for (int index = 0; index < inits->length();
index++) {
++index, for consistency

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1431: int size = 4;
what's "4"?

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1432: if (type == kLocalI64 || type == kLocalF64) size =
8;
what's "8"? 

could we set size to one of the sizeof(int32_t) or sizeof(double)... or don't we
have some MachineTypes as an alternative?

https://codereview.chromium.org/2390113003/diff/80001/test/mjsunit/wasm/wasm-...
File test/mjsunit/wasm/wasm-module-builder.js (right):

https://codereview.chromium.org/2390113003/diff/80001/test/mjsunit/wasm/wasm-...
test/mjsunit/wasm/wasm-module-builder.js:190: let o = {module: module, name:
name, kind: kExternalGlobal, type: type, mutable: false}
80 chars

[titzer, 2016-10-05 16:24:36]

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

[commit-bot: I haz the power, 2016-10-05 16:24:49]

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-10-05 16:50:02]

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

[commit-bot: I haz the power, 2016-10-05 16:50:03]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-05 18:29:39]

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

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:109: byte* raw_buffer_ptr(MaybeHandle<JSArrayBuffer>
buffer, int offset) {
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> The name strikes me as too generic. How about GetRawPointerToOffset?

Not sure that name is better, it's just longer.

Note that I went with hacker_style, since this is essentially a simple getter
with no side-effects.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1005: WasmCompiledModule::New(isolate, min_mem_pages,
globals_size, origin);
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> Weird, this isn't a change. Git cl format did this?

I removed a parameter that is no longer needed.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1141: class WasmModuleInstantiateHelper {
On 2016/10/05 16:21:08, Mircea Trofin wrote:
> If not *Pipeline, how about WasmModuleInstanceBuilder?
> 
> I would love to avoid "Helper" or "Utility" names, they communicate very
little
> about the API.

WasmInstanceBuilder FTW

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1143: Isolate* isolate_;
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> since this is a class, could you make these private and add accessors? It
would
> also help with maintainability (I can more easily understand where things are
> mutated by searching where set_blah_blah is called, rather than all uses of
> blah_blah).
> 
> also, by the looks of it, a few things are read-only, e.g. isolate_,
thrower_...
> and a few things are built internally and only offered to the user to be read:
> module_object_, memory_..

Since this is entirely contained in the CC file (and only a single method is
used outside the class), I don't see much value-add in public/private armor
here.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1310: double num;
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> double num = 0.0

Done.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1348: for (int index = 0; index < imports->length();
index++) {
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> ++index - for consistency with the file

Done.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1359: if (thrower_->error()) return 0;
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> should it return -1, because error?

Done.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1370: return 0;
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> same here

Done.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1396: return 0;
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> same here

Done.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1417: for (int index = 0; index < inits->length();
index++) {
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> ++index, for consistency

Done.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1431: int size = 4;
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> what's "4"?

Done.

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1432: if (type == kLocalI64 || type == kLocalF64) size =
8;
On 2016/10/05 16:21:09, Mircea Trofin wrote:
> what's "8"? 
> 
> could we set size to one of the sizeof(int32_t) or sizeof(double)... or don't
we
> have some MachineTypes as an alternative?

Done.

[Mircea Trofin, 2016-10-05 18:41:00]

lgtm assuming WasmInstanceBuilder's fields become private (fine if no accessors,
since they aren't even consumed out)

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

https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1143: Isolate* isolate_;
On 2016/10/05 18:29:39, titzer wrote:
> On 2016/10/05 16:21:09, Mircea Trofin wrote:
> > since this is a class, could you make these private and add accessors? It
> would
> > also help with maintainability (I can more easily understand where things
are
> > mutated by searching where set_blah_blah is called, rather than all uses of
> > blah_blah).
> > 
> > also, by the looks of it, a few things are read-only, e.g. isolate_,
> thrower_...
> > and a few things are built internally and only offered to the user to be
read:
> > module_object_, memory_..
> 
> Since this is entirely contained in the CC file (and only a single method is
> used outside the class), I don't see much value-add in public/private armor
> here.

Consistency with everywhere else in the codebase? Also, if you don't mean to
expose these properties out (appears not), there is benefit in hiding them and
not even exposing accessors, for encapsulation.

[titzer, 2016-10-06 08:37:32]

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

[commit-bot: I haz the power, 2016-10-06 08:37:39]

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

[titzer, 2016-10-06 08:43:00]

On 2016/10/05 18:41:00, Mircea Trofin wrote:
> lgtm assuming WasmInstanceBuilder's fields become private (fine if no
accessors,
> since they aren't even consumed out)
> 

Done.

> https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2390113003/diff/80001/src/wasm/wasm-module.cc...
> src/wasm/wasm-module.cc:1143: Isolate* isolate_;
> On 2016/10/05 18:29:39, titzer wrote:
> > On 2016/10/05 16:21:09, Mircea Trofin wrote:
> > > since this is a class, could you make these private and add accessors? It
> > would
> > > also help with maintainability (I can more easily understand where things
> are
> > > mutated by searching where set_blah_blah is called, rather than all uses
of
> > > blah_blah).
> > > 
> > > also, by the looks of it, a few things are read-only, e.g. isolate_,
> > thrower_...
> > > and a few things are built internally and only offered to the user to be
> read:
> > > module_object_, memory_..
> > 
> > Since this is entirely contained in the CC file (and only a single method is
> > used outside the class), I don't see much value-add in public/private armor
> > here.
> 
> Consistency with everywhere else in the codebase? Also, if you don't mean to
> expose these properties out (appears not), there is benefit in hiding them and
> not even exposing accessors, for encapsulation.

[commit-bot: I haz the power, 2016-10-06 09:05:19]

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

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

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-06 11:40:53]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-10-06 11:40:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org
Link to the patchset: https://codereview.chromium.org/2390113003/#ps160001
(title: "Address last review comments")

[commit-bot: I haz the power, 2016-10-06 11:41:04]

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-10-06 11:44:15]

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

[commit-bot: I haz the power, 2016-10-06 11:44:16]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/25785)

[titzer, 2016-10-06 11:52:21]

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

[commit-bot: I haz the power, 2016-10-06 11:52:24]

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-10-06 12:22:12]

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

[commit-bot: I haz the power, 2016-10-06 12:22:13]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-06 12:27:55]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-10-06 12:27:55]

The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org
Link to the patchset: https://codereview.chromium.org/2390113003/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-06 12:28:03]

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-10-06 12:30:40]

Description was changed from

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=
==========

to

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=
==========

[commit-bot: I haz the power, 2016-10-06 12:30:42]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-10-06 12:30:59]

Description was changed from

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=
==========

to

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=

Committed: https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Cr-Commit-Position: refs/heads/master@{#40033}
==========

[commit-bot: I haz the power, 2016-10-06 12:31:01]

Patchset 10 (id:??) landed as
https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Cr-Commit-Position: refs/heads/master@{#40033}

[Michael Hablich, 2016-10-06 13:42:34]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2395133002/ by hablich@chromium.org.

The reason for reverting is: Failes a few GC stress
tests.https://chromegw.corp.google.com/i/client.v8/builders/V8%20Linux%20-%20gc%20stress/builds/6253.

[titzer, 2016-10-06 14:48:59]

Description was changed from

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=

Committed: https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Cr-Commit-Position: refs/heads/master@{#40033}
==========

to

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=

Committed: https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Cr-Commit-Position: refs/heads/master@{#40033}
==========

[titzer, 2016-10-06 14:49:05]

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

[commit-bot: I haz the power, 2016-10-06 14:49:12]

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-10-06 15:21:22]

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

[commit-bot: I haz the power, 2016-10-06 15:21:23]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-06 15:40:21]

On 2016/10/06 13:42:34, Hablich wrote:
> A revert of this CL (patchset #10 id:180001) has been created in
> https://codereview.chromium.org/2395133002/ by mailto:hablich@chromium.org.
> 
> The reason for reverting is: Failes a few GC stress
>
tests.https://chromegw.corp.google.com/i/client.v8/builders/V8%20Linux%20-%20gc%20stress/builds/6253.

Corrected a GC bug, added a GC stress test. Relanding.

[titzer, 2016-10-06 15:40:29]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-10-06 15:40:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org
Link to the patchset: https://codereview.chromium.org/2390113003/#ps200001
(title: "Fix gc stress failure")

[commit-bot: I haz the power, 2016-10-06 15:40:33]

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-10-06 15:43:13]

Description was changed from

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=

Committed: https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Cr-Commit-Position: refs/heads/master@{#40033}
==========

to

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=

Committed: https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Cr-Commit-Position: refs/heads/master@{#40033}
==========

[commit-bot: I haz the power, 2016-10-06 15:43:14]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-06 15:43:32]

Description was changed from

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=

Committed: https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Cr-Commit-Position: refs/heads/master@{#40033}
==========

to

==========
[wasm] Refactor import handling for 0xC.

Imports and exports in 0xC can be much more than functions, including
tables, memories, and globals. This CL refactors the underlying
organization of imports and exports to support these new import types.

BUG=

Committed: https://crrev.com/599f8a83420346d9cba5ff97bd2a7520468207b6
Committed: https://crrev.com/e97ca6ec479e3941dfbd9bdd36a637956dddbe00
Cr-Original-Commit-Position: refs/heads/master@{#40033}
Cr-Commit-Position: refs/heads/master@{#40050}
==========

[commit-bot: I haz the power, 2016-10-06 15:43:34]

Patchset 11 (id:??) landed as
https://crrev.com/e97ca6ec479e3941dfbd9bdd36a637956dddbe00
Cr-Commit-Position: refs/heads/master@{#40050}