1246597 - Support DataView on SharedArrayBuffer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Lars T Hansen [:lth]]

The SharedArrayBuffer spec says that DataView is allowed on SharedArrayBuffer, and that SharedArrayBuffer.isView() on a DataView should return true.  Neither is the case in Firefox at the moment.

Atomics are not allowed on DataView (just as they are not allowed on Uint8ClampedArray), but DataView on SAB is allowed because it is convenient for reading/writing data from the buffer under a separate lock.

Not sure what the repercussions of this is yet - probably this change becomes visible to DOM somehow (IIRC it has a generic ArrayBufferView type that can represent a DataView, but more investigation needed).

Comment 1

[Lars T Hansen [:lth]]

@fscholz, it was the existing dev-doc that made me find this bug :)  Specifically the doc for SharedArrayBuffer.isView() mentions DataView, which I thought was wrong, but which turns out to be right.

Comment 2

[regis portalez]

I found this bug following my StackOverflow question http://stackoverflow.com/questions/37420570/read-memory-in-sharedarraybuffer. 

Consequences are it's impossible to share work on SharedArrayBuffer, unless there is another way to read memory from a SharedArrayBuffer without copying the entire buffer in a new TypedArray.

Comment 3

[Lars T Hansen [:lth]]

At the risk of telling you something you already know:

You can read data from the buffer (and write data to it) by creating a TypedArray on it and then accessing the data via the TypedArray.  This does *not* copy the memory, indeed it is a fairly inexpensive operation to create a view.

  var sab = ...;                  // SharedArrayBuffer received from somewhere or created here
  var ia = new Int32Array(sab);   // Create a view on the memory
  var ba = new Uint8Array(sab);   // Create another view on the same memory

  print(ia[5]);
  ia[37] = ...;
  ia[0] = -1;                     // visible to reads from ba
  print(ba[0]);                   // prints 255

These views are slightly less convenient than DataView since you only get a monotyped view (all int32s or uint8s, in this case).  But the ability to have multiple views on the same buffer at the same time helps.

Comment 4

[regis portalez]

Thanks for the answer. 
I tested and making new TypedArray from SharedArrayBuffer doesn't copy memory and has a negligeable overhead. I also edited the stackoverflow question to integrate your answer. 
As for the convenience, I don't really have the problem in my application.

Comment 5

[spectralcanine]

I was just going to open up the same bug.

