As per the spec linked from bug 1054841: - SharedArrayBuffer should not be a subtype of ArrayBuffer (neither in the implementation nor in its API) - The TypedArray types (Int8Array, etc) should not be constructable on SharedArrayBuffer, instead there must be a new hierarchy of SharedTypedArray types (SharedInt8Array, etc) that can be constructed on SharedArrayBuffer. This bug tracks that reengineering work.

Those three should apply cleanly to mozilla-inbound 5f66dd3d63f2 (August 28). They are clean and largely complete but only lightly tested; I'm in the process of constructing test cases. If I find serious bugs I will note it here. Coming later: Changes to odin, and to asm.js interfacing.

Notes to the reviewer: - these patches apply on top of each other in the order posted - try is mostly green, I'm mopping up some compilation failures on odd platforms - the largest danger is probably that there are parts of the VM that I've neglected to convert to handle both types of TypedArray

Comment on attachment 8483359 [details] [diff] [review] Reorganize ArrayBufferObject to split the shared and unshared case Review of attachment 8483359 [details] [diff] [review]: ----------------------------------------------------------------- This looks really solid. I have some comments that I think should be addressed. Most importantly, the length and offset handling in SharedTypedArrayObjectTemplate::create should be explained or fixed. Also, the large amounts of code duplication between SharedTypedArrayObjectTemplate and TypedArrayObjectTemplate are really unfortunate. I understand that there are differences in some of the methods, but maybe it'd make sense to only split those out and still share the rest? Not that that'd make the inheritance situation any nicer ... ::: js/src/js.msg @@ +346,5 @@ > MSG_DEF(JSMSG_SC_BAD_SERIALIZED_DATA, 1, JSEXN_INTERNALERR, "bad serialized structured data ({0})") > MSG_DEF(JSMSG_SC_DUP_TRANSFERABLE, 0, JSEXN_TYPEERR, "duplicate transferable for structured clone") > MSG_DEF(JSMSG_SC_NOT_TRANSFERABLE, 0, JSEXN_TYPEERR, "invalid transferable array for structured clone") > MSG_DEF(JSMSG_SC_UNSUPPORTED_TYPE, 0, JSEXN_TYPEERR, "unsupported type for structured data") > +MSG_DEF(JSMSG_SC_SHMEM_MUST_TRANSFER, 0, JSEXN_TYPEERR, "SharedArrayBuffer must be transfered") I think we should mention structured cloning here, somehow. Perhaps "SAB cannot be cloned but must be transferred during structured cloning"? Also, nit: "transferred" ::: js/src/tests/js1_8_5/extensions/sharedtypedarray.js @@ +1,5 @@ > +// |reftest| skip-if(!xulRuntime.shell) > +/* -*- Mode: js2; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/licenses/publicdomain/ Apparently this is the one to use nowadays: https://creativecommons.org/publicdomain/zero/1.0/ @@ +97,5 @@ > + assertEq(Object.getOwnPropertyDescriptor(SharedInt8Array.prototype,"buffer").get.call(x1), x1.buffer); > + assertEq(Object.getOwnPropertyDescriptor(SharedInt32Array.prototype,"buffer").get.call(x2), x2.buffer); > + > + // Not generic > + try { var thrown=false; Object.getOwnPropertyDescriptor(SharedInt8Array.prototype,"buffer").get.call({}) } catch (e) { thrown=e } You can use `assertThrowsInstanceOf` here and in the other exception tests below. ::: js/src/vm/ArrayBufferObject.h @@ +61,4 @@ > > typedef Vector<ArrayBufferObject *, 0, SystemAllocPolicy> ArrayBufferVector; > > +class ArrayBufferObjectMaybeRacy : public JSObject While I see that "Racy" is more evocative than "Shared" and assume that you want to call out that using this is potentially dangerous, I still think that deviating from the terminology used everywhere else isn't great. Hence, I think this should be called "ArrayBufferObjectMaybeShared". @@ +99,1 @@ > MAPPED_BUFFER = 0x4, Might as well make this 0x2. ::: js/src/vm/ObjectImpl-inl.h @@ +35,5 @@ > inline bool > ClassCanHaveFixedData(const Class *clasp) > { > // Normally, the number of fixed slots given an object is the maximum > + // permitted for its size class. For array buffers and nonshared typed uber-nit: "non-shared" ::: js/src/vm/Shape-inl.h @@ +14,5 @@ > #include "jsobj.h" > > #include "vm/Interpreter.h" > #include "vm/ScopeObject.h" > +#include "vm/SharedTypedArrayObject.h" Is this include needed? @@ +20,1 @@ > #include "vm/TypedArrayObject.h" Same for this? ::: js/src/vm/SharedArrayObject.h @@ +78,5 @@ > + * has a finalizer that decrements the refcount, the last one to leave > + * (globally) unmaps the memory. The sender ups the refcount before > + * transmitting the memory to another worker. > + * > + * SharedArrayBufferObject (or really its underlying memory) /is nit: s/its/the/ @@ +90,5 @@ > { > static bool byteLengthGetterImpl(JSContext *cx, CallArgs args); > > public: > + // RAWBUF_SLOT holds a pointer (as "private" data) to the SharedArrayRawBuffer nit: "." at the end ::: js/src/vm/SharedTypedArrayObject.cpp @@ +65,5 @@ > + > +inline void > +InitSharedArrayBufferViewDataPointer(SharedTypedArrayObject *obj, SharedArrayBufferObject *buffer, size_t byteOffset) > +{ > + /* An old comment in ArrayBufferObject.h says: Either line comments, or \n before contents. @@ +74,5 @@ > + * > + * lth interprets this to mean that there's a chance that adding > + * the byteOffset to dataPointer will create a value that has some > + * of the lower bits set and that this will confuse a tagging > + * scheme somewhere, if we store it in a slot. Huh. Would be good to check this assumption with someone who might know. I guess bhackett, jorendorff, or Waldo would be good bets. @@ +154,5 @@ > + if (!obj) > + return nullptr; > + > + if (script) { > + if (!types::SetInitializerObjectType(cx, script, pc, obj, newKind)) I think merging these wouldn't be frowned upon. @@ +215,5 @@ > + * new Shared{Type}Array(length) > + * new Shared{Type}Array(SharedArrayBuffer, [optional] byteOffset, [optional] length) > + */ > + static bool > + class_constructor(JSContext *cx, unsigned argc, Value *vp) At some point we'll want to have "// Step N."-type comments for all these functions as we do for all ES-specced stuff nowadays. Your call if the spec is mature enough to do that at this point. If you think it is, we should do the comments now, because otherwise we won't for a long time. @@ +239,5 @@ > + args.rval().setObject(*obj); > + return true; > + } > + > + // The first argument is converted to a length by ToLength nit: "." at the end Also, given that we still only handle uint32-ranged lengths here, this is an implementation detail, right? Also also: I don't quite understand why this doesn't use the same code as js_Array (in jsarray.cpp) does. (Well, to be sure, I like the code here better than that, but it's not clear why we have such divergent implementations of essentially the same thing, used for the same reason). At the very least, can you change TypedArrays `create` to use the same code as used here, too, and get rid of `ValueIsLength`? If only to remove a function that sounds like it might refer to ES6's usage of the term `length`, but doesn't. @@ +276,5 @@ > + return nullptr; > + > + if (numByteOffset < 0 || numByteOffset >= 0x7FFFFFFF) { > + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, > + JSMSG_SHARED_TYPED_ARRAY_ARG_RANGE, "1"); Use the argument name instead of "1" here. @@ +286,5 @@ > + double numLength; > + if (!ToLength(cx, args[2], &numLength)) > + return nullptr; > + > + if (numLength < 0 || numLength >= 0x7FFFFFFF) { ToLength never returns negative values. Depending on whether we want clamp to [0,..) or report an error, either drop this part of the conditional or use ToInteger for the conversion above. @@ +288,5 @@ > + return nullptr; > + > + if (numLength < 0 || numLength >= 0x7FFFFFFF) { > + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, > + JSMSG_SHARED_TYPED_ARRAY_ARG_RANGE, "2"); Use the argument name instead of "2" here. @@ +437,5 @@ > + JS_ReportError(cx, "Permission denied to access object"); > + return nullptr; > + } > + > + if (!IsSharedArrayBuffer(bufobj)) { I don't see how this can possibly be true after the assert and `if` above. Preexisting in TypedArrayBuffer, I know. I wouldn't be opposed to removing it there, too. ::: js/src/vm/SharedTypedArrayObject.h @@ +18,5 @@ > + > +typedef struct JSProperty JSProperty; > +namespace js { > + > +/* Note that the representation of a SharedTypedArrayObject is the same as the I wouldn't mind single-line comments, but if you use block comments, then add a \n before the contents. @@ +109,5 @@ > +} // namespace js > + > +template <> > +inline bool > +JSObject::is<js::SharedTypedArrayObject>() const Might as well move this down and merge the two `namespace js` blocks. ::: js/src/vm/StructuredClone.cpp @@ +902,5 @@ > + uint64_t type = tarr->type(); > + if (!out.write(type)) > + return false; > + > + // Write out the SharedArrayBuffer tag and contents nit: "." at the end @@ +1162,5 @@ > + size_t nbytes = arrayBuffer->byteLength(); > + ArrayBufferObject::BufferContents bufContents = > + ArrayBufferObject::stealContents(context(), arrayBuffer); > + if (!bufContents) > + return false; // Destructor will clean up the already-transferred data nit: "." at the end @@ +1420,5 @@ > + JSMSG_SC_BAD_SERIALIZED_DATA, "unhandled typed array element type"); > + return false; > + } > + > + // Push a placeholder onto the allObjs list to stand in for the typed array nit: "." at the end @@ +1426,5 @@ > + Value dummy = UndefinedValue(); > + if (!allObjs.append(dummy)) > + return false; > + > + // Read the ArrayBuffer object and its contents (but no properties) nit: "." at the end @@ +1436,5 @@ > + if (!in.read(&n)) > + return false; > + byteOffset = n; > + RootedObject buffer(context(), &v.toObject()); > + RootedObject obj(context(), nullptr); no need to pass in nullptr here, you can just leave out the second arg entirely. @@ +1689,5 @@ > vp.setObject(*obj); > break; > } > > + case SCTAG_SHARED_TYPED_ARRAY_OBJECT: { Can you move this to just below SCTAG_TYPED_ARRAY_OBJECT? @@ +1690,5 @@ > break; > } > > + case SCTAG_SHARED_TYPED_ARRAY_OBJECT: { > + // readSharedTypedArray adds the array to allObjs nit: "." at the end ::: js/src/vm/TypedArrayCommon.h @@ +14,5 @@ > + > +namespace js { > + > +// Definitions below are shared between TypedArrayObject and SharedTypedArrayObject. > +// This code happens not to be according to ES6. I think you should slightly expand on what that means. Did you introduce changes that make our TypedArrays not conform to ES6? Are these preexisting issues, such as ToInteger instead of ToLength for index handling? @@ +44,5 @@ > + return false; > +} > + > +inline bool > +ToLength(JSContext *cx, HandleValue v, double *out) Huh, we don't have a ToLength implementation yet. Rather than adding it here, it'd be good to add it to the other To[numeric type] functions. I guess in the long run we'll need a JSAPI version anyway, so maybe do the same thing as for ToNumber and add one taking a JSContext and another internally-used one taking an ExclusiveContext? ::: js/src/vm/TypedArrayObject.cpp @@ +169,5 @@ > namespace { > > +// Note, this template can probably be merged (in part) with the one in > +// SharedTypedArrayObject.cpp once the implementation of TypedArrayObject > +// is closer to ES6. The trick is probably to pass an adapter Maybe a small aside here explaining what the relevant difference(s) is/are? ::: js/src/vm/TypedArrayObject.h @@ +34,5 @@ > + * duplication in the JITs: one code path can be used, with occasional > + * decision points based on the attributes. > + */ > + > +class TypedArrayLayout Would it make sense to move this to TypedArrayCommon.h? If so, add a comment linking to that here (and probably move the above comment over, too) and fix up the comment in SharedTypedArrayObject.h. @@ +44,5 @@ > + > +public: > + TypedArrayLayout(bool isShared, bool isNeuterable, const Class *firstClass, const Class *maxClass); > + > + /* Slot containing length of the view in number of typed elements */ Use single-line comments here and end with a ".", same as below in `class TypedArrayObject`. @@ +48,5 @@ > + /* Slot containing length of the view in number of typed elements */ > + static const size_t LENGTH_SLOT = JS_BUFVIEW_SLOT_LENGTH; > + > + /* The type value */ > + static const size_t TYPE_SLOT = JS_TYPEDARR_SLOT_TYPE; Given that these fields don't really line up, remove the extra white space before the "=" here and below.

