WIP SharedArrayBuffer implementation

WIP SharedArrayBuffer implementation

BUG=none
R=jarin@chromium.org

[binji, 2015-04-09 00:10:54]

PTAL, this is some initial work I've done to add SharedArrayBuffer.

It's not clear to me how much code should be shared between SharedArrayBuffer
and ArrayBuffer at all of the different levels. What I've gone for here is to
create a new SharedArrayBuffer, Shared<Type>Array external API interface and
then share code in JSArray + JSTypedArray by adding an is_shared flag.

Does this seem like a reasonable direction?

[binji, 2015-04-09 17:22:49]

+sehr, bradnelson

[Jarin, 2015-04-13 07:21:20]

jarin@chromium.org changed reviewers:
+ dslomov@chromium.org

[Jarin, 2015-04-13 07:23:53]

Adding dslomov as a reviewer because he is our typed array expert.

[Jarin, 2015-04-16 08:40:28]

jarin@chromium.org changed reviewers:
+ jochen@chromium.org, verwaest@chromium.org
- dslomov@chromium.org

[Jarin, 2015-04-16 08:48:29]

jarin@chromium.org changed reviewers:
- verwaest@chromium.org

[jochen (gone - plz use gerrit), 2015-04-16 14:48:27]

fyi, i'm currently rewriting how we keep track of array buffers and views

the tl;dr version is that we'll get rid of all the weak pointers and stop having
different maps for on-heap and external buffers

[Jarin, 2015-04-16 14:55:28]

The overall direction looks good. Thanks for working on this!

I am not too happy about the duplication of the classes in the API, so I have
added Jochen to comment on this.

Before this is landed, there should be some better test coverage of all this.
What are your plans here?

(I am also assuming that this will be hidden behind a flag.)

https://codereview.chromium.org/1069883002/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1069883002/diff/1/src/bootstrapper.cc#newcode194
src/bootstrapper.cc:194: bool is_shared);
Could you introduce an enum for the various places where you pass the is_shared
flag? That would make it easier to read the code at call sites.

https://codereview.chromium.org/1069883002/diff/1/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/1069883002/diff/1/src/factory.h#newcode446
src/factory.h:446: Handle<JSArrayBuffer> NewJSArrayBuffer(bool is_shared =
false);
Please use an enum here and below (rather than a bool).

https://codereview.chromium.org/1069883002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1069883002/diff/1/src/objects.h#newcode10265
src/objects.h:10265: static const int kIsSharedBit = 3;
Why do you need the bit? Cannot you get the sharedness from the map?

https://codereview.chromium.org/1069883002/diff/1/src/objects.h#newcode10328
src/objects.h:10328: static const int kIsSharedOffset = kLengthOffset +
kPointerSize;
Again, why do you need the shared flag? Don't you have it on the map already?

[binji, 2015-04-16 21:00:34]

https://codereview.chromium.org/1069883002/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1069883002/diff/1/src/bootstrapper.cc#newcode194
src/bootstrapper.cc:194: bool is_shared);
On 2015/04/16 14:55:28, jarin wrote:
> Could you introduce an enum for the various places where you pass the
is_shared
> flag? That would make it easier to read the code at call sites.

Done.

https://codereview.chromium.org/1069883002/diff/1/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/1069883002/diff/1/src/factory.h#newcode446
src/factory.h:446: Handle<JSArrayBuffer> NewJSArrayBuffer(bool is_shared =
false);
On 2015/04/16 14:55:28, jarin wrote:
> Please use an enum here and below (rather than a bool).

Done.

https://codereview.chromium.org/1069883002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1069883002/diff/1/src/objects.h#newcode10265
src/objects.h:10265: static const int kIsSharedBit = 3;
On 2015/04/16 14:55:28, jarin wrote:
> Why do you need the bit? Cannot you get the sharedness from the map?

Do you mean from the ExternalArrayType? I currently share that enumeration value
between TypedArrays and SharedTypedArrays. I could add new shared types, but I'm
not sure how much code would need to be duplicated then. Also, I think that will
only work for TypedArrays, not for the SharedArrayBuffer.

Or am I missing something? (quite likely :)

https://codereview.chromium.org/1069883002/diff/1/src/objects.h#newcode10328
src/objects.h:10328: static const int kIsSharedOffset = kLengthOffset +
kPointerSize;
On 2015/04/16 14:55:28, jarin wrote:
> Again, why do you need the shared flag? Don't you have it on the map already?

see above

[Jarin, 2015-04-17 07:49:27]