Using views is not a replacement for DataView. Yes, you can make a wrapper for DataView using only views (which I actually did years ago when Chrome supported DataView and Firefox didn't), but then it becomes quite slower and less nice than a native solution.

Seeing the results of this bug report https://bugzilla.mozilla.org/show_bug.cgi?id=1279986 I am pretty sure fixing this should take perhaps a few line changes, granted I've never looked at Firefox's source.

Comment 6

[Lars T Hansen [:lth]]

Supporting DataView is actually a bit of work, due to the structure of the Firefox code.  It will be implemented, though -- it's required by the spec.

I don't understand what you mean by what you write about a "wrapper" and "slower".  Access to an ArrayBuffer via DataView is in general slower than access via a TypedArray, since on at least some platforms the DataView access for a multi-byte datum must be byte-at-a-time as the access can be unaligned (ARM, MIPS have this problem).

In fact, it does not look like Firefox's JS engine even inlines the DataView accessors, so eg view.getInt8() is almost certainly an expensive call-out to C++, not an in-line bounds check + byte load.  This should make DataView accesses at least an order of magnitude slower than comparable TypedArray accesses.  A simple benchmark (attached) bears this out: a simple loop using DataView is about 20x slower than one using TypedArray.

Also note that atomic operations are only supported on TypedArray views, so there's no way for you to implement fast synchronization using only DataView.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Simple benchmark

Comment 8

[spectralcanine]

I made in the past a partial replacement for DataView using only Uint8Array for the source (to avoid byte alignment issues), and then reading bytes and transforming them, either using other typed arrays, or just manipulating them with bitwise operators.

My uses don't need to synchronize anything over a DataView object. Rather, I want the main buffer to be shared, and then parsed on multiple workers. This is extra work, but it's so much easier than parsing on one and then transferring all of the data, which is a mess of array buffers, regular objects, hierarchies of objects, and so on (especially when prototypes get erased when sending objects, which makes it ever weirder to try to add them back while keeping object hierarchies in mind).

Comment 9

[Lars T Hansen [:lth]]

Another data point (not really speaking to your experience here) is that an emulation of DataView is much faster than DataView for both getUint8 and getInt32 accessors.  This is sad, really.

I'm not the first to notice that, either: https://bugzilla.mozilla.org/show_bug.cgi?id=1065894.

Comment 10

[Lars T Hansen [:lth]]

Attachment: Simple benchmark v2

Comment 11

[spectralcanine]

The problem with parsing using views is byte alignment. I am parsing arbitrary binary data, and many times it is not 4 byte aligned, while most of it is 32bit floats, which makes it impossible to just view into the data.

Still, it's interesting to see how native structures can be slower than ES ones.

Comment 12

[Lars T Hansen [:lth]]

We need DV-on-SAB for FF53.

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1246597-engine-bits.patch

WIP - support DV on SAB in the engine.  This still uses not-safe-for-races memcpy for fromBuffer and toBuffer and is not tested.  The engine API changes slightly to accomodate the isSharedMemory return parameter, so DOM code will need to change too, though probably not hugely.  (dom/bindings/TypedArray.h, for starters.)

Comment 14

[Lars T Hansen [:lth]]

No API changes appear to be needed in DOM or the rest of Firefox.  There needs to be a minor adjustment to a comment in dom/bindings/TypedArray.h which states that DataView can't map shared memory.

Comment 15

[Lars T Hansen [:lth]]

Attachment: test_sab_with_webgl.html

Hacked-up test case based on dom/canvas/test/webgl-mochitest/test_sab_with_webgl.html that uses DataView where it can, demonstrating that DataView-on-SAB is compatible with DOM without further changes.

(This test can be run standalone, check the console for output.)

Comment 16

[Lars T Hansen [:lth]]

Attachment: bug1246597-dom-comment-fixes.patch

Comment 17

[Lars T Hansen [:lth]]

Attachment: bug1246597-js-tests.patch

JS engine test cases, not sure if we really need more than this.

Comment 18

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #15)
> Created attachment 8814079 [details]
> test_sab_with_webgl.html
> 
> Hacked-up test case based on
> dom/canvas/test/webgl-mochitest/test_sab_with_webgl.html that uses DataView
> where it can, demonstrating that DataView-on-SAB is compatible with DOM
> without further changes.

It is arguable that WebGL is not supposed to accept DataView values, https://www.khronos.org/registry/webgl/specs/1.0/#5.13 excludes DataView implicitly by referencing a spec section on AB views that does not contain DataView.  So for DOM we want a different kind of test case.

Comment 19

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76204e07db5d

Comment 20

[Lars T Hansen [:lth]]

Attachment: bug1246597-dataview-on-shared-memory.patch

This follows the conventions established by TypedArray-on-SAB.  My main concern is that I may have missed a spot where we need to make a change to allow SAB, so it's especially worthwhile to think about that during review.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8814824 [details] [diff] [review]
bug1246597-dataview-on-shared-memory.patch

Review of attachment 8814824 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfriendapi.h
@@ +2148,5 @@
>   * it would pass such a test: it is a data view or a wrapper of a data view,
>   * and the unwrapping will succeed. If cx is nullptr, then DEBUG builds may be
>   * unable to assert when unwrapping should be disallowed.
> + *
> + * *isSharedMemory will be set to true if the DataView maps a SharedArrayBuffer,

|*isSharedMemory|, especially because of the * * thing being hard to read.

::: js/src/tests/js1_8_5/extensions/shareddataview.js
@@ +28,5 @@
> +
> +// Test that it is the same buffer memory for the two views
> +
> +dv.setInt8(1024, 37);
> +assertEq(dv2.getInt8(0), 37);

Possibly worth using a non-binary-power index here, for irregularity of the index.

::: js/src/vm/ArrayBufferObject.cpp
@@ +1154,5 @@
>  
>      uint32_t byteOffset = args[0].toPrivateUint32();
>      uint32_t byteLength = args[1].toPrivateUint32();
> +    Rooted<ArrayBufferObjectMaybeShared*> buffer(
> +        cx, &args.thisv().toObject().as<ArrayBufferObjectMaybeShared>());

Mild preference for

  Rooted<> buffer(cx);
  buffer = ...;

::: js/src/vm/ArrayBufferObject.h
@@ +258,2 @@
>      static bool createDataViewForThisImpl(JSContext* cx, const CallArgs& args);
> +

Don't add the blank line here.  The "Impl" function is only called by the non-Impl version, it does so almost trivially because of cross-compartment concerns, and as a practical matter the two functions constitute a single unit.  Better to keep them grouped together than to follow normal practice.

::: js/src/vm/TypedArrayObject.cpp
@@ +1699,5 @@
>      dvobj.setFixedSlot(TypedArrayObject::BYTEOFFSET_SLOT, Int32Value(byteOffset));
>      dvobj.setFixedSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(byteLength));
>      dvobj.setFixedSlot(TypedArrayObject::BUFFER_SLOT, ObjectValue(*arrayBuffer));
> +
> +    auto ptr = arrayBuffer->dataPointerEither();