Comment on attachment 8483360 [details] [diff] [review] Refactor common methods on TypedArray and SharedTypedArray Review of attachment 8483360 [details] [diff] [review]: ----------------------------------------------------------------- Yup, that works. r=me with nits addressed. ::: js/src/tests/js1_8_5/extensions/sharedtypedarray.js @@ +186,5 @@ > +function testSharedTypedArrayMethods() { > + var v = new SharedInt32Array(b); > + for ( var i=0 ; i < v.length ; i++ ) > + v[i] = i; > + nit: whitespace ::: js/src/vm/SharedArrayObject.h @@ +120,5 @@ > JS::ClassInfo *info); > > SharedArrayRawBuffer *rawBufferObject() const; > > + // This method does not cause GC. Why is this noteworthy? If it really is, add a brief explanation why. Otherwise it's somewhat distracting because it seems to indicate that something important is going on. ::: js/src/vm/TypedArrayCommon.h @@ +437,5 @@ > + Rooted<ArrayType *> thisTypedArray(cx, &thisTypedArrayObj->as<ArrayType>()); > + Rooted<ArrayType *> tarray(cx, &tarrayObj->as<ArrayType>()); > + JS_ASSERT(offset <= thisTypedArray->length()); > + JS_ASSERT(tarray->length() <= thisTypedArray->length() - offset); > + if (Adapter::sameBuffer(tarray->buffer(), thisTypedArray->buffer())) // Object equality is not good enough for SharedArrayBfferObject two nits: - comment should go above the code, because the line gets way too long otherwise - s/Bffer/Buffer/