https://codereview.chromium.org/1069883002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1069883002/diff/1/src/objects.h#newcode10265
src/objects.h:10265: static const int kIsSharedBit = 3;
On 2015/04/16 21:00:34, binji wrote:
> On 2015/04/16 14:55:28, jarin wrote:
> > Why do you need the bit? Cannot you get the sharedness from the map?
> 
> Do you mean from the ExternalArrayType? I currently share that enumeration
value
> between TypedArrays and SharedTypedArrays. I could add new shared types, but
I'm
> not sure how much code would need to be duplicated then. Also, I think that
will
> only work for TypedArrays, not for the SharedArrayBuffer.
> 
> Or am I missing something? (quite likely :)

I was just wondering why you could not define is_shared()
{ return map()->instance_type() == JS_SHARED_ARRAY_BUFFER_TYPE; }

[binji, 2015-04-17 09:06:44]

I'll work on some additional tests next.

I think it's worth adding tests for constructing
SharedArrayBuffers/SharedTypedArrays, ensuring that they ArrayBufferViews cannot
be used on SharedArrayBuffers and vice-versa. Testing the "shared" part is not
possible as it is more of a DOM feature requiring Workers and postMessage.

Is it worth copying the typedarray cctests/mjsunit tests and substituting
s/ArrayBuffer/SharedArrayBuffer/? I'm leaning toward yes; it's not nice to have
duplicated code, but it seems wrong for the tests to assume a shared
implementation. What do you think?

https://codereview.chromium.org/1069883002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1069883002/diff/1/src/objects.h#newcode10265
src/objects.h:10265: static const int kIsSharedBit = 3;
On 2015/04/17 07:49:27, jarin wrote:
> On 2015/04/16 21:00:34, binji wrote:
> > On 2015/04/16 14:55:28, jarin wrote:
> > > Why do you need the bit? Cannot you get the sharedness from the map?
> > 
> > Do you mean from the ExternalArrayType? I currently share that enumeration
> value
> > between TypedArrays and SharedTypedArrays. I could add new shared types, but
> I'm
> > not sure how much code would need to be duplicated then. Also, I think that
> will
> > only work for TypedArrays, not for the SharedArrayBuffer.
> > 
> > Or am I missing something? (quite likely :)
> 
> I was just wondering why you could not define is_shared()
> { return map()->instance_type() == JS_SHARED_ARRAY_BUFFER_TYPE; }

Thanks, that works. I knew I was missing something. :)

[binji, 2015-04-22 17:38:24]

On 2015/04/17 09:06:44, binji wrote:
> I'll work on some additional tests next.
> 
> I think it's worth adding tests for constructing
> SharedArrayBuffers/SharedTypedArrays, ensuring that they ArrayBufferViews
cannot
> be used on SharedArrayBuffers and vice-versa. Testing the "shared" part is not
> possible as it is more of a DOM feature requiring Workers and postMessage.
> 
> Is it worth copying the typedarray cctests/mjsunit tests and substituting
> s/ArrayBuffer/SharedArrayBuffer/? I'm leaning toward yes; it's not nice to
have
> duplicated code, but it seems wrong for the tests to assume a shared
> implementation. What do you think?

I've duplicated some mjsunit tests and fixed some issues that those found.

I understand Jochen is working on some ArrayBuffer refactoring, so I'll continue
to rebase
this patch until he's landed all his changes.

[bradn, 2015-04-28 15:55:34]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2015-04-28 15:55:35]

Gentle ping.
Ben plans to rebase as jochen refactors.
More feedback would be helpful.
Thanks :-)

[jochen (gone - plz use gerrit), 2015-04-28 18:31:46]

some comments