I think I'd prefer seeing the type -- this isn't quite an idiomatic case where the reader necessarily would just know what the type was.

Comment 22

[Jeff Walden [:Waldo]]

The function signature/name-changes seem like they should generally suss out stuff needing to change.  I can't think of any other places likely to need a change.  Worth keeping an eye on things, and hopefully fuzzers can start using this pretty quickly.

Speaking of which -- tests for this?  Even very basic tests would at least let the fuzzers that repurpose existing tests, start to use this.

Comment 23

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dbf4912c2cf

Comment 24

[Lars T Hansen [:lth]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> 
> Speaking of which -- tests for this?  Even very basic tests would at least
> let the fuzzers that repurpose existing tests, start to use this.

There are a few test cases included here to show that view access works, and atomics already can't be applied to DataViews.  That said, I'll look for existing tests to see if I can generalize them to also use SharedArrayBuffer.

Comment 25

[Lars T Hansen [:lth]]

Attachment: bug1246597-dataview-tests.patch

The only nontrivial thing here has to do with proxies, near the end of the patch.  This test uncovered an incorrect assertion in the main patch (an IsArrayBuffer assertion needed to be  IsArrayBufferMaybeShared).

Comment 26

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9287824003f42411c84cdf4b7b3506371c57a582
Bug 1246597 - support DataView on SharedArrayBuffer. r=waldo

https://hg.mozilla.org/integration/mozilla-inbound/rev/195138298619f5ee4c99dd17df0df98737762a25
Bug 1246597 - DOM comment change. r=me

Comment 27

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfc6a97862b2

Comment 28

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e905d6da8356a1da48d43afb4bbbc54b4b5ae02
Bug 1246597 - adapt existing DataView tests to SharedArrayBuffer. r=waldo

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9287824003f4
https://hg.mozilla.org/mozilla-central/rev/195138298619
https://hg.mozilla.org/mozilla-central/rev/1e905d6da835

Comment 30

[Bobby Powers]

This still doesn't seem to work for me in dev edition from 1-12:


>> var sab = new SharedArrayBuffer(4096);
undefined
>> var dv = new DataView(sab);
TypeError: DataView: expected ArrayBuffer, got SharedArrayBuffer
>> typeof SharedArrayBuffer
"function"


(that invocation is from the testcase in 928782400).  Should I open a new bug for this, or is there something immediately obvious I'm doing wrong?

Comment 31

[Lars T Hansen [:lth]]

We last branched for DevEd on Nov 14, before this patch landed, and will branch again on Jan 23, at which point the fix will be included in a DevEd a few days after that.  In the mean time you'll need to use Nightly for this feature.  I just checked that your example works in the latest Nightly.

(Be aware that DataView is quite slow and that you'll be better off using a TypedArray view on shared memory if you want good performance.)

Comment 32

[Bobby Powers]

Lars -- thanks for the followup and sorry for the noise.  I had mistakenly thought DevEd was a differently branded Nightly (rather than Aurora).

I've verified that everything works in Nightly as well, this is great!!!

Comment 33

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/53#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView/buffer