[wasm] Implement importing of WebAssembly.Memory.

[wasm] Implement importing of WebAssembly.Memory.

R=mtrofin@chromium.org,gdeepti@chromium.org
BUG=chromium:575167
Committed: https://crrev.com/e3ff4cf8c9908ba06c38539db611d8ffb393cc2a
Cr-Commit-Position: refs/heads/master@{#40076}

[titzer, 2016-10-06 18:03:41]

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

[commit-bot: I haz the power, 2016-10-06 18:03:49]

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

[Mircea Trofin, 2016-10-06 18:10:44]

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

https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:56: kWasmMemObject,
Could this move on the compiled module object instead? I'd like to reduce
or eliminate the fields on the instance object. It seems like keeping 
information participating in instantiation, on this object, makes 
analysing and avoiding GC race conditions more difficult.

As an added benefit, we'd get strongly typed APIs.

[commit-bot: I haz the power, 2016-10-06 18:31:39]

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

[commit-bot: I haz the power, 2016-10-06 18:31:40]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-07 07:13:50]

On 2016/10/06 18:10:44, Mircea Trofin wrote:
> https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:56: kWasmMemObject,
> Could this move on the compiled module object instead? I'd like to reduce
> or eliminate the fields on the instance object. It seems like keeping 
> information participating in instantiation, on this object, makes 
> analysing and avoiding GC race conditions more difficult.
> 
> As an added benefit, we'd get strongly typed APIs.

Memory--especially the memory object--is inherently part of an instance. The
only reason that we keep a reference to memory on the compiled module is because
the code is specialized to a particular memory.

[titzer, 2016-10-07 07:30:06]

Description was changed from

==========
[wasm] Implement importing of WebAssembly.Memory.

R=mtrofin@chromium.org,gdeepti@chromium.org
BUG=chromium:575167
==========

to

==========
[wasm] Implement importing of WebAssembly.Memory.

R=mtrofin@chromium.org,gdeepti@chromium.org
BUG=chromium:575167
==========

[titzer, 2016-10-07 07:30:07]

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

[ahaas, 2016-10-07 08:23:36]

lgtm with nits

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

https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-js.cc#newcode744
src/wasm/wasm-js.cc:744: Maybe<bool> has_brand =
i::JSObject::HasOwnProperty(object, sym);
Can sym also be further up the prototype chain? If yes, I think you should not
use HasOwnProperty but JSReceiver::HasProperty.

https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-js.cc#newcode754
src/wasm/wasm-js.cc:754: Handle<Object>
buf(JSObject::cast(*value)->GetInternalField(0), isolate);
Could we use a constant for index 0?

https://codereview.chromium.org/2392943006/diff/1/test/mjsunit/wasm/import-me...
File test/mjsunit/wasm/import-memory.js (right):

https://codereview.chromium.org/2392943006/diff/1/test/mjsunit/wasm/import-me...
test/mjsunit/wasm/import-memory.js:13: assertEquals(65536,
memory.buffer.byteLength);
Can we put 65536 into a constant, maybe in wasm-constants.js?

[titzer, 2016-10-07 08:49:59]

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

[commit-bot: I haz the power, 2016-10-07 08:50:10]

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-07 08:50:53]

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

https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-js.cc#newcode744
src/wasm/wasm-js.cc:744: Maybe<bool> has_brand =
i::JSObject::HasOwnProperty(object, sym);
On 2016/10/07 08:23:36, ahaas wrote:
> Can sym also be further up the prototype chain? If yes, I think you should not
> use HasOwnProperty but JSReceiver::HasProperty.

Definitely don't want to go up the prototype chain. I copied a bit of code from
the BrandCheck() above.

https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-js.cc#newcode754
src/wasm/wasm-js.cc:754: Handle<Object>
buf(JSObject::cast(*value)->GetInternalField(0), isolate);
On 2016/10/07 08:23:36, ahaas wrote:
> Could we use a constant for index 0?

Done.

https://codereview.chromium.org/2392943006/diff/1/test/mjsunit/wasm/import-me...
File test/mjsunit/wasm/import-memory.js (right):

https://codereview.chromium.org/2392943006/diff/1/test/mjsunit/wasm/import-me...
test/mjsunit/wasm/import-memory.js:13: assertEquals(65536,
memory.buffer.byteLength);
On 2016/10/07 08:23:36, ahaas wrote:
> Can we put 65536 into a constant, maybe in wasm-constants.js?

Done.

[commit-bot: I haz the power, 2016-10-07 09:28:42]

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

[commit-bot: I haz the power, 2016-10-07 09:28:42]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-07 09:31:13]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-10-07 09:31:14]

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

[commit-bot: I haz the power, 2016-10-07 09:31:18]

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-07 09:34:11]

Description was changed from

==========
[wasm] Implement importing of WebAssembly.Memory.

R=mtrofin@chromium.org,gdeepti@chromium.org
BUG=chromium:575167
==========

to

==========
[wasm] Implement importing of WebAssembly.Memory.

R=mtrofin@chromium.org,gdeepti@chromium.org
BUG=chromium:575167
==========

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

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-07 09:34:36]

Description was changed from

==========
[wasm] Implement importing of WebAssembly.Memory.

R=mtrofin@chromium.org,gdeepti@chromium.org
BUG=chromium:575167
==========

to

==========
[wasm] Implement importing of WebAssembly.Memory.

R=mtrofin@chromium.org,gdeepti@chromium.org
BUG=chromium:575167

Committed: https://crrev.com/e3ff4cf8c9908ba06c38539db611d8ffb393cc2a
Cr-Commit-Position: refs/heads/master@{#40076}
==========

[commit-bot: I haz the power, 2016-10-07 09:34:37]

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

[Mircea Trofin, 2016-10-07 15:07:16]

On 2016/10/07 07:13:50, titzer wrote:
> On 2016/10/06 18:10:44, Mircea Trofin wrote:
> > https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-module.cc
> > File src/wasm/wasm-module.cc (right):
> > 
> >
>
https://codereview.chromium.org/2392943006/diff/1/src/wasm/wasm-module.cc#new...
> > src/wasm/wasm-module.cc:56: kWasmMemObject,
> > Could this move on the compiled module object instead? I'd like to reduce
> > or eliminate the fields on the instance object. It seems like keeping 
> > information participating in instantiation, on this object, makes 
> > analysing and avoiding GC race conditions more difficult.
> > 
> > As an added benefit, we'd get strongly typed APIs.
> 
> Memory--especially the memory object--is inherently part of an instance. The
> only reason that we keep a reference to memory on the compiled module is
because
> the code is specialized to a particular memory.

Good point. Maybe we can use this as a pivot for separating data between
compiled 
module and owning instance (but I have to think a bit more how that'd work).