Comment on attachment 8483361 [details] [diff] [review] Changes to Baseline JIT for SharedTypedArray Review of attachment 8483361 [details] [diff] [review]: ----------------------------------------------------------------- Ok, I think I can reasonably review these changes, as they're really about typed arrays, not baseline. I do wonder if there isn't any better way to test whether something has been inlined. Perhaps by retrieving the spew and searching for keywords automatically or something? No matter, that's probably way out of scope for this bug. ::: js/src/jit/BaselineIC.cpp @@ +3728,5 @@ > MOZ_CRASH("Procedure should never have been called."); > } > } > > +// FIXME: obj -> shape would be better Please create a bug for this and mention the bug number here and in the FIXMEs below.

Comment on attachment 8483363 [details] [diff] [review] Changes to Ion JIT for SharedTypedArray Review of attachment 8483363 [details] [diff] [review]: ----------------------------------------------------------------- Delightful how little has to change in the JITs.

(In reply to Till Schneidereit [:till] from comment #10) > Comment on attachment 8483359 [details] [diff] [review] > Reorganize ArrayBufferObject to split the shared and unshared case > > Review of attachment 8483359 [details] [diff] [review]: > ----------------------------------------------------------------- Very select followup (I've taken to heart everything else). > The large amounts of code duplication between > SharedTypedArrayObjectTemplate and TypedArrayObjectTemplate are really > unfortunate. As you note, a later patch in the set cleans this up somewhat. For the rest, we'll want to wait until TypedArray has been cleaned up, and then factor out more bits. On that note: > I don't quite understand why this doesn't use the same code > as js_Array (in jsarray.cpp) does. (Well, to be sure, I like the code > here better than that, but it's not clear why we have such divergent > implementations of essentially the same thing, used for the same > reason). At the very least, can you change TypedArrays `create` to use > the same code as used here, too, and get rid of `ValueIsLength`? If > only to remove a function that sounds like it might refer to ES6's > usage of the term `length`, but doesn't. TypedArrays need to be fixed but I think Waldo is working on that, and it's a big job. So I think I'll leave it alone for now. I don't want to get bogged down in possible regressions in TypedArray either, which will happen if I change its API as you suggest. The code I have for SharedTypedArray follows ES6 TypedArray, at least to a point (it does not mirror the prototype hierarchy). I wasn't aware of js_Array and I'm not sure it performs quite the same function. > While I see that "Racy" is more evocative than "Shared" and assume > that you want to call out that using this is potentially dangerous, I > still think that deviating from the terminology used everywhere else > isn't great. Hence, I think this should be called > "ArrayBufferObjectMaybeShared". I've thought long and hard about this and I've decided to keep it as it is, but if I get pushback in the second round of reviews I will change it. > ::: js/src/vm/TypedArrayObject.h > > +class TypedArrayLayout > Would it make sense to move this to TypedArrayCommon.h? Sadly no, since that would mean that TypedArrayObject.h must include TypedArrayCommon.h, yet the latter must include TypedArrayObject.h.

The first of a set of five patches containing all refactoring, new code, and test cases for the SharedArrayBuffer rewrite and the SharedTypedArray hierarchy. Sean: Till's been over the first four of these once. The middle three are substantially unchanged since that review. The first has a little bit of new code, mostly some bits that were missing in gc/, and the code around SharedTypedArray creation has been cleaned up in accordance with Till's wishes. The fifth patch is new.

Try run of this patch set in progress: https://tbpl.mozilla.org/?tree=Try&rev=c6eaa9af955f

(In reply to Lars T Hansen [:lth] from comment #14) > TypedArrays need to be fixed but I think Waldo is working on that, and > it's a big job. So I think I'll leave it alone for now. I don't want > to get bogged down in possible regressions in TypedArray either, which > will happen if I change its API as you suggest. Ok, that's fair. > > The code I have for SharedTypedArray follows ES6 TypedArray, at least to > a point (it does not mirror the prototype hierarchy). I wasn't aware of > js_Array and I'm not sure it performs quite the same function. Sorry, I wasn't very clear here. I only meant the argument coercion stuff, not the entire function. As long as both Arrays and (Shared)TypedArrays only deal with uint32-1 lengths/indices, having three very different ways of coercing arguments to a used length value seems redundant and error-prone. > > > While I see that "Racy" is more evocative than "Shared" and assume > > that you want to call out that using this is potentially dangerous, I > > still think that deviating from the terminology used everywhere else > > isn't great. Hence, I think this should be called > > "ArrayBufferObjectMaybeShared". > > I've thought long and hard about this and I've decided to keep it as > it is, but if I get pushback in the second round of reviews I will > change it. Ok, I can live with that. My suggested name isn't all that great, so I'm not married to it at all. > > > ::: js/src/vm/TypedArrayObject.h > > > +class TypedArrayLayout > > Would it make sense to move this to TypedArrayCommon.h? > > Sadly no, since that would mean that TypedArrayObject.h must include > TypedArrayCommon.h, yet the latter must include TypedArrayObject.h. Right, that makes sense :/

Comment on attachment 8487097 [details] [diff] [review] bug1054882-ArrayBufferReorg.diff Asking also for review from bzbarsky since I'm introducing new top-level names (albeit in Nightly only for the time being) and that requires review from a DOM peer, apparently. Boris, the new names are: Shared{Int,Uint}{8,16,32}Array, which can be applied to the existing (but rewritten) SharedArrayBuffer. See bug 1054841 for context and links to specs.

Comment on attachment 8487097 [details] [diff] [review] bug1054882-ArrayBufferReorg.diff Boris, nevermind, I see that I should ask you for review on the update to the Mochitest (which is forthcoming). Sorry for the bother.

Adjustments to Mochitests for new top-level names in Nightly.

These are very minor point fixes to the first and fifth patch to fix compilation issues on some platforms and to fix an error I introduced in asm.js.

Try run is green with the full patch set: https://tbpl.mozilla.org/?tree=Try&rev=2df92baa9ff3

Comment on attachment 8487968 [details] [diff] [review] bug1054882-ArrayBufferMochiTests.diff A few things: 1) The ecmaGlobals bits are owned by the JS engine peers, not DOM peers, since they're part of the engine, not of the DOM. 2) The "spec" linked to seems to just be a Google doc, not an actual public spec that anyone has been asked to comment on. What standard body has this been submitted to, if any? If none, which ones are you planning to submit it to? If none, please don't call it a "spec". ;) 3) This could probably use an intent to implement mail. See https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Implement