it's unfortunate that the shared thingies have a different map - that means that
code that handles both regular and shared typed arrays will go polymorphic (i.e.
ultra slow)

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h#newcode3684
include/v8.h:3684: class V8_EXPORT Allocator {  // NOLINT
why can't we use the allocator form ArrayBuffer?

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h#newcode3751
include/v8.h:3751: size_t byte_length);
we should have a version that creates an internalized shared array buffer

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h#newcode3769
include/v8.h:3769: Contents Externalize();
also add GetContents() that gets the contents without externalizing

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h#newcode6081
include/v8.h:6081: static void SetSharedArrayBufferAllocator(
the allocator should be per isolate and passed in via CreateParams. I'll change
the array buffer one soon to follow this pattern.

https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc#newco...
src/accessors.cc:103: case JS_SHARED_TYPED_ARRAY_TYPE:
can shared array buffers be neutered?

https://codereview.chromium.org/1069883002/diff/220001/src/harmony-sharedtype...
File src/harmony-sharedtypedarray.js (right):

https://codereview.chromium.org/1069883002/diff/220001/src/harmony-sharedtype...
src/harmony-sharedtypedarray.js:86: %_TypedArrayInitialize(obj, ARRAY_ID, null,
0, byteLength);
what's the point of having an on-heap shared typed array?

https://codereview.chromium.org/1069883002/diff/220001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1069883002/diff/220001/src/objects.h#newcode1...
src/objects.h:10267: inline SharedFlag is_shared();
why is that not just a bool like the other bits?

https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
File src/runtime/runtime-typedarray.cc (right):

https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
src/runtime/runtime-typedarray.cc:201: DCHECK(!array_buffer->is_shared());
why? What about ArrayBuffer.transfer?

https://codereview.chromium.org/1069883002/diff/220001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/1069883002/diff/220001/tools/gyp/v8.gyp#newco...
tools/gyp/v8.gyp:1733: '../../src/harmony-sharedtypedarray.js',
please also update BUILD.gn

[Dmitry Lomov (no reviews), 2015-04-28 19:39:05]

On 2015/04/28 18:31:46, jochen wrote:
> some comments
> 
> it's unfortunate that the shared thingies have a different map - that means
that
> code that handles both regular and shared typed arrays will go polymorphic
(i.e.
> ultra slow)
 
Different maps (and having different code) is very much justified here: shared
array buffer accesses MUST be different since they give memory model guarantees.

[jochen (gone - plz use gerrit), 2015-04-28 19:42:09]

On 2015/04/28 at 19:39:05, dslomov wrote:
> On 2015/04/28 18:31:46, jochen wrote:
> > some comments
> > 
> > it's unfortunate that the shared thingies have a different map - that means
that
> > code that handles both regular and shared typed arrays will go polymorphic
(i.e.
> > ultra slow)
>  
> Different maps (and having different code) is very much justified here: shared
array buffer accesses MUST be different since they give memory model guarantees.

ok, but in the current patch, there's no difference yet..

[Jarin, 2015-04-29 04:45:19]

On 2015/04/28 19:42:09, jochen wrote:
> On 2015/04/28 at 19:39:05, dslomov wrote:
> > On 2015/04/28 18:31:46, jochen wrote:
> > > some comments
> > > 
> > > it's unfortunate that the shared thingies have a different map - that
means
> that
> > > code that handles both regular and shared typed arrays will go polymorphic
> (i.e.
> > > ultra slow)
> >  
> > Different maps (and having different code) is very much justified here:
shared
> array buffer accesses MUST be different since they give memory model
guarantees.
> 
> ok, but in the current patch, there's no difference yet..

SharedArrayBuffers should be only used in asm.js code and such code will be
specialized for the specific buffer anyway, so that should not be a big problem.

[Jarin, 2015-04-29 04:46:01]

https://codereview.chromium.org/1069883002/diff/220001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1069883002/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1377: is_shared ? JS_SHARED_TYPED_ARRAY_TYPE :
JS_TYPED_ARRAY_TYPE;
change to (is_shared == SHARED) ? ...

https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
File src/runtime/runtime-typedarray.cc (right):

https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
src/runtime/runtime-typedarray.cc:79: bool is_shared =
array_buffer->is_shared();
This should be array_buffer->is_shared() == SHARED. Or just make
JSArrayBuffer::is_shared return bool, the main motivation for the SharedFlag is
to make the sharing explicit when passed as a parameter.

(That said, I am a bit surprised that C++ is happy about using enum as bool.)

https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
src/runtime/runtime-typedarray.cc:201: DCHECK(!array_buffer->is_shared());
On 2015/04/28 18:31:46, jochen wrote:
> why? What about ArrayBuffer.transfer?

The whole idea of SharedArrayBuffer is that it can be shared, i.e., not neutered
on transfer.

[jochen (gone - plz use gerrit), 2015-04-29 07:29:32]

On 2015/04/29 at 04:45:19, jarin wrote:
> On 2015/04/28 19:42:09, jochen wrote:
> > On 2015/04/28 at 19:39:05, dslomov wrote:
> > > On 2015/04/28 18:31:46, jochen wrote:
> > > > some comments
> > > > 
> > > > it's unfortunate that the shared thingies have a different map - that
means
> > that
> > > > code that handles both regular and shared typed arrays will go
polymorphic
> > (i.e.
> > > > ultra slow)
> > >  
> > > Different maps (and having different code) is very much justified here:
shared
> > array buffer accesses MUST be different since they give memory model
guarantees.
> > 
> > ok, but in the current patch, there's no difference yet..
> 
> SharedArrayBuffers should be only used in asm.js code and such code will be
specialized for the specific buffer anyway, so that should not be a big problem.

well, it's a full-blown language feature, so it has to work everywhere.

and ArrayBuffer.transfer is a "for-asm.js" feature... it's used for this change
heap thing.

otoh, if you can't neuter them, then they shouldn't use the array buffer code
that checks for neutering all over the place.

[jochen (gone - plz use gerrit), 2015-04-29 07:30:18]

i'd also be interested in what happens if you postMessage an array buffer in
blink. Or if any API that expects some form of serialized script value gets an
shared array buffer?

[Jarin, 2015-04-29 08:03:28]

On 2015/04/29 07:30:18, jochen wrote:
> i'd also be interested in what happens if you postMessage an array buffer in
> blink. Or if any API that expects some form of serialized script value gets an
> shared array buffer?

We should find a way for the shared array buffers not to spread anywhere beyond
JavaScript (ideally, SharedArrayBuffers should be confined to asm.js).

[jochen (gone - plz use gerrit), 2015-04-29 08:05:58]

On 2015/04/29 at 08:03:28, jarin wrote:
> On 2015/04/29 07:30:18, jochen wrote:
> > i'd also be interested in what happens if you postMessage an array buffer in
> > blink. Or if any API that expects some form of serialized script value gets
an
> > shared array buffer?
> 
> We should find a way for the shared array buffers not to spread anywhere
beyond JavaScript (ideally, SharedArrayBuffers should be confined to asm.js).

there isn't really anything beyond javascript :)

[binji, 2015-04-29 18:27:22]

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h#newcode3684
include/v8.h:3684: class V8_EXPORT Allocator {  // NOLINT
On 2015/04/28 18:31:46, jochen wrote:
> why can't we use the allocator form ArrayBuffer?

Done.

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h#newcode3751
include/v8.h:3751: size_t byte_length);
On 2015/04/28 18:31:46, jochen wrote:
> we should have a version that creates an internalized shared array buffer

Done.

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h#newcode3769
include/v8.h:3769: Contents Externalize();
On 2015/04/28 18:31:46, jochen wrote:
> also add GetContents() that gets the contents without externalizing

Done.

https://codereview.chromium.org/1069883002/diff/220001/include/v8.h#newcode6081
include/v8.h:6081: static void SetSharedArrayBufferAllocator(
On 2015/04/28 18:31:46, jochen wrote:
> the allocator should be per isolate and passed in via CreateParams. I'll
change
> the array buffer one soon to follow this pattern.

Done.

https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc#newco...
src/accessors.cc:103: case JS_SHARED_TYPED_ARRAY_TYPE:
On 2015/04/28 18:31:46, jochen wrote:
> can shared array buffers be neutered?

no, does that affect the code here?

https://codereview.chromium.org/1069883002/diff/220001/src/harmony-sharedtype...
File src/harmony-sharedtypedarray.js (right):

https://codereview.chromium.org/1069883002/diff/220001/src/harmony-sharedtype...
src/harmony-sharedtypedarray.js:86: %_TypedArrayInitialize(obj, ARRAY_ID, null,
0, byteLength);
On 2015/04/28 18:31:46, jochen wrote:
> what's the point of having an on-heap shared typed array?

Done.

https://codereview.chromium.org/1069883002/diff/220001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1069883002/diff/220001/src/objects.h#newcode1...
src/objects.h:10267: inline SharedFlag is_shared();
On 2015/04/28 18:31:46, jochen wrote:
> why is that not just a bool like the other bits?

Done.

https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
File src/runtime/runtime-typedarray.cc (right):

https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
src/runtime/runtime-typedarray.cc:79: bool is_shared =
array_buffer->is_shared();
On 2015/04/29 04:46:01, jarin wrote:
> This should be array_buffer->is_shared() == SHARED. Or just make
> JSArrayBuffer::is_shared return bool, the main motivation for the SharedFlag
is
> to make the sharing explicit when passed as a parameter.
> 
> (That said, I am a bit surprised that C++ is happy about using enum as bool.)

Done.

https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
src/runtime/runtime-typedarray.cc:201: DCHECK(!array_buffer->is_shared());
On 2015/04/28 18:31:46, jochen wrote:
> why? What about ArrayBuffer.transfer?

I asked about this on the spec doc, but I think it doesn't make much sense to
add a SharedArrayBuffer.transfer. How would it work if the buffer's backing
store is already being shared across isolates? Either you'd need to have a
synchronization point to swap in the new backing store for all threads (in which
case you wouldn't neuter) or you'd have to neuter on one thread and eventually
share the new buffer with the other threads (which wouldn't help with memory
pressure).

AIUI, the only other case for neutering is transferring an ArrayBuffer to a
Worker, which is the method for sharing a SAB, and does not neuter.

https://codereview.chromium.org/1069883002/diff/220001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/1069883002/diff/220001/tools/gyp/v8.gyp#newco...
tools/gyp/v8.gyp:1733: '../../src/harmony-sharedtypedarray.js',
On 2015/04/28 18:31:46, jochen wrote:
> please also update BUILD.gn

Done.

[binji, 2015-04-29 21:50:35]

On 2015/04/29 07:30:18, jochen wrote:
> i'd also be interested in what happens if you postMessage an array buffer in
> blink. Or if any API that expects some form of serialized script value gets an
> shared array buffer?

I have a WIP CL here: https://codereview.chromium.org/1097773004/

The current plan is that any API that expects an ArrayBuffer will not work with
a SharedArrayBuffer. AIUI, that restriction may be loosened in the future on an
API-by-API basis.

[jochen (gone - plz use gerrit), 2015-04-30 07:07:59]

On 2015/04/29 at 18:27:22, binji wrote:
> https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc
> File src/accessors.cc (right):
> 
>
https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc#newco...
> src/accessors.cc:103: case JS_SHARED_TYPED_ARRAY_TYPE:
> On 2015/04/28 18:31:46, jochen wrote:
> > can shared array buffers be neutered?
> 
> no, does that affect the code here?

JSArrayBufferViewFieldAccessors include a check whether the underlying
ArrayBuffer is neutered. Also, they blindly assume that the types all derive
from JSArrayBufferView.

And so do the elements() accesses in hydrogen.cc


>
https://codereview.chromium.org/1069883002/diff/220001/src/runtime/runtime-ty...
> src/runtime/runtime-typedarray.cc:201: DCHECK(!array_buffer->is_shared());
> On 2015/04/28 18:31:46, jochen wrote:
> > why? What about ArrayBuffer.transfer?
> 
> I asked about this on the spec doc, but I think it doesn't make much sense to
add a SharedArrayBuffer.transfer. How would it work if the buffer's backing
store is already being shared across isolates? Either you'd need to have a
synchronization point to swap in the new backing store for all threads (in which
case you wouldn't neuter) or you'd have to neuter on one thread and eventually
share the new buffer with the other threads (which wouldn't help with memory
pressure).
> 
> AIUI, the only other case for neutering is transferring an ArrayBuffer to a
Worker, which is the method for sharing a SAB, and does not neuter.

it could detach your local copy and give you a new one that is no longer shared
across isolates. dunno. it could just not support changing size.

[jochen (gone - plz use gerrit), 2015-04-30 07:11:34]

On 2015/04/29 at 21:50:35, binji wrote:
> On 2015/04/29 07:30:18, jochen wrote:
> > i'd also be interested in what happens if you postMessage an array buffer in
> > blink. Or if any API that expects some form of serialized script value gets
an
> > shared array buffer?
> 
> I have a WIP CL here: https://codereview.chromium.org/1097773004/
> 
> The current plan is that any API that expects an ArrayBuffer will not work
with a SharedArrayBuffer. AIUI, that restriction may be loosened in the future
on an API-by-API basis.

what about v8 value converter in //src/content/public/child and
gin_java_bridge_value_converter in //src/content/renderer/java

[binji, 2015-04-30 16:21:38]

On 2015/04/30 07:07:59, jochen wrote:
> On 2015/04/29 at 18:27:22, binji wrote:
> > https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc
> > File src/accessors.cc (right):
> > 
> >
>
https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc#newco...
> > src/accessors.cc:103: case JS_SHARED_TYPED_ARRAY_TYPE:
> > On 2015/04/28 18:31:46, jochen wrote:
> > > can shared array buffers be neutered?
> > 
> > no, does that affect the code here?
> 
> JSArrayBufferViewFieldAccessors include a check whether the underlying
> ArrayBuffer is neutered. Also, they blindly assume that the types all derive
> from JSArrayBufferView.
> 
> And so do the elements() accesses in hydrogen.cc

This should be OK, I think. SharedArrayBuffer and SharedTypedArrays use
JSArrayBuffer/JSArrayBufferView/JSTypedArray under the hood.

It's unfortunate that SAB don't support neutering while the JSArrayBuffers do,
but duplicating the JSArrayBuffer code would be worse IMO. As long as the
external API blocks all attempts to neuter it should be fine, right? As far as
JSArrayBufferViewFieldAccessors is concerned, it will check whether a SAB is
neutered, but it will never be true.

[binji, 2015-04-30 16:39:14]

On 2015/04/30 07:11:34, jochen wrote:
> On 2015/04/29 at 21:50:35, binji wrote:
> > On 2015/04/29 07:30:18, jochen wrote:
> > > i'd also be interested in what happens if you postMessage an array buffer
in
> > > blink. Or if any API that expects some form of serialized script value
gets
> an
> > > shared array buffer?
> > 
> > I have a WIP CL here: https://codereview.chromium.org/1097773004/
> > 
> > The current plan is that any API that expects an ArrayBuffer will not work
> with a SharedArrayBuffer. AIUI, that restriction may be loosened in the future
> on an API-by-API basis.
> 
> what about v8 value converter in //src/content/public/child and
> gin_java_bridge_value_converter in //src/content/renderer/java

I took a look at both of these, and they use
Value::Is{ArrayBuffer,ArrayBufferView,TypedArray} to decide whether to convert.
These return false for SAB. Actually, it didn't return false for
JSArrayBufferView, so I've fixed that in the CL.

[binji, 2015-04-30 16:39:43]

https://codereview.chromium.org/1069883002/diff/220001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1069883002/diff/220001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1377: is_shared ? JS_SHARED_TYPED_ARRAY_TYPE :
JS_TYPED_ARRAY_TYPE;
On 2015/04/29 04:46:00, jarin wrote:
> change to (is_shared == SHARED) ? ...

Done.

[jochen (gone - plz use gerrit), 2015-05-04 07:05:22]

On 2015/04/30 at 16:21:38, binji wrote:
> On 2015/04/30 07:07:59, jochen wrote:
> > On 2015/04/29 at 18:27:22, binji wrote:
> > > https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc
> > > File src/accessors.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc#newco...
> > > src/accessors.cc:103: case JS_SHARED_TYPED_ARRAY_TYPE:
> > > On 2015/04/28 18:31:46, jochen wrote:
> > > > can shared array buffers be neutered?
> > > 
> > > no, does that affect the code here?
> > 
> > JSArrayBufferViewFieldAccessors include a check whether the underlying
> > ArrayBuffer is neutered. Also, they blindly assume that the types all derive
> > from JSArrayBufferView.
> > 
> > And so do the elements() accesses in hydrogen.cc
> 
> This should be OK, I think. SharedArrayBuffer and SharedTypedArrays use
JSArrayBuffer/JSArrayBufferView/JSTypedArray under the hood.

It's unnecessarily slow, and, as far as I understand the spec, not the correct
memory model for SAB?

> 
> It's unfortunate that SAB don't support neutering while the JSArrayBuffers do,
but duplicating the JSArrayBuffer code would be worse IMO. As long as the
external API blocks all attempts to neuter it should be fine, right? As far as
JSArrayBufferViewFieldAccessors is concerned, it will check whether a SAB is
neutered, but it will never be true.

[jochen (gone - plz use gerrit), 2015-05-05 15:27:29]

I tried to think a bit about how we could make progress here.

What is essentially unclear to me is how the memory model, atomics, etc.. will
be implemented.

On a high level, this CL just introduces an alias for ArrayBuffer et al without
any functional change. However, if that's the way it's supposed to be in the
end, we can have this with far less changes to V8.

So maybe we should instead first do the really hairy bits, e.g. teach the three
compiles to do atomic accesses?

I think before we add SharedArrayBuffer as done in this CL, we should also
figure out the desired interaction with Blink and Chrome.

[binji, 2015-05-05 15:46:42]

On 2015/05/05 15:27:29, jochen wrote:
> I tried to think a bit about how we could make progress here.
> 
> What is essentially unclear to me is how the memory model, atomics, etc.. will
> be implemented.

My understanding of the memory model is this: normal SAB accesses work the same
as
ArrayBuffer accesses. Any other guarantees would be too slow. It's up to the
developer
to correctly synchronize their application via the Atomics API. The suggestions
I've
gotten for implementation are to use
http://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html
for the atomics instruction sequences.

> 
> On a high level, this CL just introduces an alias for ArrayBuffer et al
without
> any functional change. However, if that's the way it's supposed to be in the
> end, we can have this with far less changes to V8.

As I understand it, SAB at the V8 level isn't supposed to have different
behavior,
but it needs to be distinguishable from an ArrayBuffer/TypedArray at all levels,
and it needs to have a different API. The Atomics API only works on SAB, for
example. If we can do this with a smaller CL, that would be great!

> 
> So maybe we should instead first do the really hairy bits, e.g. teach the
three
> compiles to do atomic accesses?

I'm working on a CL for that here: https://codereview.chromium.org/1124813005/.
I'm just using the __atomic_* builtins for now, for simplicity.

> 
> I think before we add SharedArrayBuffer as done in this CL, we should also
> figure out the desired interaction with Blink and Chrome.

Please take a look at the CL here: https://codereview.chromium.org/1097773004/

I think the desired behavior in Blink is that a SharedArrayBuffer works almost
exactly like an ArrayBuffer (with a smaller API). It must not be allowed to be
used where an ArrayBuffer can be used. It also can be shared with a Worker by
including it in the "transfer" argument of worker.postMessage.

[binji, 2015-05-05 15:54:05]

On 2015/05/04 07:05:22, jochen wrote:
> On 2015/04/30 at 16:21:38, binji wrote:
> > On 2015/04/30 07:07:59, jochen wrote:
> > > On 2015/04/29 at 18:27:22, binji wrote:
> > > > https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc
> > > > File src/accessors.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1069883002/diff/220001/src/accessors.cc#newco...
> > > > src/accessors.cc:103: case JS_SHARED_TYPED_ARRAY_TYPE:
> > > > On 2015/04/28 18:31:46, jochen wrote:
> > > > > can shared array buffers be neutered?
> > > > 
> > > > no, does that affect the code here?
> > > 
> > > JSArrayBufferViewFieldAccessors include a check whether the underlying
> > > ArrayBuffer is neutered. Also, they blindly assume that the types all
derive
> > > from JSArrayBufferView.
> > > 
> > > And so do the elements() accesses in hydrogen.cc
> > 
> > This should be OK, I think. SharedArrayBuffer and SharedTypedArrays use
> JSArrayBuffer/JSArrayBufferView/JSTypedArray under the hood.
> 
> It's unnecessarily slow, and, as far as I understand the spec, not the correct
> memory model for SAB?

OK, I see. I don't really understand how the hydrogen stuff works; is it
possible to get the map/instance_type and skip the neuter check instruction? It
seems like the instance_type is used elsewhere in the file, but I can't see how
to get the map from the values available in
BuildUncheckedMonomorphicElementAccess for example.

[jochen (gone - plz use gerrit), 2015-05-05 16:06:19]

If a SAB behaves like an AB of you don't use the correct atomics, we could
basically allow atomic operations on ABs as well - since they can't be shared,
nothing can go wrong.

It sounds like all we need is a bunch of constructors that create regular ABs
and set a bit in addition that this AB can be shared.

Then we only need to check this bit when we need to decide whether to neuter or
share the AB for postMessage.

[binji, 2015-05-05 16:14:22]

On 2015/05/05 16:06:19, jochen wrote:
> If a SAB behaves like an AB of you don't use the correct atomics, we could
> basically allow atomic operations on ABs as well - since they can't be shared,
> nothing can go wrong.

True.

> 
> It sounds like all we need is a bunch of constructors that create regular ABs
> and set a bit in addition that this AB can be shared.

Yes, that is how ncbray@'s prototype worked. But if a SAB is just an AB, then it
means that a SAB can be passed to APIs that expect ABs, which we don't want.
Similarly, we don't want all the ArrayBuffer APIs to be available to SAB (e.g.
ArrayBuffer.slice, ArrayBuffer.transfer, etc.)

Or are you thinking that SAB is a type that is only available to Blink, not v8?
Perhaps that is possible -- though I'm not sure how the v8 type <-> Blink type
coercion works if the v8 type is shared between two Blink types.

> 
> Then we only need to check this bit when we need to decide whether to neuter
or
> share the AB for postMessage.

[binji, 2015-05-05 17:52:43]

On 2015/05/05 16:14:22, binji wrote:
> On 2015/05/05 16:06:19, jochen wrote:
> > If a SAB behaves like an AB of you don't use the correct atomics, we could
> > basically allow atomic operations on ABs as well - since they can't be
shared,
> > nothing can go wrong.
> 
> True.
> 
> > 
> > It sounds like all we need is a bunch of constructors that create regular
ABs
> > and set a bit in addition that this AB can be shared.
> 
> Yes, that is how ncbray@'s prototype worked. But if a SAB is just an AB, then
it
> means that a SAB can be passed to APIs that expect ABs, which we don't want.
> Similarly, we don't want all the ArrayBuffer APIs to be available to SAB (e.g.
> ArrayBuffer.slice, ArrayBuffer.transfer, etc.)
> 
> Or are you thinking that SAB is a type that is only available to Blink, not
v8?
> Perhaps that is possible -- though I'm not sure how the v8 type <-> Blink type
> coercion works if the v8 type is shared between two Blink types.

I should also mention that we may need to store a mutex per SAB to handle atomic
accesses on architectures that don't support 64-bit atomics. Similarly, we may
need a data structure on the SAB to track futex waiters. There are other
possible implementations, but in general I think we will need some additional
data structures on SAB types. We probably don't want to require those for
ArrayBuffers as well.

> 
> > 
> > Then we only need to check this bit when we need to decide whether to neuter
> or
> > share the AB for postMessage.

[Jarin, 2015-05-06 06:27:06]

On 2015/05/05 17:52:43, binji wrote:
> On 2015/05/05 16:14:22, binji wrote:
> > On 2015/05/05 16:06:19, jochen wrote:
> > > If a SAB behaves like an AB of you don't use the correct atomics, we could
> > > basically allow atomic operations on ABs as well - since they can't be
> shared,
> > > nothing can go wrong.
> > 
> > True.
> > 
> > > 
> > > It sounds like all we need is a bunch of constructors that create regular
> ABs
> > > and set a bit in addition that this AB can be shared.
> > 
> > Yes, that is how ncbray@'s prototype worked. But if a SAB is just an AB,
then
> it
> > means that a SAB can be passed to APIs that expect ABs, which we don't want.
> > Similarly, we don't want all the ArrayBuffer APIs to be available to SAB
(e.g.
> > ArrayBuffer.slice, ArrayBuffer.transfer, etc.)
> > 
> > Or are you thinking that SAB is a type that is only available to Blink, not
> v8?
> > Perhaps that is possible -- though I'm not sure how the v8 type <-> Blink
type
> > coercion works if the v8 type is shared between two Blink types.
> 
> I should also mention that we may need to store a mutex per SAB to handle
atomic
> accesses on architectures that don't support 64-bit atomics. Similarly, we may
> need a data structure on the SAB to track futex waiters. There are other
> possible implementations, but in general I think we will need some additional
> data structures on SAB types. We probably don't want to require those for
> ArrayBuffers as well.
> 
> > 
> > > 
> > > Then we only need to check this bit when we need to decide whether to
neuter
> > or
> > > share the AB for postMessage.

I think the complexity of this is getting a bit out of hands. Could we possibly
have a separate design document which outlines what needs to be done?

The document should describe at least the following:

1. Representation of SAB in v8.

2. Compilation of non-atomic accesses to SAB. (Probably just like normal
accesses to AB, but if the SAB have different map as in the current version, one
has to be careful about map checks/polymorphism in hydrogen and ICs.)

3. Compilation of atomic accesses to SAB. This should discuss all the compilers:
full code, hydrogen and turbofan. My guess is we would be fine with having
runtime functions for full code and hydrogen, but turbofan needs to have
optimized version. This should also discuss architecture specific details (such
as implementation on architectures without 64-bit atomic instructions).

4. Compilation of futexes.

5. Discussion of neutering and friends.

6. API design, interface to the rest of chrome.

I am sure I am missing other important aspects.

[Jarin, 2015-05-06 06:31:22]

On 2015/05/05 16:06:19, jochen wrote:
> If a SAB behaves like an AB of you don't use the correct atomics, we could
> basically allow atomic operations on ABs as well - since they can't be shared,
> nothing can go wrong.

The spec says atomic ops are supposed to throw TypeError on non-SAB arguments.

[Jarin, 2015-05-06 06:35:43]

> OK, I see. I don't really understand how the hydrogen stuff works; is it
> possible to get the map/instance_type and skip the neuter check instruction?
It
> seems like the instance_type is used elsewhere in the file, but I can't see
how
> to get the map from the values available in
> BuildUncheckedMonomorphicElementAccess for example.

I do not think we have to worry about hydrogen because the main use case for
SABs is asm.js and all asm.js code is executed by turbofan.

We do have to worry about turbofan and this is where things become interesting.

[binji, 2015-05-06 16:53:23]

> I think the complexity of this is getting a bit out of hands. Could we
possibly
> have a separate design document which outlines what needs to be done?
> 
> The document should describe at least the following:
> 
> 1. Representation of SAB in v8.
> 
> 2. Compilation of non-atomic accesses to SAB. (Probably just like normal
> accesses to AB, but if the SAB have different map as in the current version,
one
> has to be careful about map checks/polymorphism in hydrogen and ICs.)
> 
> 3. Compilation of atomic accesses to SAB. This should discuss all the
compilers:
> full code, hydrogen and turbofan. My guess is we would be fine with having
> runtime functions for full code and hydrogen, but turbofan needs to have
> optimized version. This should also discuss architecture specific details
(such
> as implementation on architectures without 64-bit atomic instructions).
> 
> 4. Compilation of futexes.
> 
> 5. Discussion of neutering and friends.
> 
> 6. API design, interface to the rest of chrome.
> 
> I am sure I am missing other important aspects.

Sorry for the confusion. You're right, it will be helpful to have an
implementation document. I'll write one up today.

[binji, 2015-05-07 01:03:01]

On 2015/05/06 16:53:23, binji wrote:
> > I think the complexity of this is getting a bit out of hands. Could we
> possibly
> > have a separate design document which outlines what needs to be done?
> > 
> > The document should describe at least the following:
> > 
> > 1. Representation of SAB in v8.
> > 
> > 2. Compilation of non-atomic accesses to SAB. (Probably just like normal
> > accesses to AB, but if the SAB have different map as in the current version,
> one
> > has to be careful about map checks/polymorphism in hydrogen and ICs.)
> > 
> > 3. Compilation of atomic accesses to SAB. This should discuss all the
> compilers:
> > full code, hydrogen and turbofan. My guess is we would be fine with having
> > runtime functions for full code and hydrogen, but turbofan needs to have
> > optimized version. This should also discuss architecture specific details
> (such
> > as implementation on architectures without 64-bit atomic instructions).
> > 
> > 4. Compilation of futexes.
> > 
> > 5. Discussion of neutering and friends.
> > 
> > 6. API design, interface to the rest of chrome.
> > 
> > I am sure I am missing other important aspects.
> 
> Sorry for the confusion. You're right, it will be helpful to have an
> implementation document. I'll write one up today.

Still a lot more to add, but here is the doc:
https://docs.google.com/document/d/17bWw19CozbeDitdZxXBA-_7IdUzwzXB6XptLAIcsD...