I would like to have reviewed and land a default-disabled prototype of the SharedArrayBuffer developed at https://github.com/sstangl/pthreads-gecko over the past few months. It will likely take a while to chop all the changes into reviewable pieces. SharedArrayBuffer is a component in an experiment to support multithreaded codebases using pthreads in Emscripten. The buffer functions nearly identically to an ArrayBuffer, except that when passed to a worker via the |transferables| argument of postMessage(), a SharedArrayBuffer is passed by it's raw buffer's address, explicitly permitting contained racy behavior. Landing on trunk will enable easier prototyping and reduce time spent rebasing. I am also hoping that landing this will make the project seem more tangible, so that a more meaningful discussion on JS parallelism occurs.

Isn't regular ArrayBuffer shared?

emk: If you post an ArrayBuffer to another thread, by default, it is copied. If you ask for it to be transferred, then the recipient gets the buffer (not a copy), but the caller no longer has it. The caller's ArrayBuffer becomes empty.

Comment on attachment 827655 [details] [diff] [review] WIP Part 1 - Define SharedArrayBufferObject Review of attachment 827655 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/SharedArrayObject.cpp @@ +80,5 @@ > +SharedArrayRawBuffer * > +SharedArrayRawBuffer::New(uint32_t nbytes) > +{ > + // Enforced by SharedArrayBufferObject constructor. > + JS_ASSERT(IsPowerOfTwo(nbytes) && nbytes >= AsmJSAllocationGranularity); Can the IsPowerOfTwo restriction be relaxed? Perhaps it need only check that it is a multiple of the page size here? @@ +102,5 @@ > + uint32_t allocSize = sizeof(SharedArrayRawBuffer) + sizeof(ObjectElements) + nbytes; > + if (allocSize <= nbytes) > + return nullptr; > + > + void *p = MapMemoryHelper(allocSize, true); Might it be prudent to think ahead to page aligning the first element for the benefit of buffer.discard()? Could just pad out the tail of SharedArrayRawBuffer so that when it starts at the start of a page then the elements start at the next page. ::: js/src/vm/SharedArrayObject.h @@ +72,5 @@ > + > +/* > + * SharedArrayBufferObject > + * > + * Opaque ArrayBuffer usable only from AsmJS. It is correct? Can JIT code not use the shared array buffer? @@ +86,5 @@ > + static Class protoClass; > + > + // Slot used for storing the view list in a SharedArrayBufferObject. > + // ObjectElements cannot be used because it is stored in shared memory. > + static const int32_t VIEW_SLOT = 0; Could you elaborate a little in the comment. Would it be possible to explain 'shared memory' further here. All threads of a process share the same memory space, and are workers that can shared the buffer always in the same process? Are the buffers only being shared between threads of a process, and not between processes? Is it the case that each worker thread has its own view list?

> Can the IsPowerOfTwo restriction be relaxed? Perhaps it need only check > that it is a multiple of the page size here? See also IsValidAsmJSHeapLenth() in AsmJS.h.

I am not fully clear on the motivation for landing this, even behind a flag. This is an experiment to investigate one possible approach (there are several other ideas floating around) to a subject on which there is a lot of debate and even controversy (shared typed arrays). I can see that landing it will reduce rebasing for you, as other people working on trunk won't break it directly. But it also means that we are adding work for those people, for code that is exploratory and experimental, and may be removed entirely later on? Also, landing it even behind a pref is typically an indication of a certain level of confidence in the approach, higher than I think anyone has for this experiment at this point - the experiment may change that, but it's far too early to tell - so it feels like it sends an odd message to land it on trunk? Overall this kind of thing feels like it makes sense for a side branch. Am I missing something here?

(In reply to Alon Zakai (:azakai) from comment #6) > I am not fully clear on the motivation for landing this, even behind a flag. Sorry for letting this comment hang for a week -- I haven't been able to work on the SharedArrayBuffer recently, and I forgot to respond. Thanks for reminding me. As you know, there have recently been a number of competing proposals for safely sharing buffers between workers, for the purpose of supporting multithreaded C/C++ video games on the Web. We have iterated through several stages of these proposals (for example, https://gist.github.com/dherman/5463054, which provides for explicit region locks) to a point where every current proposal of which I am aware involves an explicit SharedArrayBuffer, with ongoing discussions primarily concerning restrictions, locks, and the memory model. So as it currently stands, some sort of SharedArrayBuffer is Mozilla's most plausible attempt at a solution to the parallelism problem. There is somewhat of a time-based component as well: NaCl/PNaCl already enable multithreaded C/C++ video games to run in Chrome. Although Google is developing pepper.js to cross-compile PNaCl codebases to the Web via Emscripten, and officially mentions this as a way to demonstrate that PNaCl does not induce vendor lock-in, multithreaded PNaCl programs are locked into Chrome for the foreseeable future without action. This is a shame, because there are many interesting multithreaded games that ought to work on the Web at large. By delaying, we risk eroding the Web as a deployment target for modern translated software. So I'm hoping that by landing SharedArrayBuffer on trunk -- disabled, with no intention to enable, and with !EXPERIMENTAL! and !DANGERZONE! warnings abundant -- we can demonstrate the plausibility of shared-state JavaScript. Hopefully, this will garner interest from other browsers sufficiently to discuss and develop a Web-standard target for pthreads via Emscripten. Shipping the feature in a release version of Firefox (after the user manually enables the preference) would demonstrate that we are serious about the problem space and allow testing by games companies, as with asm.js, increasing the likelihood of cross-browser collaboration. Discussion and standardization will take a very long time -- but it's important for the Web, and if we're going to take this problem space seriously, we might as well move now.

I somehow missed the earlier comments as well -- landing this pref'd off will allow a wider group of people to experiment and give feedback on this approach (whether positive or negative). I know Sean has been busy, but I hope that we can get this in soon.

This is a rebased patch that also allows the SAB to be linked against for AsmJS. The last remaining components are rebasing the Structured Cloning algorithm changes, and making SAB preffed-off. It shouldn't be much work. A whole hell of a lot of GC debugging was finally resolved by moving the ObjectElements from shared memory to fixed slots, so that SAB can use the same OldObjectRepresentationHack as ArrayBufferObject.

I'm going to post the SharedArrayBuffer patches now. They work and have been thoroughly tested. The only item of work remaining is to make the constructor conditional on browser and shell preferences, but the patches are reviewable without the preference. Once this work is landed, we can experiment with the more interesting components, such as atomics and locks. Please bear in mind that for whatever reason, this patchset tends to bitrot on a near-daily basis. Generally it's fairly easy to modify a patch to again be valid -- please ping me if it's not, and I'll upload a new version. Any comments and feedback would be most welcome.

More tests are warranted, in particular xpcshell tests to cover StructuredClone changes, but these ones were useful for isolating GC issues.

I'm not sure who the best person (or people) to review Part 1 would be. Waldo comes to mind, but he's going on vacation soon.

Comment on attachment 8371905 [details] [diff] [review] Part 2 - AsmJS support for SharedArrayBuffer Review of attachment 8371905 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/AsmJSLink.cpp @@ +209,5 @@ > static bool > +AttachArrayBuffer(JSContext *cx, AsmJSModule &module, Handle<ArrayBufferObject*> heap) > +{ > + if (!IsValidAsmJSHeapLength(heap->byteLength())) { > + return LinkFail(cx, JS_smprintf("ArrayBuffer byteLength 0x%x is not a valid heap length. The next valid length is 0x%x", You'll get a rebase conflict with https://hg.mozilla.org/mozilla-central/rev/be7fa3989aed be careful not to reintroduce the leak :) @@ +257,5 @@ > if (!IsTypedArrayBuffer(bufferVal)) > return LinkFail(cx, "bad ArrayBuffer argument"); > > + heap = &AsTypedArrayBuffer(bufferVal); > + if (!AttachArrayBuffer(cx, module, heap)) Perhaps we could name it LinkModuleToHeap instead? ::: js/src/vm/TypedArrayObject.cpp @@ +4047,5 @@ > js::IsTypedArrayBuffer(HandleValue v) > { > + return v.isObject() && > + (v.toObject().is<ArrayBufferObject>() || > + v.toObject().is<SharedArrayBufferObject>()); Can you align the first char of 2nd and 3rd lines to match 'v' on first?

Comment on attachment 8371903 [details] [diff] [review] Part 1 - Define SharedArrayBufferObject Review of attachment 8371903 [details] [diff] [review]: ----------------------------------------------------------------- The commit message is a little detailed. :) I want to go back over this once more and figure out why you need the As* and Is* functions instead of overriding as<> and is<>, but I need to take off and it isn't worth blocking landing over. I'll look again later. ::: js/src/vm/GlobalObject.h @@ +26,5 @@ > extern JSObject * > js_InitTypedArrayClasses(JSContext *cx, js::HandleObject obj); > > +extern JSObject * > +js_InitSharedArrayBufferClass(JSContext *cx, js::HandleObject obj); Can't you just do this initialization within js_InitTypedArrayClasses? ::: js/src/vm/ObjectImpl.cpp @@ +534,5 @@ > JSObject * > js::ArrayBufferDelegate(JSContext *cx, Handle<ObjectImpl*> obj) > { > + MOZ_ASSERT(obj->hasClass(&ArrayBufferObject::class_) || > + obj->hasClass(&SharedArrayBufferObject::class_)); I think these read better as obj->is<ArrayBufferObject>() || obj->is<SharedArrayBufferObject> these days. ::: js/src/vm/SharedArrayObject.cpp @@ +29,5 @@ > + * SharedArrayRawBuffer > + */ > + > +inline void * > +MapMemoryHelper(size_t length, bool commit) Why "inline FooHelper"? Why not just static inline void * MapMemory(...) here and in the other *Helper functions? @@ +37,5 @@ > + int flags = (commit ? PAGE_READWRITE : PAGE_NOACCESS); > + void *p = VirtualAlloc(nullptr, length, prot, flags); > + if (!p) > + return nullptr; > + return p; The if (!p) return nullptr part doesn't do much... @@ +100,5 @@ > + void *base = MapMemoryHelper(allocSize, true); > + if (!base) > + return nullptr; > + > + // TODO: Align buffer to page boundary. Sorry for being anal, but can you put a bug number here? @@ +110,5 @@ > +void > +SharedArrayRawBuffer::addReference() > +{ > + JS_ASSERT(this->refcount > 0); > + ++(this->refcount); // Atomic. extra parens, here and other places. Precedence of -> is nice and high. @@ +136,5 @@ > +/* > + * SharedArrayBufferObject > + */ > +bool > +js::IsSharedArrayBuffer(HandleValue v) Why HandleValue? Seems like overkill. I don't expect is<> things to ever GC. @@ +253,5 @@ > +SharedArrayRawBuffer * > +SharedArrayBufferObject::rawBufferObject() const > +{ > + Value v = getReservedSlot(SharedArrayBufferObject::RAWBUF_SLOT); > + return (SharedArrayRawBuffer *)v.toPrivate(); Wait... if RAWBUF_SLOT is undefined, then this will assert, right? So there's a contract here that you won't do dropRawBuffer() followed by rawBufferObject()? Seems worth a comment. @@ +385,5 @@ > +JSObject * > +js_InitSharedArrayBufferClass(JSContext *cx, HandleObject obj) > +{ > + JS_ASSERT(obj->isNative()); > + Rooted<GlobalObject*> global(cx, &obj->as<GlobalObject>()); global and obj are never used. Who calls this? ::: js/src/vm/TypedArrayObject.cpp @@ +129,5 @@ > MOZ_ALWAYS_INLINE bool > IsArrayBuffer(HandleValue v) > { > + return v.isObject() && > + (v.toObject().hasClass(&ArrayBufferObject::class_) || is<>() for this too. @@ +2829,4 @@ > // Verify that the private slot is at the expected place > JS_ASSERT(dvobj.numFixedSlots() == DATA_SLOT); > > + AsArrayBuffer((HandleObject)arrayBuffer).addView(&dvobj); I don't see why AsArrayBuffer should require a Handle in the first place. Nor do I understand why you need to cast at all?

Comment on attachment 8371906 [details] [diff] [review] Part 3 - StructuredClone support for SharedArrayBuffer Review of attachment 8371906 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/StructuredClone.cpp @@ +977,5 @@ > + return false; > + if (!out.write(0)) // |userdata|, intended to be passed to callbacks. > + return false; > + > + // FIXME: This should probably be done in transferOwnership(). Why can't you do this in transferOwnership? This is visible, isn't it? As in, the shared array buffer could be modified during the cloning. Or maybe you need to do it in both places? Here, so that the output buffer owns a reference to the raw buffer object, and then overwrite it with the final raw buffer object at the end? (Or keep a vector of owned raw buffer objects directly in the output buffer, I guess; transferable stuff doesn't need to be serialized to a byte stream.) I'd at least like to discuss this before marking it r+.

Comment on attachment 8372635 [details] [diff] [review] Make SAB preffed-off outside of Nightly Review of attachment 8372635 [details] [diff] [review]: ----------------------------------------------------------------- Oh, yuck. *This* is why you have a separate init function. Ugly.

Thanks for the quick review! > Can't you just do this initialization within js_InitTypedArrayClasses? Ideally in the future yes, but since SharedArrayBuffer must be a preffable constructor, it's treated specially. > I think these read better as > obj->is<ArrayBufferObject>() || obj->is<SharedArrayBufferObject> > these days. This code refers to ObjectImpl, which doesn't have an is() function. > Why HandleValue? Seems like overkill. I don't expect is<> things to ever GC. IsSharedArrayBuffer is used in CallNonGenericMethod<IsSharedArrayBuffer, byteLengthGetterImpl>, and the first template argument requires a signature with HandleValue.

Same as before, as discussed on IRC, except that the buffer refcount is incremented during transferOwnership().

Comment on attachment 8378672 [details] [diff] [review] Part 3 v2 - StructuredClone support for SharedArrayBuffer0003-StructuredClone-changes.patch Review of attachment 8378672 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/StructuredClone.cpp @@ +971,5 @@ > + > +bool > +JSStructuredCloneWriter::writeSharedArrayBufferForTransfer(SharedArrayBufferObject &obj) > +{ > + if (!out.writePair(SCTAG_TRANSFER_MAP_ENTRY, SCTAG_TM_SHARED_BUFFER)) These don't need to be separate any more. It seems better to not write SCTAG_TM_SHARED_BUFFER yet, since you really haven't, and you're using a nullptr to signal the same thing that SCTAG_TM_UNFILLED does. You just need to adjust the comment a little. @@ +1043,5 @@ > + LittleEndian::writeUint64(point++, reinterpret_cast<uint64_t>(content)); > + LittleEndian::writeUint64(point++, 0); > + } else if (obj->is<SharedArrayBufferObject>()) { > + MOZ_ASSERT(uint32_t(LittleEndian::readUint64(point) >> 32) == SCTAG_TRANSFER_MAP_ENTRY); > + MOZ_ASSERT(uint32_t(LittleEndian::readUint64(point)) == SCTAG_TM_SHARED_BUFFER); This assert will change if you do what I recommended above, and in fact could be commoned up. @@ +1047,5 @@ > + MOZ_ASSERT(uint32_t(LittleEndian::readUint64(point)) == SCTAG_TM_SHARED_BUFFER); > + > + SharedArrayRawBuffer *rawbuf = obj->as<SharedArrayBufferObject>().rawBufferObject(); > + > + // Shared buffers are already written. Not anymore. @@ +1054,5 @@ > + point++; // Skip userdata. > + > + // Add a reference to the SharedArrayRawBuffer in advance of the transfer. > + // This avoids a race condition where the parent thread frees the buffer > + // before the child has accepted the transferable. Stale comment.

Full patch after rebasing around Niko's changes and getting the patch to pass tests again. Looks promising on Tryserver. Note that the old patches no longer apply in any reasonable way. This is a change to ArrayBuffer code, so fuzzing that interface would be very useful. Additionally, the global constructor "SharedArrayBuffer" is also fuzzable.

Comment on attachment 8380017 [details] [diff] [review] Full patch for fuzzing The following test asserts on a 64 bit debug build: function f( ... actual ) { var x = new SharedArrayBuffer(0x1000); var y = f(x); } f(); Assertion failure: JSVAL_IS_DOUBLE_IMPL(data), at ../../dist/include/js/Value.h:1170

Comment on attachment 8380017 [details] [diff] [review] Full patch for fuzzing g = (function(stdlib, foreign) { var f = foreign.ff; return f })(this, { ff: SharedArrayBuffer }); function f(x) { return g(x >>> 1) } try { f(0x80000000); f(0x80000000); f(0x80000000); } catch (e) {} Crash [@ js::SharedArrayBufferObject::Finalize] in 32-bit opt shells, and also asserts debug shells at Assertion failure: JSVAL_IS_DOUBLE_IMPL(data), at ../../dist/include/js/Value.h Opt configure parameters: LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-exact-rooting --with-ccache --enable-threadsafe <other NSPR flags>

I am skeptical of the claim that the intentional C++ data races introduced by this patch are "benign". http://blog.regehr.org/archives/658 http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Thank you both for the previous round of fuzzing. Both testcases wound up exposing the same issue: OOM during SharedArrayBufferObject creation prevented a SharedArrayRawBuffer from being attached, and ::Finalize() assumed its presence. Also, thanks for attaching both variants of the failure -- it actually proved to be useful. Decoder's version works across architectures and is therefore more useful to include in the test suite, while Gary's version failed immediately on x86 and was easier to debug. This full patch fixes that issue.

(In reply to Jesse Ruderman from comment #27) > I am skeptical of the claim that the intentional C++ data races introduced > by this patch are "benign". A SharedArrayBuffer functions like an ArrayBuffer, except that the bytes of the underlying raw buffer are always malloc()d. When a SharedArrayBuffer is transferred to a WebWorker, the recipient is passed the address of the malloc()d raw buffer. The recipient then creates its own local SharedArrayBuffer object which points to the raw buffer -- no engine objects are racy. There should hence be no C++ data races, at least in the engine. With regards to raciness, only writing to and reading from the buffer is racy, with respect to other memory accesses from other WebWorkers sharing the same underlying buffer. This is fine for us, since the goal is to support multithreaded C++ programs using the SharedArrayBuffer as their AsmJS heap, and these programs necessarily already operate in such an environment (without C++11's extensions -- the codebases we care about just use pthreads and atomics for safety).

Comment on attachment 8380911 [details] [diff] [review] [v2] Full patch for fuzzing (function(stdlib, foreign, heap) { "use asm"; var Float64ArrayView = new stdlib.Float64Array(heap); function f() {} return f })(this, {}, new SharedArrayBuffer(4096)) Assertion failure: !buffer->isSharedArrayBuffer(), at vm/ArrayBufferObject.cpp asserts 32-bit js debug shell with the following parameters: LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-exact-rooting --with-ccache --enable-threadsafe <other NSPR flags>

(In reply to Sean Stangl [:sstangl] from comment #29) > (In reply to Jesse Ruderman from comment #27) > > I am skeptical of the claim that the intentional C++ data races introduced > > by this patch are "benign". > > With regards to raciness, only writing to and reading from the buffer is > racy, with respect to other memory accesses from other WebWorkers sharing > the same underlying buffer. This is fine for us, since the goal is to > support multithreaded C++ programs using the SharedArrayBuffer as their > AsmJS heap, and these programs necessarily already operate in such an > environment (without C++11's extensions -- the codebases we care about just > use pthreads and atomics for safety). I would describe it as: yes, this patch introduces the possibility of data races to the Web platform. It does not introduce races to the actual engine, so it's not increasing our attack surface (other than it being additional code and functionality, but it's not special.) It *does* mean that workers can easily corrupt the contents of the shared memory region, just as you could when writing native C++ code running directly on memory, and so worker authors have all of the (enormous) headaches associated with that. The solutions are the same -- use synchronization primitives to avoid races, or weird stuff will happen (where "weird stuff" == "the contents of the shared memory region may not be what you expect"). And compiler-generated asm.js code will do all of the nasty things described in the article you posted. I believe the question is still open as to whether we really want to introduce this stuff to the web platform (or rather, to the web developer community), but that's a separate question.

(In reply to Steve Fink [:sfink] from comment #31) > I believe the question is still open as to whether we really want to > introduce this stuff to the web platform (or rather, to the web developer > community), but that's a separate question. Note that this question has been taken extraordinarily seriously by most involved. While this patch adds SAB w/o any restrictions on usage, this is only for Nightly and experimenting. What is being discussed for actual standardization is that "normal JS" isn't allowed to race on a SAB. The exact restriction is still under discussion (hence it is premature to attempt to put the restriction in this experimental patch), but contenders are "only code in Web Workers" or "only asm.js-validated code" (which means standardizing the asm.js validation predicate).

Thanks for the last round of fuzzing. It found a discrepancy between x86_64 and x86 paths.

Comment on attachment 8381624 [details] [diff] [review] [v3] Full patch for fuzzing I didn't find anything anymore after an overnight run.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2e4e1be13c https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ebf76f5010 https://hg.mozilla.org/integration/mozilla-inbound/rev/79c2a2e38761 https://hg.mozilla.org/integration/mozilla-inbound/rev/b21841db83df https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb98e166b5b

Could this have caused what looks like a 5% regression on x86 GGC kraken-parse-financial? Might even be a 1% regression on non-GGC?

https://hg.mozilla.org/mozilla-central/rev/fe2e4e1be13c https://hg.mozilla.org/mozilla-central/rev/d5ebf76f5010 https://hg.mozilla.org/mozilla-central/rev/79c2a2e38761 https://hg.mozilla.org/mozilla-central/rev/b21841db83df https://hg.mozilla.org/mozilla-central/rev/7bb98e166b5b

I had another issue, but wasn't quick enough to report it here. I filed bug 977860 for this :)