(In reply to Boris Zbarsky [:bz] from comment #27) > Comment on attachment 8487968 [details] [diff] [review] > bug1054882-ArrayBufferMochiTests.diff > > 1) The ecmaGlobals bits are owned by the JS engine peers, not DOM peers, > since they're part of the engine, not of the DOM. OK. (I was confused by the error message on Try.) Thanks for the redirect. > 2) The "spec" linked to seems to just be a Google doc, not an actual public > spec that anyone has been asked to comment on. Lots of people have been asked to comment on this, and a number of people have, both within Mozilla and elsewhere, but there has not been a globally visible announcement. (The doc is commentable by anyone who wants to.) > What standard body has this been submitted to, if any? None at this time. > If none, which ones are you planning to submit it to? If none, please don't call it a "spec". ;) The eventual destination is Ecma TC39. > 3) This could probably use an intent to implement mail. See > https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Implement Thanks for the pointer, I will investigate.

Comment on attachment 8487097 [details] [diff] [review] bug1054882-ArrayBufferReorg.diff Review of attachment 8487097 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this. Mostly nits. ::: js/src/jsapi.cpp @@ +80,5 @@ > #include "vm/SharedArrayObject.h" > #include "vm/StopIterationObject.h" > #include "vm/StringBuffer.h" > #include "vm/Symbol.h" > +#include "vm/TypedArrayCommon.h" TypedArrayCommon.h necessarily includes both TypedArrayObject.h and indirectly SharedArrayObject.h, so both of those includes can be removed here. ::: js/src/jsnum.h @@ +255,5 @@ > +ToLength(JSContext *cx, HandleValue v, double *out) > +{ > + if (!ToNumber(cx, v, out)) > + return false; > + *out = ToInteger(*out); Historically, numeric code that has attempted to follow the specification literally wrt Number has been a source of performance issues from the double <=> int conversion. This particular code seems to only be used in class creation methods, so it's not terribly important, but I would strongly prefer to avoid double code on the main path if possible. The similar code in ValueIsLength() seems reasonable and spec-compliant in many circumstances here. @@ +306,5 @@ > } > > +// Ditto ToLength. > +MOZ_ALWAYS_INLINE bool > +ToLength(ExclusiveContext *cx, HandleValue v, double *out) Would a template on the first parameter be appropriate here? ::: js/src/tests/js1_8_5/shell.js @@ +4,5 @@ > * http://creativecommons.org/licenses/publicdomain/ > */ > > +if (typeof assertThrowsInstanceOf === 'undefined') { > + var assertThrowsInstanceOf = function assertThrowsInstanceOf(f, ctor, msg) { This code appears to be copied from tests/ecma_6/shell.js, but that code is MPL2, and this file is CC-PD. The function is probably too trivial for us to actually care about it, and I don't know who would have authorization for code relicensing anyway. Should be fine due to triviality. ::: js/src/vm/ArrayBufferObject-inl.h @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef vm_ArrayBufferCommon_h > +#define vm_ArrayBufferCommon_h This file (at least according to the review tool) is called "ArrayBufferObject-inl.h". "ArrayBufferCommon.h" (or possibly "ArrayBufferObjectCommon.h"?) do seem like better names. If the name is correct but the includes are misleading, be sure to also change the comment at the bottom of the file. @@ +12,5 @@ > +#include "vm/ArrayBufferObject.h" > + > +#include "js/Value.h" > + > +#include "vm/SharedArrayObject.h" If this file is renamed, the include order will have to change, or check_spidermonkey_style.py will burn the tree on checkin. @@ +17,5 @@ > + > +namespace js { > + > +inline uint32_t > +AnyArrayBufferByteLength(const ArrayBufferObjectMaybeRacy *buf) { nit: { on new line @@ +24,5 @@ > + return buf->as<SharedArrayBufferObject>().byteLength(); > +} > + > +inline uint8_t * > +AnyArrayBufferDataPointer(const ArrayBufferObjectMaybeRacy *buf) { nit: { on new line @@ +31,5 @@ > + return buf->as<SharedArrayBufferObject>().dataPointer(); > +} > + > +inline ArrayBufferObjectMaybeRacy & > +AsAnyArrayBuffer(HandleValue val) { nit: { on new line @@ +37,5 @@ > + return val.toObject().as<ArrayBufferObject>(); > + return val.toObject().as<SharedArrayBufferObject>(); > +} > + > +} nit: // namespace js ::: js/src/vm/ArrayBufferObject.h @@ +56,5 @@ > +// > +// As ArrayBufferObject and SharedArrayBufferObject are separated, so are the > +// TypedArray hierarchies below the two. However, the TypedArrays have the > +// same layout (see TypedArrayObject.h), so there is little code duplication > +// in SpiderMonkey. Can go without "in SpiderMonkey" and terminate on previous line. @@ +61,4 @@ > > typedef Vector<ArrayBufferObject *, 0, SystemAllocPolicy> ArrayBufferVector; > > +class ArrayBufferObjectMaybeRacy; I would strongly prefer using existing terminology with "ArrayBufferObjectMaybeShared". There has historically been mild confusion around the distinction between the (never-shared) memory for the Object and the (maybe-shared) memory for the Buffer that the Racy naming seems to perpetuate. @@ +87,5 @@ > * access. It can be created explicitly and passed to an ArrayBufferViewObject > * subclass, or can be created implicitly by constructing a TypedArrayObject > * with a size. > + * > + * ArrayBufferObject /is not racy/: the memory is private to a single worker. SharedArrayBufferObject also isn't racy, and its memory is also private to a single worker! With the exception of a single pointer to a shared allocation. ::: js/src/vm/SharedArrayObject.cpp @@ +13,5 @@ > # include "jswin.h" > #else > # include <sys/mman.h> > #endif > +#include "jswrapper.h" This will (or should, per intent) break check_spidermonkey_style.py on non-XP_WIN systems. To keep it in the required order, it must be: #ifdef XP_WIN # include "jswin.h" #endif #include "jswrapper.h" #ifndef XP_WIN # include <sys/mman.h> #endif @@ +22,5 @@ > > #include "mozilla/Atomics.h" > > #include "asmjs/AsmJSValidate.h" > +#include "vm/SharedTypedArrayObject.h" TypedArrayCommon.h necessarily includes SharedTypedArrayObject.h, so it isn't necessary to also include it here. @@ +260,5 @@ > SharedArrayBufferObject::rawBufferObject() const > { > + Value v = getReservedSlot(RAWBUF_SLOT); > + JS_ASSERT(!v.isUndefined()); > + return reinterpret_cast<SharedArrayRawBuffer *>(v.toPrivate()); Unrelated to this patch, it just occurred to me that I never integrated the SharedArrayRawBuffer with about:memory. It would be nice to do so, if njn hasn't handled it already. ::: js/src/vm/SharedTypedArrayObject.h @@ +16,5 @@ > +#include "vm/SharedArrayObject.h" > +#include "vm/TypedArrayObject.h" > + > +typedef struct JSProperty JSProperty; > +namespace js { nit: vertical whitespace before this line ::: js/src/vm/TypedArrayCommon.h @@ +60,5 @@ > +template<> inline Scalar::Type TypeIDOfType<double>() { return Scalar::Float64; } > +template<> inline Scalar::Type TypeIDOfType<uint8_clamped>() { return Scalar::Uint8Clamped; } > + > +inline bool > +IsAnyTypedArray(HandleObject obj) { nit: whole file after this point needs { on new line -- only exception to this rule is for inline functions within class declarations (in the same file, indented) @@ +137,5 @@ > +IsAnyTypedArrayClass(const Class *clasp) { > + return IsTypedArrayClass(clasp) || IsSharedTypedArrayClass(clasp); > +} > + > +} nit: // namespace js, and vertical whitespace after. ::: js/src/vm/TypedArrayObject.cpp @@ +35,5 @@ > #include "vm/GlobalObject.h" > #include "vm/Interpreter.h" > #include "vm/NumericConversions.h" > #include "vm/SharedArrayObject.h" > +#include "vm/TypedArrayCommon.h" SharedArrayObject.h may be removed. ::: js/src/vm/TypedArrayObject.h @@ +41,5 @@ > + const bool isNeuterable_; > + const Class *firstClass_; > + const Class *maxClass_; > + > +public: nit: 2-space indent

Comment on attachment 8487098 [details] [diff] [review] bug1054882-TypedArrayMethods.diff Review of attachment 8487098 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: js/src/tests/js1_8_5/extensions/sharedtypedarray.js @@ +173,5 @@ > +function testSharedTypedArrayMethods() { > + var v = new SharedInt32Array(b); > + for ( var i=0 ; i < v.length ; i++ ) > + v[i] = i; > + nit: extraneous whitespace ::: js/src/vm/SharedTypedArrayObject.cpp @@ +104,5 @@ > static const size_t BYTES_PER_ELEMENT = sizeof(ThisType); > > + class SharedTypedArrayObjectAdapter > + { > + public: nit: 2-space indent for "public:" @@ +115,5 @@ > + return true; > + } > + > + static bool sameBuffer(SharedArrayBufferObject *a, SharedArrayBufferObject *b) { > + return a->globalID() == b->globalID(); This is nice. ::: js/src/vm/TypedArrayCommon.h @@ +150,5 @@ > + typedef typename Adapter::ElementType NativeType; > + typedef typename Adapter::TypedArrayObjectType ArrayType; > + typedef typename Adapter::ArrayBufferObjectType BufferType; > + > +public: nit: 2-space indent for "public:" @@ +188,5 @@ > + uint32_t length = tarray->length(); > + uint32_t begin = 0, end = length; > + > + if (args.length() > 0) { > + if (!ToClampedIndex(cx, args[0], length, &begin)) While we're here, this would look better as: if (args.length() > 0 && !To...) return false; if (args.length() > 1 && !To...) return false; @@ +216,5 @@ > +{ > + typedef typename Adapter::ElementType NativeType; > + typedef typename Adapter::TypedArrayObjectType ArrayType; > + > +public: nit: 2-space indent @@ +310,5 @@ > +{ > + typedef typename Adapter::ElementType NativeType; > + typedef typename Adapter::TypedArrayObjectType ArrayType; > + > +public: nit: 2-space indent ::: js/src/vm/TypedArrayObject.cpp @@ +188,5 @@ > static const size_t BYTES_PER_ELEMENT = sizeof(ThisType); > > + class TypedArrayObjectAdapter > + { > + public: nit: 2-space indent

Comment on attachment 8487099 [details] [diff] [review] bug1054882-ArrayBufferBaselineJIT.diff Review of attachment 8487099 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/sharedbuf/inline-access.js @@ +17,5 @@ > +function f(ta) { > + return (ta[2] = ta[0] + ta[1] + ta.length); > +} > + > +var v = new SharedInt32Array(1024); This test (like the other tests in sharedbuf/) must be conditional on the existence of the SharedArrayBuffer, otherwise these tests will start burning the tree once we promote from Nightly.

Comment on attachment 8487100 [details] [diff] [review] bug1054882-ArrayBufferIonJIT.diff Review of attachment 8487100 [details] [diff] [review]: ----------------------------------------------------------------- This patch is acceptable to include for the moment, but I don't understand the interplay between this patch and Jukka's "volatile" designation. Are we eventually going to distinguish between TypedArrayObject and SharedTypedArrayObject on a MIR level to inhibit some compiler optimizations? ::: js/src/jit/IonCaches.h @@ +19,5 @@ > namespace js { > > class LockedJSContext; > class TypedArrayObject; > +class SharedTypedArrayObject; Could we just include TypedArrayCommon.h? @@ +600,5 @@ > } > bool monitoredResult() const { > return monitoredResult_; > } > + bool hasTypedArrayLengthStub(HandleObject obj) const { Is there some way to name this function that makes it obvious that SharedTypedArrayLengthStubs are included also? Perhaps "hasSomeTyped.."? @@ +1137,5 @@ > } > TypedOrValueRegister output() const { > return output_; > } > + bool hasTypedArrayLengthStub(HandleObject obj) const { Ditto. ::: js/src/jit/MCallOptimize.cpp @@ +230,5 @@ > // typed array prototype, and make sure we are accessing the right one > // for the type of the instance object. > if (thisTypes) { > + Scalar::Type type; > + type = thisTypes->getTypedArrayType(); Prefer vertical whitespace between these two lines. @@ +236,5 @@ > MInstruction *length = addTypedArrayLength(callInfo.thisArg()); > current->push(length); > return InliningStatus_Inlined; > } > + type = thisTypes->getSharedTypedArrayType(); Prefer one line of vertical whitespace before this line. ::: js/src/jit/MIR.h @@ +23,5 @@ > #include "jit/TypedObjectPrediction.h" > #include "jit/TypePolicy.h" > #include "vm/ScopeObject.h" > +#include "vm/SharedTypedArrayObject.h" > +#include "vm/TypedArrayCommon.h" Is SharedTypedArrayObject.h needed here? Should be covered by TypedArrayCommon.h. TypedArrayObject.h below can also be removed. @@ +8081,1 @@ > {} It would be nice to assert that someTypedArray is actually some sort of TypedArray, although viewType() will catch that later.

Comment on attachment 8487972 [details] [diff] [review] bug1054882-BugFixes.diff Review of attachment 8487972 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsnum.cpp @@ +1699,5 @@ > return true; > } > > +bool > +js::ToLength(JSContext *cx, HandleValue v, double *out) I'm still not a fan of these functions. If they're in a CPP file, they should definitely be templated and instantiated.

Brief followup to second round of reviews. I have taken all review comments into account (and notably cleaned up ToLength substantially). Brief remarks about the following two outstanding items: ----- From review of ArrayBufferReorg.diff ----- > @@ +260,5 @@ > > SharedArrayBufferObject::rawBufferObject() const > > { > > + Value v = getReservedSlot(RAWBUF_SLOT); > > + JS_ASSERT(!v.isUndefined()); > > + return reinterpret_cast<SharedArrayRawBuffer *>(v.toPrivate()); > > Unrelated to this patch, it just occurred to me that I never integrated the > SharedArrayRawBuffer with about:memory. It would be nice to do so, if njn > hasn't handled it already. I will file investigate/fix of this as followup item. ----- From review of ArrayBufferIonJIT.diff ----- > This patch is acceptable to include for the moment, but I don't understand the > interplay between this patch and Jukka's "volatile" designation. Are we > eventually going to distinguish between TypedArrayObject and > SharedTypedArrayObject on a MIR level to inhibit some compiler optimizations? At the moment I believe we are not intending to do that. We are instead going to use Atomics.load and Atomics.store to access volatile variables. In principle that will cause a performance hit, but there are some mitigating circumstances: - on x86 there will only be an extra cost on volatile stores, since there will be an MFENCE following such a store that would not be needed on "normal" volatile stores in C++. (Factual.) - volatiles are basically unreliable on weakly ordered memory systems (anything but x86 and SPARC) and using the atomics will probably make some ported x86 code run on weakly ordered systems that would not otherwise run. (Speculative.) - until we know that the performance hit is actually really bad it's a simplification. (Smaller API surface.) If the performance hit turns out to be bad - after we've implemented desirable optimizations to the atomics - and that mapping volatiles to atomics does not make more programs run, then we should probably introduce Atomics.loadRelaxed() and Atomics.storeRelaxed() and map volatile loads and stores to those.

Comment on attachment 8487968 [details] [diff] [review] bug1054882-ArrayBufferMochiTests.diff Oh look, Till is also a module peer.

Rollup patch. https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9ed5c35329 https://tbpl.mozilla.org/?tree=Try&rev=259312fdc348

This seems to break devices, as reported in bug 1068539

(In reply to Alexandre LISSY :gerard-majax from comment #38) > This seems to break devices, as reported in bug 1068539 This patch did introduce some new data types, but I don't know how that would impact users of the engine, given that the types are new. I suppose it is possible that there are data type IDs in the serialized representation and that the patch changes those, but that would have been inadvertent, and indeed looking in StructuredClone.cpp the ID that I added is at the end of the ID list. However it could be that the END_OF_KEYS item itself has a new ID and that that causes some confusion. I'll investigate a little further, though Jason will know more about the dangers of hacking on that code.

Ah, this is almost certainly what's going on: The old value of END_OF_KEYS coincides with the new value of SHARED_TYPED_ARRAY_OBJECT. Deserialization dispatches to the routine for the latter when it sees the former. The subsequent error is then entirely as expected. Normally one would expect there to be a guard against these kinds of mishaps, such as a serial number or a length field providing the number of entries to be interpreted, or a stable end value. I haven't found anything like that yet.

I suppose it's more important for END_OF_KEYS to have a stable value than to be the last key in the list. Maybe that's the fundamental mistake - new keys should come after END_OF_KEYS in the list.

(In reply to Lars T Hansen [:lth] from comment #41) > I suppose it's more important for END_OF_KEYS to have a stable value than to > be the last key in the list. Maybe that's the fundamental mistake - new > keys should come after END_OF_KEYS in the list. That seems like the correct analysis. A patch is attached to bug 1068539.

There's a build warning x9 that leads to this landing: /home/ben/code/moz/inbound/js/src/vm/SharedTypedArrayObject.cpp:529:29: warning: unused variable 'key' [-Wunused-const-variable] static const JSProtoKey key = JSProto_SharedInt8Array; ^

Comment on attachment 8491419 [details] [diff] [review] warning.patch Review of attachment 8491419 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. Those lines were removed at some point and I don't know how they came back, but they're supposed to be gone...

Rebasing is often the right culprit :) https://hg.mozilla.org/integration/mozilla-inbound/rev/ecbaea72e7d3

For future reference, "Rollup patch" is a terrible commit message.