[wasm] Use a Managed<WasmModule> to hold metadata about modules.

[wasm] Use a Managed<WasmModule> to hold metadata about modules.

This CL refactors the handling of metadata associated with WebAssembly
modules to reduce the duplicate marshalling of data from the C++ world
to the JavaScript world. It does this by wrapping the C++ WasmModule*
object in a Foreign that is rooted from the on-heap WasmCompiledModule
(which is itself just a FixedArray). Upon serialization, the C++ object
is ignored and the original WASM wire bytes are serialized. Upon
deserialization, the C++ object is reconstituted by reparsing the bytes.

This is motivated by increasing complications in implementing the JS
API, in particular WebAssembly.Table, which must perform signature
canonicalization across instances.

Additionally, this CL implements the proper base + offset initialization
behavior for tables.

R=rossberg@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org,yangguo@chromium.org
BUG=v8:5507, chromium:575167, chromium:657316
Committed: https://crrev.com/418b239f0b379cbf97bc74b93a31d61382a1ed13
Cr-Commit-Position: refs/heads/master@{#40434}

[titzer, 2016-10-15 15:30:25]

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

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

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-15 15:32:09]

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

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

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  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_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[Mircea Trofin, 2016-10-15 17:38:41]

I love the direction of this CL. 

My main high-level feedback/concerns:
- are there a few CLs in one here, can they be thus separated?
- see the comment on api.cc
- the comment about string interning in wasm-module.cc
- when does the Cpp WasmModule get deleted? (It may well be a property of
Foreign, but haven't seen that documented on Foreign itself. Maybe I missed
something, too)

https://codereview.chromium.org/2424623002/diff/1/src/api.cc
File src/api.cc (left):

https://codereview.chromium.org/2424623002/diff/1/src/api.cc#oldcode7195
src/api.cc:7195: compiled_part->reset_module_bytes();
Consider the scenario when we try to deserialize from an older version stored in
indexedDB, and need to recompile. To recompile, we need the wasm wire bytes.
Since deserialization won't even get past the serialized header, our only way to
get back to the wire bytes is to explicitly serialize them (as discussed, long
term, we envision they are stored separately as Blobs). 

This means the serialized payload is wire_bytes + serialized_module.

If we leave the wire bytes hanging off the serialized module, the serialized
payload is 2*wire_bytes + serialized_module.

Typically, the serialized module is about 4-5x larger than the wire bytes. So
this means the duplicate wire_bytes constitute a 16-20% increase in serialized
data.

To achieve the goal of this CL, we only need the header bytes. Is the plan to
follow-up with another CL, where that happens? If so, I think we should do that
before we ship, so we should add an issue#; also, are we OK with taking a hit in
user-observable properties in the meantime? (I realize the whole wasm feature is
under a flag, so I don't think this is much of an issue myself, just calling it
out to make sure we understand the trade-offs and are all OK with them).

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

