[wasm] cloning compiled module before instantiation

[wasm] cloning compiled module before instantiation

To correctly support instantiating a compiled module multiple times, we clone the
compiled module each time we create an instance, since some of the data is specific
to the instance - e.g. export code, wasm functions, indirect table.

BUG=v8:5072
Committed: https://crrev.com/81f42220a63f8d29bde947171b1ed8e555034b53
Cr-Commit-Position: refs/heads/master@{#37692}

[Mircea Trofin, 2016-07-08 06:29:16]

Description was changed from

==========
[wasm] cloning compiled module before instantiation
BUG=
==========

to

==========
[wasm] cloning compiled module before instantiation

To correctly support instantiating a compiled module multiple times, we clone
the
compiled module each time we create an instance, since some of the data is
specific
to the instance - e.g. export code, wasm functions, indirect table.


BUG=v8:5072
==========

[Mircea Trofin, 2016-07-08 06:29:16]

mtrofin@chromium.org changed reviewers:
+ rossberg@chromium.org, titzer@chromium.org

[Mircea Trofin, 2016-07-08 06:32:06]

PTAL.

After this CL makes it in, I'll add more tests to 2121593002, which, at that
point, should
correctly support multiple instantiation. Thanks, Andreas, for pointing out
those tests in your
feedback for the aforementioned CL!

A subsequent change will address the topic of removing the overhead of keeping
around the 
compiled code as a "template".

[Mircea Trofin, 2016-07-09 04:36:44]

PTAL

[titzer, 2016-07-11 08:25:36]

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

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:190: enum WasmExportMetadata {
I didn't see the prior CL where these started popping up. With this, now we have
3 representations of a WASM module: the encoded bytes, the C++ data structures,
and a reification on the JS heap.

Why don't we instead serialize the export section as bytes and decode it at
instantiate time?

(same for imports and data segments)?

Ultimately, we're going to end up with a complete copy of the original WASM
bytes, just in a less dense form.

[rossberg, 2016-07-11 10:49:46]

This looks okay, but could probably use at least some basic tests.

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

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:130: void PatchFunctionTable(Handle<Code> code,
Nit: static or in namespace?

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1104: export_metadata->set(kExportCode, *export_code);
Instead of spreading the details here and elsewhere, can you introduce
de/serialise functions that encapsulate the heap representation and the
conversion between a WasmExport and a metadata fixed array?

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1196: if (!maybe_indirect_table.is_null()) {
Nit: use ToHandle instead of separate is_null and ToHandleCHecked (here and
below)

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1229: // we are taking the risk of invalid values. We
should fix this in
FixedArray cannot actually support the full uint32_t range on 32bit platforms.
So some guarding is inevitable. (Also, STL indices and sizes being unsigned is a
known footgun.)

[Mircea Trofin, 2016-07-11 16:49:24]

Patchset #6 (id:100001) has been deleted

[Mircea Trofin, 2016-07-11 16:52:57]

@rossberg, re. tests: This change is meant to be an in-place, to ensure that, by
cloning, nothing breaks. There's going to be more testing with the other CL
where I add the implementation for WebAssembly.Module  and .Instance - and we
can actually try out 1 compilation->multiple instantiations.

Was there additional testing you had in mind?

Thanks!

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

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:130: void PatchFunctionTable(Handle<Code> code,
On 2016/07/11 10:49:45, rossberg wrote:
> Nit: static or in namespace?

Done.

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:190: enum WasmExportMetadata {
On 2016/07/11 08:25:35, titzer wrote:
> I didn't see the prior CL where these started popping up. With this, now we
have
> 3 representations of a WASM module: the encoded bytes, the C++ data
structures,
> and a reification on the JS heap.
> 
> Why don't we instead serialize the export section as bytes and decode it at
> instantiate time?
> 
> (same for imports and data segments)?
> 
> Ultimately, we're going to end up with a complete copy of the original WASM
> bytes, just in a less dense form.

My short term goal is to get an end-to-end indexedDB-able compiled module. I
looked into reusing the deserializer back when I did the other CLs. Adapting it
for the scenario here doesn't seem too difficult, but I'd prefer doing that once
I got the end-to-end story up and running. This is because, as we've seen up to
this point, there are larger issues to address (such as handling the tie-in
between instance and compiled code, and the more subtle implications that turned
out to have); in comparison, once all is done, reworking the specific data
handed off between compiled code and instantiation is a fairly local, risk-free
operation.

There's another reason for preferring to wait with cleaning up this. We want to
move wasm off the JS Heap. At that point, I suspect we won't need to do this
dance with the data anymore in the first place. I'd prefer doing that move
sooner rather than later, too (e.g. after the indexedDB feature lands).

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1104: export_metadata->set(kExportCode, *export_code);
On 2016/07/11 10:49:45, rossberg wrote:
> Instead of spreading the details here and elsewhere, can you introduce
> de/serialise functions that encapsulate the heap representation and the
> conversion between a WasmExport and a metadata fixed array?

It's not really a WasmExport anymore, because it has the compiled code. See also
the above reply to titzer.

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1196: if (!maybe_indirect_table.is_null()) {
On 2016/07/11 10:49:45, rossberg wrote:
> Nit: use ToHandle instead of separate is_null and ToHandleCHecked (here and
> below)

Done.

https://codereview.chromium.org/2134593002/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1229: // we are taking the risk of invalid values. We
should fix this in
On 2016/07/11 10:49:45, rossberg wrote:
> FixedArray cannot actually support the full uint32_t range on 32bit platforms.
> So some guarding is inevitable. (Also, STL indices and sizes being unsigned is
a
> known footgun.)

We should talk about this separately. I trimmed the TODO for now. I think we
have a problem at the boundary wasm/JS because of this, and it'd be great if we
didn't spread the guarding allover the place - so leaving a TODO there.

[titzer, 2016-07-12 15:45:52]

Mostly there, so I am willing to go with this for now, but I am still worried
about having 3 representations of WASM modules, especially since the on-JS-heap
representation cannot be accessed by multiple threads. That is going to have to
be solved if we want to, e.g. do any parallel work in instantiation.

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

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:115: Handle<JSFunction> CreateExport(Isolate* isolate,
Handle<Code> export_code,
WrapCodeAsJSFunction?

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:269: for (uint32_t i = 0; i <
module->function_table.size(); ++i) {
I'm surprised you didn't get a warning for removing that static cast.

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:445: Assembler::FlushICache(isolate,
code->instruction_start(),
I'm not sure how expensive Assembler::FlushICache is; it might be worth it to
just collect the entire range and flush it all in one go, or all code pages at
once, e.g.

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

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:170: // presence/absence separately, as a bool.
Can you remove this TODO? I'd rather keep the current behavior. We'll need to
establish a limit on the number of functions in the decoder and the range of an
int seems reasonable to me.

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:365: // Create an export JSFunction
s/an/and/

[Mircea Trofin, 2016-07-12 16:12:23]

PTAL 

We'll have all sorts of problems as long as we live on the JS Heap, but,
as our plan has been, we'll move off it as soon as we have the 
indexedDB-able compiled code.

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

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:115: Handle<JSFunction> CreateExport(Isolate* isolate,
Handle<Code> export_code,
On 2016/07/12 15:45:52, titzer wrote:
> WrapCodeAsJSFunction?

Done, keeping the label "Export" somewhere there, to capture what value it
provides to the caller, rather than 
just what it does internally.

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:269: for (uint32_t i = 0; i <
module->function_table.size(); ++i) {
On 2016/07/12 15:45:52, titzer wrote:
> I'm surprised you didn't get a warning for removing that static cast.

it's not losing signedness, so it can cast LHS to the larger RHS type without
warning.

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:445: Assembler::FlushICache(isolate,
code->instruction_start(),
On 2016/07/12 15:45:52, titzer wrote:
> I'm not sure how expensive Assembler::FlushICache is; it might be worth it to
> just collect the entire range and flush it all in one go, or all code pages at
> once, e.g.

That's why I moved this in one place, to flush it all when I'm done with the
various patchings.

...or did I misinterpret your comment?

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

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:170: // presence/absence separately, as a bool.
On 2016/07/12 15:45:52, titzer wrote:
> Can you remove this TODO? I'd rather keep the current behavior. We'll need to
> establish a limit on the number of functions in the decoder and the range of
an
> int seems reasonable to me.

Reworded it, we do have a problem, so I'd be remiss if I just removed it.

https://codereview.chromium.org/2134593002/diff/120001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:365: // Create an export JSFunction
On 2016/07/12 15:45:52, titzer wrote:
> s/an/and/

Done.

[titzer, 2016-07-12 21:09:14]

lgtm

[Mircea Trofin, 2016-07-12 21:13:20]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2016-07-12 21:13:35]

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-07-12 21:37:15]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-07-12 21:37:30]

Description was changed from

==========
[wasm] cloning compiled module before instantiation

To correctly support instantiating a compiled module multiple times, we clone
the
compiled module each time we create an instance, since some of the data is
specific
to the instance - e.g. export code, wasm functions, indirect table.


BUG=v8:5072
==========

to

==========
[wasm] cloning compiled module before instantiation

To correctly support instantiating a compiled module multiple times, we clone
the
compiled module each time we create an instance, since some of the data is
specific
to the instance - e.g. export code, wasm functions, indirect table.

BUG=v8:5072

Committed: https://crrev.com/81f42220a63f8d29bde947171b1ed8e555034b53
Cr-Commit-Position: refs/heads/master@{#37692}
==========

[commit-bot: I haz the power, 2016-07-12 21:37:31]

Patchset 7 (id:??) landed as
https://crrev.com/81f42220a63f8d29bde947171b1ed8e555034b53
Cr-Commit-Position: refs/heads/master@{#37692}