https://codereview.chromium.org/2424623002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2160: Node* sig_match =
graph()->NewNode(machine->WordEqual(), load_sig,
Is this related, or can it be a separate CL?

https://codereview.chromium.org/2424623002/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2424623002/diff/1/src/isolate.cc#newcode512
src/isolate.cc:512: Handle<Object> wasm_module(wasm_frame->wasm_obj(), this);
Could the renames be a separate change?

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

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:80: return isolate->factory()->NewStringFromUtf8(vec);
Shouldn't we worry about GC happening when: allocating the memory for the new
string; then module_bytes being compacted; and then raw being invalid (and
therefore what vec holds on to), and then the copying ?

Also (this is after reading further down in the change), it looks like we're
using this to peel out strings from serialized data for things like imports (the
labels used internally by wasm functions), but we'd do that every time we create
a new instance. We could avoid that allocation, the strings are shared by all
module instances. We can either use one of the Internalize* String APIs (like
InternalizeUtf8String); that has some computation overhead; or we can take a
model complexity hit and hang them off the CompiledModule in a fixed array. I
realize this would go back a step (albeit small) from the reduction of
duplication goal of this CL.

Or - could the APIs consuming these strings not use strings? Don't we just do
lookups and other stuff like that?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:699: WasmModule* CppModule(Handle<JSObject> wasm_module)
{
GetCppModule? (as opposed to Create)

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:729: Handle<FixedArray> function_tables;
I thought we're supposed to use MaybeHandle whenever "null" was a valid value
for the variable.

Alternatively - since we're only using this further below, to set on the
returned value - could we just do it all there? (line 809)

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:815: for (auto exp : export_table) {
I know auto is allowed for iterators, but since we don't use exp as an iterator,
but rather as a ref to whatever export_table contains, mind typing it as such
(easier to read - don't need to jump to definition of export_table)

Also, it's a const, correct?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:823: func_index++;
++func_index;

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1049: for (int i = 0; i < new_table->length(); i++) {
++i

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1055: for (auto table_init : module_->table_inits) {
same comment about auto as above.

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1058: for (size_t i = 0; i < table_init.entries.size();
i++) {
++i

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1069: for (int i = 0; i < code_table->length(); i++) {
++i

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1072: for (int j = 0; j < function_table_count; j++) {
++j

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1161: WasmModule* module_;
elsewhere, there was a CppModule API. Could we call this cpp_module_ - or
perhaps let's punt this renaming until we agree on a nomenclature
(https://bugs.chromium.org/p/v8/issues/detail?id=5521)

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1276: Handle<ByteArray> exportedSig = Handle<ByteArray>(
do we still need this, given we now have the native signature available?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1326: *GetRawGlobalPtr<int32_t>(global, globals) =
static_cast<int32_t>(num);
Is this change here related, or could it be a different change?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1350: index++) {
++index

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1353: ExtractStringFromModuleBytes(isolate_,
compiled_module_,
ExtractString allocates each time we call it. Should we rather intern this
string?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1508: for (auto exp : module_->export_table) {
same comment about auto as above.

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1510: ExtractStringFromModuleBytes(isolate_,
compiled_module_,
same comment about string interning as above

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1990: Handle<FixedArray> array) {
why not pass this guy as a Handle<WasmCompiledModule> outright?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:2004: DecodeWasmModule(isolate, start, end, false,
kWasmOrigin);
Could you add a TODO to avoid decoding the entire module, since we only need the
headers?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:392: MACRO(OBJECT, Foreign, module_wrapper)             
\
I assume the pointer held by the Foreign is deleted when the Foreign object is
collected?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:395: MACRO(OBJECT, FixedArray, function_tables)         
\
Why drop the "indirect" prefix?

https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:502: bool IsWasmModule(Object* wasm_module);
I also think "WasmModule" is a better name than "WasmObject", however, our code
has 2 types of objects that are "WasmModules": an instance of the native
WasmModule struct; and an instance of WebAssembly.Module.

Separately, we have the compiled data, currently referred to
"WasmCompiledModule", which is also talking about "WasmModule".

(While we're at it) we also have the wire bytes, and I think we landed on
"WasmWireBytes" as a term to refer to them.

Separate from this CL, I think we should take a look at all these concepts and
come up with a terminology we can use consistently internally. Opened
https://bugs.chromium.org/p/v8/issues/detail?id=5521 to track.

[titzer, 2016-10-17 09:04:07]

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

[commit-bot: I haz the power, 2016-10-17 09:04:17]

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-17 09:16:48]

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

[commit-bot: I haz the power, 2016-10-17 09:16:49]

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/16280)

[titzer, 2016-10-17 12:06:54]

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

[commit-bot: I haz the power, 2016-10-17 12:07:07]

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-17 12:09:36]

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

[commit-bot: I haz the power, 2016-10-17 12:09:37]

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

[titzer, 2016-10-17 12:22:17]

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

[commit-bot: I haz the power, 2016-10-17 12:22: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-17 12:24:13]

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

[commit-bot: I haz the power, 2016-10-17 12:24:14]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14710)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[titzer, 2016-10-17 12:28:31]

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

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

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-17 12:31:32]

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

[commit-bot: I haz the power, 2016-10-17 12:31:33]

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

[titzer, 2016-10-17 12:31:59]

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

[commit-bot: I haz the power, 2016-10-17 12:32:07]

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

[Yang, 2016-10-17 12:47:33]

On 2016/10/15 17:38:41, Mircea Trofin wrote:
> I love the direction of this CL. 
> 
> My main high-level feedback/concerns:
> - are there a few CLs in one here, can they be thus separated?
> - see the comment on api.cc
> - the comment about string interning in wasm-module.cc
> - when does the Cpp WasmModule get deleted? (It may well be a property of
> Foreign, but haven't seen that documented on Foreign itself. Maybe I missed
> something, too)
> 
> https://codereview.chromium.org/2424623002/diff/1/src/api.cc
> File src/api.cc (left):
> 
> https://codereview.chromium.org/2424623002/diff/1/src/api.cc#oldcode7195
> src/api.cc:7195: compiled_part->reset_module_bytes();
> Consider the scenario when we try to deserialize from an older version stored
in
> indexedDB, and need to recompile. To recompile, we need the wasm wire bytes.
> Since deserialization won't even get past the serialized header, our only way
to
> get back to the wire bytes is to explicitly serialize them (as discussed, long
> term, we envision they are stored separately as Blobs). 
> 
> This means the serialized payload is wire_bytes + serialized_module.
> 
> If we leave the wire bytes hanging off the serialized module, the serialized
> payload is 2*wire_bytes + serialized_module.
> 
> Typically, the serialized module is about 4-5x larger than the wire bytes. So
> this means the duplicate wire_bytes constitute a 16-20% increase in serialized
> data.
> 
> To achieve the goal of this CL, we only need the header bytes. Is the plan to
> follow-up with another CL, where that happens? If so, I think we should do
that
> before we ship, so we should add an issue#; also, are we OK with taking a hit
in
> user-observable properties in the meantime? (I realize the whole wasm feature
is
> under a flag, so I don't think this is much of an issue myself, just calling
it
> out to make sure we understand the trade-offs and are all OK with them).
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/compiler/wasm-compiler.cc
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.cc:2160: Node* sig_match =
> graph()->NewNode(machine->WordEqual(), load_sig,
> Is this related, or can it be a separate CL?
> 
> https://codereview.chromium.org/2424623002/diff/1/src/isolate.cc
> File src/isolate.cc (right):
> 
> https://codereview.chromium.org/2424623002/diff/1/src/isolate.cc#newcode512
> src/isolate.cc:512: Handle<Object> wasm_module(wasm_frame->wasm_obj(), this);
> Could the renames be a separate change?
> 
> https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:80: return isolate->factory()->NewStringFromUtf8(vec);
> Shouldn't we worry about GC happening when: allocating the memory for the new
> string; then module_bytes being compacted; and then raw being invalid (and
> therefore what vec holds on to), and then the copying ?
> 
> Also (this is after reading further down in the change), it looks like we're
> using this to peel out strings from serialized data for things like imports
(the
> labels used internally by wasm functions), but we'd do that every time we
create
> a new instance. We could avoid that allocation, the strings are shared by all
> module instances. We can either use one of the Internalize* String APIs (like
> InternalizeUtf8String); that has some computation overhead; or we can take a
> model complexity hit and hang them off the CompiledModule in a fixed array. I
> realize this would go back a step (albeit small) from the reduction of
> duplication goal of this CL.
> 
> Or - could the APIs consuming these strings not use strings? Don't we just do
> lookups and other stuff like that?
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:699: WasmModule* CppModule(Handle<JSObject>
wasm_module)
> {
> GetCppModule? (as opposed to Create)
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:729: Handle<FixedArray> function_tables;
> I thought we're supposed to use MaybeHandle whenever "null" was a valid value
> for the variable.
> 
> Alternatively - since we're only using this further below, to set on the
> returned value - could we just do it all there? (line 809)
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:815: for (auto exp : export_table) {
> I know auto is allowed for iterators, but since we don't use exp as an
iterator,
> but rather as a ref to whatever export_table contains, mind typing it as such
> (easier to read - don't need to jump to definition of export_table)
> 
> Also, it's a const, correct?
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:823: func_index++;
> ++func_index;
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1049: for (int i = 0; i < new_table->length(); i++) {
> ++i
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1055: for (auto table_init : module_->table_inits) {
> same comment about auto as above.
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1058: for (size_t i = 0; i <
table_init.entries.size();
> i++) {
> ++i
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1069: for (int i = 0; i < code_table->length(); i++) {
> ++i
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1072: for (int j = 0; j < function_table_count; j++) {
> ++j
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1161: WasmModule* module_;
> elsewhere, there was a CppModule API. Could we call this cpp_module_ - or
> perhaps let's punt this renaming until we agree on a nomenclature
> (https://bugs.chromium.org/p/v8/issues/detail?id=5521)
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1276: Handle<ByteArray> exportedSig =
Handle<ByteArray>(
> do we still need this, given we now have the native signature available?
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1326: *GetRawGlobalPtr<int32_t>(global, globals) =
> static_cast<int32_t>(num);
> Is this change here related, or could it be a different change?
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1350: index++) {
> ++index
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1353: ExtractStringFromModuleBytes(isolate_,
> compiled_module_,
> ExtractString allocates each time we call it. Should we rather intern this
> string?
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1508: for (auto exp : module_->export_table) {
> same comment about auto as above.
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1510: ExtractStringFromModuleBytes(isolate_,
> compiled_module_,
> same comment about string interning as above
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1990: Handle<FixedArray> array) {
> why not pass this guy as a Handle<WasmCompiledModule> outright?
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:2004: DecodeWasmModule(isolate, start, end, false,
> kWasmOrigin);
> Could you add a TODO to avoid decoding the entire module, since we only need
the
> headers?
> 
> https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h
> File src/wasm/wasm-module.h (right):
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
> src/wasm/wasm-module.h:392: MACRO(OBJECT, Foreign, module_wrapper)            

> \
> I assume the pointer held by the Foreign is deleted when the Foreign object is
> collected?
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
> src/wasm/wasm-module.h:395: MACRO(OBJECT, FixedArray, function_tables)        

> \
> Why drop the "indirect" prefix?
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
> src/wasm/wasm-module.h:502: bool IsWasmModule(Object* wasm_module);
> I also think "WasmModule" is a better name than "WasmObject", however, our
code
> has 2 types of objects that are "WasmModules": an instance of the native
> WasmModule struct; and an instance of WebAssembly.Module.
> 
> Separately, we have the compiled data, currently referred to
> "WasmCompiledModule", which is also talking about "WasmModule".
> 
> (While we're at it) we also have the wire bytes, and I think we landed on
> "WasmWireBytes" as a term to refer to them.
> 
> Separate from this CL, I think we should take a look at all these concepts and
> come up with a terminology we can use consistently internally. Opened
> https://bugs.chromium.org/p/v8/issues/detail?id=5521 to track.

src/snapshot lgtm.

[commit-bot: I haz the power, 2016-10-17 12:55:32]

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

[commit-bot: I haz the power, 2016-10-17 12:55:33]

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...)
  v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/14676)
  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-17 17:50:47]

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

[commit-bot: I haz the power, 2016-10-17 17:50:52]

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-17 18:18:00]

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

[commit-bot: I haz the power, 2016-10-17 18:18:01]

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...)
  v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
  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...)

[titzer, 2016-10-18 10:55:52]

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

[commit-bot: I haz the power, 2016-10-18 10:55:54]

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-18 11:21:54]

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

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

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-18 12:56:14]

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

[commit-bot: I haz the power, 2016-10-18 12:56:24]

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-18 12:58:45]

On 2016/10/15 17:38:41, Mircea Trofin wrote:
> I love the direction of this CL. 
> 

Thanks!

> My main high-level feedback/concerns:
> - are there a few CLs in one here, can they be thus separated?
> - see the comment on api.cc
> - the comment about string interning in wasm-module.cc

I've added a TODO

> - when does the Cpp WasmModule get deleted? 

It's a Managed<T>, which was introduced in the dependent CL which has now
landed.

> 
> https://codereview.chromium.org/2424623002/diff/1/src/api.cc
> File src/api.cc (left):
> 
> 
> To achieve the goal of this CL, we only need the header bytes. Is the plan to
> follow-up with another CL, where that happens? If so, I think we should do
that
> before we ship, so we should add an issue#; also, are we OK with taking a hit
in
> user-observable properties in the meantime? (I realize the whole wasm feature
is
> under a flag, so I don't think this is much of an issue myself, just calling
it
> out to make sure we understand the trade-offs and are all OK with them).
> 

Acknowledged. I've added a TODO to only save the header. Note that we would need
to store the names section as well, however.

>
https://codereview.chromium.org/2424623002/diff/1/src/compiler/wasm-compiler.cc
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.cc:2160: Node* sig_match =
> graph()->NewNode(machine->WordEqual(), load_sig,
> Is this related, or can it be a separate CL?

I can pull this out to a separate CL.

> 
> https://codereview.chromium.org/2424623002/diff/1/src/isolate.cc
> File src/isolate.cc (right):
> 
> https://codereview.chromium.org/2424623002/diff/1/src/isolate.cc#newcode512
> src/isolate.cc:512: Handle<Object> wasm_module(wasm_frame->wasm_obj(), this);
> Could the renames be a separate change?

I renamed the API method to IsWasmInstance(), so it makes sense to rename the
local variables.
I think we should rename the frame's method in a later CL.

> 
> https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:80: return isolate->factory()->NewStringFromUtf8(vec);
> Shouldn't we worry about GC happening when: allocating the memory for the new
> string; then module_bytes being compacted; and then raw being invalid (and
> therefore what vec holds on to), and then the copying ?
> 

Yes, this is why I had a TODO. I've fixed this with a new factory method.

> Also (...), it looks like we're
> using this to peel out strings from serialized data for things like imports
(...), but we'd do that every time we create
> a new instance. ... 

I don't think we should necessarily internalize them (that canonicalizes them in
a hashtable),
but caching we can do later. Left a TODO.

> 
> Or - could the APIs consuming these strings not use strings? Don't we just do
> lookups and other stuff like that?

JS property accesses require strings.

> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:699: WasmModule* CppModule(Handle<JSObject>
wasm_module)
> {
> GetCppModule? (as opposed to Create)
> 

Renamed to GetCppModule. I didn't really like the Cpp* name, so I hope it
remains temporary
(and currently it is local to this file).

>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:729: Handle<FixedArray> function_tables;
> I thought we're supposed to use MaybeHandle whenever "null" was a valid value
> for the variable.

I've changed this to just allocate a zero-length array.

> 
> Alternatively - since we're only using this further below, to set on the
> returned value - could we just do it all there? (line 809)
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:815: for (auto exp : export_table) {
> I know auto is allowed for iterators, but since we don't use exp as an
iterator,
> but rather as a ref to whatever export_table contains, mind typing it as such
> (easier to read - don't need to jump to definition of export_table)
> 
> Also, it's a const, correct?
> 

The style guide does recommend using auto for range-based for loops where enough
context
makes it clear what the type is. Since this is exp : export_table, it should be
clear
that it is an export structure, and since it's in the module, it should be clear
its
the export structure from the WasmModule.

>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:823: func_index++;
> ++func_index;
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1049: for (int i = 0; i < new_table->length(); i++) {
> ++i
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1055: for (auto table_init : module_->table_inits) {
> same comment about auto as above.
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1058: for (size_t i = 0; i <
table_init.entries.size();
> i++) {
> ++i
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1069: for (int i = 0; i < code_table->length(); i++) {
> ++i
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1072: for (int j = 0; j < function_table_count; j++) {
> ++j
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1161: WasmModule* module_;
> elsewhere, there was a CppModule API. Could we call this cpp_module_ - or
> perhaps let's punt this renaming until we agree on a nomenclature
> (https://bugs.chromium.org/p/v8/issues/detail?id=5521)

I'd rather keep the short name {module} for the things we use most, and qualify
the others.

> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1276: Handle<ByteArray> exportedSig =
Handle<ByteArray>(
> do we still need this, given we now have the native signature available?
> 

Done.

>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1326: *GetRawGlobalPtr<int32_t>(global, globals) =
> static_cast<int32_t>(num);
> Is this change here related, or could it be a different change?
> 

It is related, since it is now better encapsulated with EvalUint32InitExpr.

>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1350: index++) {
> ++index
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1353: ExtractStringFromModuleBytes(isolate_,
> compiled_module_,
> ExtractString allocates each time we call it. Should we rather intern this
> string?
> 

TODO, that's an optimization.

>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1508: for (auto exp : module_->export_table) {
> same comment about auto as above.
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1510: ExtractStringFromModuleBytes(isolate_,
> compiled_module_,
> same comment about string interning as above
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1990: Handle<FixedArray> array) {
> why not pass this guy as a Handle<WasmCompiledModule> outright?
> 

Ah, because the caller would have to do a WasmCompiledModule::cast() which
actually checks
the fields, but the fixed array is not fully initialized yet, and the cast
fails.

[titzer, 2016-10-18 12:58:55]

>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:2004: DecodeWasmModule(isolate, start, end, false,
> kWasmOrigin);
> Could you add a TODO to avoid decoding the entire module, since we only need
the
> headers?

It does skip verifying the function bodies.

> 
> https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h
> File src/wasm/wasm-module.h (right):
> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
> src/wasm/wasm-module.h:392: MACRO(OBJECT, Foreign, module_wrapper)            

> \
> I assume the pointer held by the Foreign is deleted when the Foreign object is
> collected?

Managed<T> does that, yes.

> 
>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
> src/wasm/wasm-module.h:395: MACRO(OBJECT, FixedArray, function_tables)        

> \
> Why drop the "indirect" prefix?
> 

This is the only place where it is called indirect_function_tables.

>
https://codereview.chromium.org/2424623002/diff/1/src/wasm/wasm-module.h#newc...
> src/wasm/wasm-module.h:502: bool IsWasmModule(Object* wasm_module);
> I also think "WasmModule" is a better name than "WasmObject", however, our
code
> has 2 types of objects that are "WasmModules": an instance of the native
> WasmModule struct; and an instance of WebAssembly.Module.

This was actually wrong; it's now IsWasmInstance() now.

> Separately, we have the compiled data, currently referred to
> "WasmCompiledModule", which is also talking about "WasmModule".
> 
> (While we're at it) we also have the wire bytes, and I think we landed on
> "WasmWireBytes" as a term to refer to them.
> 
> Separate from this CL, I think we should take a look at all these concepts and
> come up with a terminology we can use consistently internally. Opened
> https://bugs.chromium.org/p/v8/issues/detail?id=5521 to track.

Agree.

[commit-bot: I haz the power, 2016-10-18 13:24:18]

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

[commit-bot: I haz the power, 2016-10-18 13:24:19]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-10-19 05:28:56]

https://codereview.chromium.org/2424623002/diff/180001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2424623002/diff/180001/src/factory.cc#newcode339
src/factory.cc:339: for (int i = 0; i < non_ascii_start; i++) {
can't this be a condition on ascii_data not hitting ascii_data +
non_ascii_start? Less context for the reader to think about.

https://codereview.chromium.org/2424623002/diff/180001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2424623002/diff/180001/src/isolate.cc#newcode513
src/isolate.cc:513: Handle<Object> instance(wasm_frame->wasm_obj(), this);
I really wish the renames (while super-welcome) came in as a separate CL.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/module-decode...
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/module-decode...
src/wasm/module-decoder.cc:1099: // allocated on the C++ heap.
mind opening and referencing an issue for this?

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/signature-map.cc
File src/wasm/signature-map.cc (right):

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/signature-map...
src/wasm/signature-map.cc:52: for (size_t i = 0; i < a->parameter_count(); i++)
{
This bottom part could be done blindly wrt what's a parameter and what's a
return value, since we 
established they are the same. At that point, can't we just compare the
underlying storage?

Any reason this API isn't a member of FunctionSig?

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/signature-map.h
File src/wasm/signature-map.h (right):

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/signature-map...
src/wasm/signature-map.h:29: static bool Equal(FunctionSig* a, FunctionSig* b);
Did you mean "Equals" (or maybe "Is" or "AreEqual")?

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

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:68: MaybeHandle<String> ExtractStringFromModuleBytes(
Did you mean to add a TODO here to intern the strings - if so, mind opening an
issue, too?

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:832: // {WasmModule}. E.g. function bodies could be
omitted.
Mind opening an issue for this and referencing here? 

Actually, there may be 2 issues:
1) once Chrome uses DeserializeOrCompile instead of Deserialize, we can avoid
the memory/storage use regression this CL will introduce by having had removed
the 3 lines in api.cc - 7207, 7208 and 7212, by getting the wire bytes directly
from what's passed in to DeserializeOrCompile. Depending on timing (like if my
change to chrome goes in before this CL), we could avoid this regression
altogether

2) the topic of this TODO here, which, once addressed, would also address the
topic above.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1961: // We parse the module again directly from the
module bytes; disable GC.
"disable GC" is misleading. DisallowHeapAllocation (misleadingly :) ) just
asserts there's no heap allocation in that scope, and, therefore, no risk of GC.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1967: // For now, we assume serialized modules did not
originate from asm.js.
I don't think it could, because there's no API to serialize from asm.js, and
RecreateModuleWrapper is a (de)serialization facility - or I'm missing
something?

[rossberg, 2016-10-19 09:19:16]

Refactoring LGTM modulo a few nits

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/signature-map.h
File src/wasm/signature-map.h (right):

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/signature-map...
src/wasm/signature-map.h:28: // Compares two signatures for equality.
Shouldn't this better be a method of Signature?

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

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:68: MaybeHandle<String> ExtractStringFromModuleBytes(
On 2016/10/19 05:28:55, Mircea Trofin wrote:
> Did you mean to add a TODO here to intern the strings - if so, mind opening an
> issue, too?

I'm actually not sure whether it's worth (or even good) to intern all strings by
default.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:701: WasmModule* GetCppModule(Handle<JSObject> instance)
{
`GetCppModule`, really? :)

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:734: if (function_table_count > 0) {
Wrapping this conditional around the loop seems redundant.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1176: Handle<JSObject> wasm_module_;
Nit: It's a bit irritating that module_ hast type WasmModule and wam_module has
not.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1291: isMatch = SignatureMap::Equal(sig, other_sig);
Style nit: is_match

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:446: return min_mem_pages() * WasmModule::kPageSize;
return default_mem_size() ?

[titzer, 2016-10-19 09:44:57]

Description was changed from

==========
[wasm] Use a Managed<WasmModule> to hold metadata about modules.

This CL refactors the handling of metadata associated with WebAssembly
modules to reduce the duplicate marshalling of data from the C++ world
to the JavaScript world. It does this by wrapping the C++ WasmModule*
object in a Foreign that is rooted from the on-heap WasmCompiledModule
(which is itself just a FixedArray). Upon serialization, the C++ object
is ignored and the original WASM wire bytes are serialized. Upon
deserialization, the C++ object is reconstituted by reparsing the bytes.

This is motivated by increasing complications in implementing the JS
API, in particular WebAssembly.Table, which must perform signature
canonicalization across instances.

Additionally, this CL implements the proper base + offset initialization
behavior for tables.

R=rossberg@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org,yangguo@...
BUG=v8:5507, chromium:575167
==========

to

==========
[wasm] Use a Managed<WasmModule> to hold metadata about modules.

This CL refactors the handling of metadata associated with WebAssembly
modules to reduce the duplicate marshalling of data from the C++ world
to the JavaScript world. It does this by wrapping the C++ WasmModule*
object in a Foreign that is rooted from the on-heap WasmCompiledModule
(which is itself just a FixedArray). Upon serialization, the C++ object
is ignored and the original WASM wire bytes are serialized. Upon
deserialization, the C++ object is reconstituted by reparsing the bytes.

This is motivated by increasing complications in implementing the JS
API, in particular WebAssembly.Table, which must perform signature
canonicalization across instances.

Additionally, this CL implements the proper base + offset initialization
behavior for tables.

R=rossberg@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org,yangguo@...
BUG=v8:5507, chromium:575167, chromium:657316
==========

[titzer, 2016-10-19 09:54:31]

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

[commit-bot: I haz the power, 2016-10-19 09:54:41]

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-19 09:54:41]

https://codereview.chromium.org/2424623002/diff/180001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2424623002/diff/180001/src/factory.cc#newcode339
src/factory.cc:339: for (int i = 0; i < non_ascii_start; i++) {
On 2016/10/19 05:28:55, Mircea Trofin wrote:
> can't this be a condition on ascii_data not hitting ascii_data +
> non_ascii_start? Less context for the reader to think about.

This is essentially a copy of the above code, so further improvements should
probably be done in both places.

https://codereview.chromium.org/2424623002/diff/180001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2424623002/diff/180001/src/isolate.cc#newcode513
src/isolate.cc:513: Handle<Object> instance(wasm_frame->wasm_obj(), this);
On 2016/10/19 05:28:55, Mircea Trofin wrote:
> I really wish the renames (while super-welcome) came in as a separate CL.

Renaming split out to: https://codereview.chromium.org/2428343005/

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/module-decode...
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/module-decode...
src/wasm/module-decoder.cc:1099: // allocated on the C++ heap.
On 2016/10/19 05:28:55, Mircea Trofin wrote:
> mind opening and referencing an issue for this?

Done.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/signature-map.cc
File src/wasm/signature-map.cc (right):

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/signature-map...
src/wasm/signature-map.cc:52: for (size_t i = 0; i < a->parameter_count(); i++)
{
On 2016/10/19 05:28:55, Mircea Trofin wrote:
> This bottom part could be done blindly wrt what's a parameter and what's a
> return value, since we 
> established they are the same. At that point, can't we just compare the
> underlying storage?
> 
> Any reason this API isn't a member of FunctionSig?

Done.

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

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:68: MaybeHandle<String> ExtractStringFromModuleBytes(
On 2016/10/19 09:19:16, rossberg wrote:
> On 2016/10/19 05:28:55, Mircea Trofin wrote:
> > Did you mean to add a TODO here to intern the strings - if so, mind opening
an
> > issue, too?
> 
> I'm actually not sure whether it's worth (or even good) to intern all strings
by
> default.

TODO(wasm) added

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:701: WasmModule* GetCppModule(Handle<JSObject> instance)
{
On 2016/10/19 09:19:15, rossberg wrote:
> `GetCppModule`, really? :)

Yeah, I hope to rename it in the next round.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:734: if (function_table_count > 0) {
On 2016/10/19 09:19:15, rossberg wrote:
> Wrapping this conditional around the loop seems redundant.

Done.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:832: // {WasmModule}. E.g. function bodies could be
omitted.
On 2016/10/19 05:28:55, Mircea Trofin wrote:
> Mind opening an issue for this and referencing here? 
> 
> Actually, there may be 2 issues:
> 1) once Chrome uses DeserializeOrCompile instead of Deserialize, we can avoid
> the memory/storage use regression this CL will introduce by having had removed
> the 3 lines in api.cc - 7207, 7208 and 7212, by getting the wire bytes
directly
> from what's passed in to DeserializeOrCompile. Depending on timing (like if my
> change to chrome goes in before this CL), we could avoid this regression
> altogether
> 
> 2) the topic of this TODO here, which, once addressed, would also address the
> topic above.

Done.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1176: Handle<JSObject> wasm_module_;
On 2016/10/19 09:19:15, rossberg wrote:
> Nit: It's a bit irritating that module_ hast type WasmModule and wam_module
has
> not.

Renamed to module_object_

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1291: isMatch = SignatureMap::Equal(sig, other_sig);
On 2016/10/19 09:19:16, rossberg wrote:
> Style nit: is_match

Done.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1961: // We parse the module again directly from the
module bytes; disable GC.
On 2016/10/19 05:28:55, Mircea Trofin wrote:
> "disable GC" is misleading. DisallowHeapAllocation (misleadingly :) ) just
> asserts there's no heap allocation in that scope, and, therefore, no risk of
GC.

Reworded

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1967: // For now, we assume serialized modules did not
originate from asm.js.
On 2016/10/19 05:28:55, Mircea Trofin wrote:
> I don't think it could, because there's no API to serialize from asm.js, and
> RecreateModuleWrapper is a (de)serialization facility - or I'm missing
> something?

We might want to cache compiled asm.js in the future.

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2424623002/diff/180001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:446: return min_mem_pages() * WasmModule::kPageSize;
On 2016/10/19 09:19:16, rossberg wrote:
> return default_mem_size() ?

Done.

[commit-bot: I haz the power, 2016-10-19 09:57:30]

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

[commit-bot: I haz the power, 2016-10-19 09:57:31]

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

[titzer, 2016-10-19 10:00:19]

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

[commit-bot: I haz the power, 2016-10-19 10:00:25]

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-19 10:05:12]

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

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

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

[titzer, 2016-10-19 11:13:49]

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

[commit-bot: I haz the power, 2016-10-19 11:14:00]

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-19 12:19:00]

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

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

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-19 12:22:41]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-10-19 12:22:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, rossberg@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2424623002/#ps240001 (title: "[wasm] Use
a Managed<WasmModule> to hold metadata about modules.")

[commit-bot: I haz the power, 2016-10-19 12:22:49]

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-19 13:06:49]

Description was changed from

==========
[wasm] Use a Managed<WasmModule> to hold metadata about modules.

This CL refactors the handling of metadata associated with WebAssembly
modules to reduce the duplicate marshalling of data from the C++ world
to the JavaScript world. It does this by wrapping the C++ WasmModule*
object in a Foreign that is rooted from the on-heap WasmCompiledModule
(which is itself just a FixedArray). Upon serialization, the C++ object
is ignored and the original WASM wire bytes are serialized. Upon
deserialization, the C++ object is reconstituted by reparsing the bytes.

This is motivated by increasing complications in implementing the JS
API, in particular WebAssembly.Table, which must perform signature
canonicalization across instances.

Additionally, this CL implements the proper base + offset initialization
behavior for tables.

R=rossberg@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org,yangguo@...
BUG=v8:5507, chromium:575167, chromium:657316
==========

to

==========
[wasm] Use a Managed<WasmModule> to hold metadata about modules.

This CL refactors the handling of metadata associated with WebAssembly
modules to reduce the duplicate marshalling of data from the C++ world
to the JavaScript world. It does this by wrapping the C++ WasmModule*
object in a Foreign that is rooted from the on-heap WasmCompiledModule
(which is itself just a FixedArray). Upon serialization, the C++ object
is ignored and the original WASM wire bytes are serialized. Upon
deserialization, the C++ object is reconstituted by reparsing the bytes.

This is motivated by increasing complications in implementing the JS
API, in particular WebAssembly.Table, which must perform signature
canonicalization across instances.

Additionally, this CL implements the proper base + offset initialization
behavior for tables.

R=rossberg@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org,yangguo@...
BUG=v8:5507, chromium:575167, chromium:657316
==========

[commit-bot: I haz the power, 2016-10-19 13:06:51]

Committed patchset #13 (id:240001)

[Michael Achenbach, 2016-10-19 13:42:04]

Please fix
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder...

(or help figuring out how to switch off this warning if it's not helpful)

[Michael Achenbach, 2016-10-19 13:42:45]

On 2016/10/19 13:42:04, machenbach (slow) wrote:
> Please fix
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder...
> 
> (or help figuring out how to switch off this warning if it's not helpful)

Also on the gcov bot:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20gcov%20cov...

[titzer, 2016-10-19 13:56:05]

On 2016/10/19 13:42:45, machenbach (slow) wrote:
> On 2016/10/19 13:42:04, machenbach (slow) wrote:
> > Please fix
> >
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder...
> > 
> > (or help figuring out how to switch off this warning if it's not helpful)
> 
> Also on the gcov bot:
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20gcov%20cov...

Fix landed: https://chromiumcodereview.appspot.com/2431183005/

[titzer, 2016-10-19 13:58:02]

On 2016/10/19 13:56:05, titzer wrote:
> On 2016/10/19 13:42:45, machenbach (slow) wrote:
> > On 2016/10/19 13:42:04, machenbach (slow) wrote:
> > > Please fix
> > >
> >
>
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder...
> > > 
> > > (or help figuring out how to switch off this warning if it's not helpful)
> > 
> > Also on the gcov bot:
> >
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20gcov%20cov...
> 
> Fix landed: https://chromiumcodereview.appspot.com/2431183005/

BTW I think the warning here was slightly wrong, since it warned on a pattern
like this:

int a = cond ? a = 1 : 2;

Whatever analysis it is using thinks that "a" on the right hand side is a use of
"a" rather than an assignment.

[commit-bot: I haz the power, 2016-11-17 22:06:27]

Description was changed from

==========
[wasm] Use a Managed<WasmModule> to hold metadata about modules.

This CL refactors the handling of metadata associated with WebAssembly
modules to reduce the duplicate marshalling of data from the C++ world
to the JavaScript world. It does this by wrapping the C++ WasmModule*
object in a Foreign that is rooted from the on-heap WasmCompiledModule
(which is itself just a FixedArray). Upon serialization, the C++ object
is ignored and the original WASM wire bytes are serialized. Upon
deserialization, the C++ object is reconstituted by reparsing the bytes.

This is motivated by increasing complications in implementing the JS
API, in particular WebAssembly.Table, which must perform signature
canonicalization across instances.

Additionally, this CL implements the proper base + offset initialization
behavior for tables.

R=rossberg@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org,yangguo@...
BUG=v8:5507, chromium:575167, chromium:657316
==========

to

==========
[wasm] Use a Managed<WasmModule> to hold metadata about modules.

This CL refactors the handling of metadata associated with WebAssembly
modules to reduce the duplicate marshalling of data from the C++ world
to the JavaScript world. It does this by wrapping the C++ WasmModule*
object in a Foreign that is rooted from the on-heap WasmCompiledModule
(which is itself just a FixedArray). Upon serialization, the C++ object
is ignored and the original WASM wire bytes are serialized. Upon
deserialization, the C++ object is reconstituted by reparsing the bytes.

This is motivated by increasing complications in implementing the JS
API, in particular WebAssembly.Table, which must perform signature
canonicalization across instances.

Additionally, this CL implements the proper base + offset initialization
behavior for tables.

R=rossberg@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org,yangguo@...
BUG=v8:5507, chromium:575167, chromium:657316

Committed: https://crrev.com/418b239f0b379cbf97bc74b93a31d61382a1ed13
Cr-Commit-Position: refs/heads/master@{#40434}
==========

[commit-bot: I haz the power, 2016-11-17 22:06:29]

Patchset 13 (id:??) landed as
https://crrev.com/418b239f0b379cbf97bc74b93a31d61382a1ed13
Cr-Commit-Position: refs/heads/master@{#40434}