In preparation for TC39 we have to at least explore the consequences of using TypedArray instead of SharedTypedArray to map SharedArrayBuffer. (SharedArrayBuffer should remain intact.) The main issue internally in the JS engine and Firefox is that C++ makes data races undefined behavior. To avoid all races in the C++ code -- and the consensus so far is that it is necessary to avoid them -- it is necessary for all code that touches buffer memory from a TypedArray to be aware of whether the memory might be shared or not, and to use safe-for-races operations when the memory might be shared. In many cases code can just use safe-for-races operations always, but code must always be aware. The easiest fix I've found for this is to wrap a pointer to buffer memory in a structure that carries information about whether the memory is shared or not; the act of unwrapping the pointer forces client code to take a stand on whether it will break down by cases or always assume shared memory. This turns out to be not too gross in practice. Safe-for-races operations cannot be written in C++ but must be written in assembler, must be jitted, or must be written using C++ intrinsics that are safe-for-races but not synchronizing (none are known to me as of yet). (A secondary issue is that the TypedArray API must adapt, somehow, since it assumes ArrayBuffer in some places where it will not always be appropriate.)

This patch really belongs to bug 1084248 - it gets rid of undefined behavior internally in the engine, with the current SharedTypedArray setup. It introduces a notion of safe-for-races operations in the AtomicOperations suite.

This patch removes SharedTypedArray and makes a large number of adjustments throughout the system (see initial description). Many test cases have been corrected here, but there are several FIXMEs scattered throughout. What this needs are at least the following: - fix the FIXMEs - test cases - actually implement the safe operations (but SMOP) - clean up TypedArray semantics - currently biased toward unshared memory, eg, should ta.slice() return shared or unshared memory if the underlying buffer is shared? Lots of cases like this, probably a real mess. - Atomics are inconsistent: the interpreter versions will only work on shared memory but the JIT versions will work also on unshared memory. Obviously we should not force the JIT versions to make an extra check so if we want to keep the restriction (I'm indifferent, except for futexes) then the type propagation in the JIT needs to somehow figure out the underlying memory type without the benefit of an array type.

Work in progress, requires the patches on bug 1084243.

General remarks about the upcoming patch set: The patch set is very large because of API changes to the JS engine both in C++ and in JS. There are additional changes I need to make (https://docs.google.com/spreadsheets/d/1PFa3aDxY6mffT8uoflCaFitX9lKj_Y4_aZwtMApIRiI/edit?usp=sharing, look for "Mop-up work") but the patches here can stand on their own, don't break anything, and don't make anything worse. There are three fundamental patches to the engine, called "preliminary adjustments", "core changes", and "builtin-lib changes". These rip out the SharedTypedArray hierarchy and allow TypedArray to be applied to SharedArrayBuffer. Everything else is either (minor) JIT adjustments, (mostly) superficial test adjustments, or DOM/gecko adjustments as a result of API changes.

Trivial-ish patch: Remove engine and test files affected by removal of SharedTypedArray.

Another trivial-ish patch: some naming changes, indentation fixes, and code removal, plus a couple of changes that foreshadow the next patch.

The heart of the matter: remove SharedTypedArray and make TypedArray apply to SharedArrayBuffer. "Easy" changes to the builtins and selfhosting code are largely in the next patch. This also removes TypedArrayLayout. I could have removed TypedArrayCommon.h but I'd like to do that in a follow-up bug in order to keep the churn manageable.

Builtins and self-hosting code.

This creates machinery that implements dynamic type checks to guard atomic operations, which are legal only on TypedArrays that map shared memory. The patch also sets up a structure for optimizing away those type checks, but I want to defer that optimization to a follow-up bug.

Minor adjustments to Odin (since we already allow the TypedArray constructors to be used with shared memory): just remove knowledge of the SharedTypedArray constructors.

Necessary changes to CTypes, xpconnect, and the shell. Basically these all just make sure we opt out of using shared memory. Opting in is follow-up work.

Comment on attachment 8687249 [details] [diff] [review] bug1176214-part6-odin-changes.patch Review of attachment 8687249 [details] [diff] [review]: ----------------------------------------------------------------- w00t

Change the jit-tests to deal with the removal of SharedTypedArray. Note that part1 also removed a couple of jit-test files entirely.

Ditto changes for the tests/ directory.

And adjustments to jsapi-tests. More tests are needed here, but that's follow-up work.

Bz, you may remember that we discussed this change in Whistler. There are basically three different types of change here: First, TypedArray is given awareness of whether memory is shared, and anyone who uses existing APIs on TypedArray (Length and Data) will receive a zero length and a null pointer if the memory happens to be shared. There are new APIs on TypedArray to get the length and data if the memory is shared (the client thus opts in to shared memory); the subsequent WebGL patch uses these now. Second, code that goes directly to the JSAPI to get the length and data pointer have to use slightly altered APIs and are forced to consider whether memory is shared, and can choose whether to opt in or opt out; all code except code in WebGL has been changed to opt out. Third, clients that call TypedArray::Obj() are modified to throw better errors when the memory is shared; I don't think this is necessary for safety, as the other two mechanism guard access to shared memory, but it should improve usability. There's a follow-up bug that's coming that pertains to this and the next patch: the JS engine needs to export its APIs for safely accessing memory that can be racy from C++. But the present patches don't make anything any worse than it was before.

This adapts Jukka's previous WebGL work to use just TypedArrays again, but opting in to using shared memory (see comment on previous patch). The test case passes. Jukka, it would be lovely if you could run any other tests you might have lying around.

Minor changes to deal with changed JS engine API and to opt out of using shared memory (for now).

Minor changes to adapt to a new JS engine API and to opt out of using JS shared memory (for now).

Minor change to adapt to a changed JS engine API and to opt out of using shared memory (for now).

Comment on attachment 8687252 [details] [diff] [review] bug1176214-part8-jit-test-changes.patch Review of attachment 8687252 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: js/src/jit-test/tests/asm.js/sta-transition.js @@ -3,5 @@ > -// memory but the former only when an atomic operation is referenced, > -// as per spec. Eventually it will stop accepting "SharedInt32Array", > -// because that name is going away. > -// > -// These should not run with --no-asmjs. Can you leave the comment or make it that this test also run with --no-asmjs? ::: js/src/jit-test/tests/atomics/basic-tests.js @@ +368,5 @@ > return sum; > } > > function adHocExchange() { > + var a = new Int8Array(new SharedArrayBuffer(16*4)); Why 16 * 4? ::: js/src/jit-test/tests/atomics/optimization-tests.js @@ +83,5 @@ > sum += Atomics.or(ia, 7, 1); > } > > function mod(stdlib, ffi, heap) { > "use asm"; If you want to test that this is actually run in asm.js mode, you need to use asmCompile (resp. asmLink) to make sure it validates (resp. links) in the first place. ::: js/src/jit-test/tests/sharedbuf/inline-access.js @@ +20,5 @@ > function f(ta) { > return (ta[2] = ta[0] + ta[1] + ta.length); > } > > +var v = new Int32Array(new SharedArrayBuffer(1024)); Not equivalent to the previous test (sizes differ). Not a big deal, though.

Comment on attachment 8687253 [details] [diff] [review] bug1176214-part9-tests-changes.patch Review of attachment 8687253 [details] [diff] [review]: ----------------------------------------------------------------- Unless I've missed something, a lot of tests should be conditional upon the existence of this.SharedArrayBuffer, to not make the build infrastructure crazy on the next merge day :-) r+ conditional on that ::: js/src/tests/ecma_6/TypedArray/constructor-not-callable.js @@ +8,5 @@ > Uint32Array, > Float32Array, > + Float64Array, > + sharedConstructor(Int8Array), > + sharedConstructor(Uint8Array), Does sharedConstructor(Uint8ClampedArray) need to be added to this list (and quite a few others)? ::: js/src/tests/ecma_6/TypedArray/entries.js @@ +34,5 @@ > assertDeepEq(iterator.next(), {value: [2, 3], done: false}); > assertDeepEq(iterator.next(), {value: undefined, done: true}); > > // Called from other globals. > + if (typeof newGlobal === "function" && !constructor.__isShared__) { I'd rather have another helper "isShared(obj)" in shell.js, but it's test only, so i don't have too much a strong opinion. (this comment applies to quite a few other test files as well) ::: js/src/tests/ecma_6/TypedArray/shell.js @@ +9,5 @@ > + throw new TypeError("Not callable"); > + var x = new constructor(...args); > + var b = x.buffer; > + var o = x.byteOffset; > + var l = x.length; Please use meaningful variable names here (there are so many that it's a bit confusing, plus you can use destructured assignment): var {buffer, byteOffset, length} = x; @@ +10,5 @@ > + var x = new constructor(...args); > + var b = x.buffer; > + var o = x.byteOffset; > + var l = x.length; > + var y = new SharedArrayBuffer(b.byteLength); What if SharedArrayBuffer is not defined in the current global? ::: js/src/tests/ecma_7/SIMD/conversions.js @@ +91,5 @@ > INT8_MAX, INT8_MIN, INT8_MAX, INT8_MIN, INT8_MAX, INT8_MIN, INT8_MAX, INT8_MIN]]; > for (var v of vals) { > var i = Int8x16(...v); > + assertEqX4(Float32x4.fromInt8x16Bits(i), expected(i, ArrayBuffer)); > + assertEqX4(Float32x4.fromInt8x16Bits(i), expected(i, SharedArrayBuffer)); This should be made conditional upon the existence of this.SharedArrayBuffer, right? ::: js/src/tests/js1_8_5/extensions/sharedtypedarray.js @@ +83,3 @@ > > // not congruent mod element size > + assertThrowsInstanceOf(() => new Int32Array(b, 3), TypeError); // Likely a bug that this is not RangeError Follow-up?

Comment on attachment 8687254 [details] [diff] [review] bug1176214-part10-jsapi-tests-changes.patch Review of attachment 8687254 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you really need it; however, lacking the context of the jsapi changes, I'd suggest folding this patch with the patch changing the API (there really aren't not that many changes here).

Comment on attachment 8687257 [details] [diff] [review] bug1176214-part12-dom-webgl.patch Review of attachment 8687257 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, r+. I suppose the WebIDL changes to WebGL were done already in a previous commit, since I don't see those changes in this?

(In reply to Jukka Jylänki from comment #32) > Comment on attachment 8687257 [details] [diff] [review] > bug1176214-part12-dom-webgl.patch > > Review of attachment 8687257 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me, r+. I suppose the WebIDL changes to WebGL were done > already in a previous commit, since I don't see those changes in this? Good point. The WebIDL changes are in the other DOM patch. I'll add a r? on that for you.

Comment on attachment 8687255 [details] [diff] [review] bug1176214-part11-dom-except-webgl.patch Jukka, maybe you can look at the WebIDL changes too.

Comment on attachment 8687246 [details] [diff] [review] bug1176214-part5-ion-changes.patch Review of attachment 8687246 [details] [diff] [review]: ----------------------------------------------------------------- Cool to see them merged in the code. (Ps: A Value to save 1 bit seems a bit wasteful...) ::: js/src/jit/MCallOptimize.cpp @@ +2978,5 @@ > } > > void > +IonBuilder::atomicsCheckType(MDefinition* obj) > +{ Not sure I understand the need for this function. Can't we just call "addSharedtypeArrayGuard" instead of using this function? Seems more descriptive too. ::: js/src/jit/MIR.h @@ +13074,5 @@ > +// if the obj does not map a SharedArrayBuffer. > + > +class MGuardSharedTypedArray > + : public MUnaryInstruction, > + public ObjectPolicy<0>::Data public SingleObjectPolicy::Data ::: js/src/vm/TypeInference.h @@ +729,5 @@ > + * If there is such a common type and sharedness is not nullptr then > + * *sharedness is set to what we know about the sharedness of the memory. > + */ > + Scalar::Type getTypedArrayType(CompilerConstraintList* constraints, > + TypedArraySharedness* sharedness=nullptr); style nit: let the equal sign breath with spaces before and after.

Comment on attachment 8687238 [details] [diff] [review] bug1176214-part2-preliminary-adjustments.patch Review of attachment 8687238 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TypedArrayCommon.h @@ +74,5 @@ > > inline bool > IsAnyTypedArray(JSObject* obj) > { > + return obj->is<TypedArrayObject>(); I hope I see a followup patch removing IsAnyTypedArray and friends at some point here, but fine as an incremental step. @@ +495,4 @@ > template<typename SomeTypedArray> > class TypedArrayMethods > { > + static_assert(mozilla::IsSame<SomeTypedArray, TypedArrayObject>::value, Also expect to see something eventually here that removes the template parameter, if it's always going to have one value. @@ +774,5 @@ > + switch (target->type()) { > + case Scalar::Int8: > + if (isShared) > + return ElementSpecific<Int8ArrayType, SharedOps>::setFromAnyTypedArray(cx, target, source, offset); > + return ElementSpecific<Int8ArrayType, UnsharedOps>::setFromAnyTypedArray(cx, target, source, offset); Such grody.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #36) > Comment on attachment 8687238 [details] [diff] [review] > bug1176214-part2-preliminary-adjustments.patch > > > I hope I see a followup patch removing IsAnyTypedArray and friends at some > point here, but fine as an incremental step. That is bug 1225031, I will reference your comments from that.

Comment on attachment 8687255 [details] [diff] [review] bug1176214-part11-dom-except-webgl.patch Review of attachment 8687255 [details] [diff] [review]: ----------------------------------------------------------------- Perfect!

Comment on attachment 8687255 [details] [diff] [review] bug1176214-part11-dom-except-webgl.patch >+++ b/dom/base/Crypto.cpp >+ aRv.Throw(NS_ERROR_DOM_TYPE_MISMATCH_ERR); It's probably better to throw the same sort of exception that would be thrown if the caller passed something that's not a valid arg at all. Probably best is to add a new string with JSEXN_TYPEERR to dom/bindings/Errors.msg, perhaps like so: MSG_DEF(MSG_TYPEDARRAY_IS_SHARED, 1, JSEXN_TYPEERR, "{0} can't be a shared typed array") and then here do: aRv.ThrowTypeError<MSG_TYPEDARRAY_IS_SHARED>(&NS_LITERAL_STRING("Argument of Crypto.getRandomValues")); or some such. And at other places where we want to throw this sort of exception just adjust the argument to ThrowTypeError. >+++ b/dom/bindings/TypedArray.h >+ void GetLengthAndData(JSObject*, uint32_t*, bool*, T**)> Should probably call it GetLengthAndDataAndSharedness. Or just GetBufferInfo or something. Same thing in the TypedArray and ArrayBufferView_base struct templates. >+ // About shared memory: So DataViews never map SharedArrayBuffers? >+ // the view has been computed; the JS_GetTypedArraySharedness() That just works on typed arrays, right? Is there a way to check whether an ArrayBuffer is shared or not? I guess JS_IsSharedArrayBuffer, right. >+ // Callers of Obj() that do not opt into shared memory I'm not sure what it means for a caller of Obj() to opt into shared memory. >+ inline bool shared() const { IsShared() or Shared(), please (with capital first letter). >+++ b/dom/canvas/ImageData.cpp Again, want to aRv.ThrowTypeError here. >+++ b/dom/crypto/CryptoBuffer.h >+ void GetLengthAndData(JSObject*, uint32_t*, bool*, T**)> Again, might want a better name. >+++ b/dom/fmradio/FMRadio.cpp >+ if (isShared) { How could it be shared? We just created it, and we didn't create it shared... >+++ b/dom/media/webaudio/AudioBuffer.cpp >@@ -149,17 +151,23 @@ AudioBuffer::CopyFromChannel(const Float >+ if (isShared) { I believe the only place channelArray can come from (the only place that adds things to mJSChannels) is AudioBuffer::RestoreJSChannelData which puts in return values from JS_NewFloat32Array, so isn't shared. >@@ -190,18 +198,24 @@ AudioBuffer::CopyToChannel(JSContext* aJ Same here. >@@ -237,18 +251,25 @@ AudioBuffer::StealJSArrayDataIntoSharedC And here. >+++ b/dom/media/webaudio/AudioContext.cpp ThrowTypeError as described above. >+++ b/dom/workers/XMLHttpRequest.cpp ThrowTypeError as above. I guess TCPSocket::Send is not a problem because it takes ArrayBuffer, not ArrayBufferView? But I think it would be good to change the necko patch to check for sharedness in SetData(), not much later in ReadSegments(). r=me with the above fixed.

Comment on attachment 8687257 [details] [diff] [review] bug1176214-part12-dom-webgl.patch A question: Given an ArrayBufferView backed by a SharedArrayBuffer, can that SharedArrayBuffer be detached from another thread? I'm assuming no: that the other thread has a different SharedArrayBuffer with the same data pointer, but that detaching that doesn't actually affect the thing we see, right? Put another way, is it true that the valid data length can't randomly change as we go along, as long as we don't call into JS? >@@ -208,18 +208,20 @@ WebGL2Context::GetBufferSubDataT(GLenum >+ // need to use safe operations when those become available. Can this comment please come with a bug number, with that bug depending on the bug about safe operations? And can other uses of DataAllowShared() please come with comments explaining why they're safe? r=me. Sorry for the terrible lag. :(

Comment on attachment 8687241 [details] [diff] [review] bug1176214-part3-core-changes.patch Review of attachment 8687241 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TypedArrayObject.h @@ +55,5 @@ > + static_assert(IS_SHAREDMEM_SLOT == JS_TYPEDARRAYLAYOUT_IS_SHAREDMEM_SLOT, > + "self-hosted code with burned-in constants must get the " > + "right byteOffset slot"); > + > + static const size_t RESERVED_SLOTS = 7; // Three slots unused This 3->7 (minimum 4->8 size class) change seems unideal. I suppose it's at least *possible* we could weather it, but I'm skeptical we actually could. Fortunately, I don't think we need to. We still have plenty of space in ObjectElements::flags to store this bit -- we should be able to steal a bit in there, having meaning only on typed arrays, and not have to do this change. Or am I missing something? I don't think our typed array structure has changed such that this wouldn't work, and #jsapi didn't point out any holes in this idea when I mooted it. This alteration obviously would obsolete much of this patch if performed, and I think it probably should be performed, so denying review pending a new patch with this approach taken instead of burning a full slot. I didn't really look at the rest of the patch after seeing this, and I didn't read much patch before seeing it, either.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #41) > Comment on attachment 8687241 [details] [diff] [review] > bug1176214-part3-core-changes.patch > > Review of attachment 8687241 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/vm/TypedArrayObject.h > @@ +55,5 @@ > > + static_assert(IS_SHAREDMEM_SLOT == JS_TYPEDARRAYLAYOUT_IS_SHAREDMEM_SLOT, > > + "self-hosted code with burned-in constants must get the " > > + "right byteOffset slot"); > > + > > + static const size_t RESERVED_SLOTS = 7; // Three slots unused > > This 3->7 (minimum 4->8 size class) change seems unideal. I suppose it's at > least *possible* we could weather it, but I'm skeptical we actually could. > > Fortunately, I don't think we need to. We still have plenty of space in > ObjectElements::flags to store this bit -- we should be able to steal a bit > in there, having meaning only on typed arrays, and not have to do this > change. That requires an extra level of indirection for the dynamic tag check in jitted code. Absent static information that the typed array is mapping shared memory every Atomics operation will need to check this flag (see the "jit" patch), which is why it's stored in the TypedArray object itself, and not in the pointed-to buffer or other linked-to objects. From what I can tell, if I store this bit on the ObjectHeader I first have to load the elements_ pointer (which I otherwise don't need, so it won't be loaded already) and then load the flags, and then test the bit (probably not more expensive than the existing zero test), and then branch. That makes the guard a bit larger and more elaborate, and the second load will have an unpleasant dependency on the first. Still, I agree that the size increase is not good. I will implement what you suggest, but if it later turns out that type inference can't get rid of the tag check in most cases then we may have to think of something else.

Actually TypedArrays are prohibited from having "normal" indexed elements, hence have the default global emptyElementsHeader, which cannot carry that flag because it is read-only and shared by lots of objects: bool NativeObject::canHaveNonEmptyElements() { return !IsAnyTypedArray(this); } I suppose TypedArrays mapping shared memory could carry a /different/ emptyElementsHeader, except emptyElementsHeader is used as a cookie here and there and that sounds tricky and probably wrong. Will pursue this issue on IRC before I bog down further.

(In reply to Boris Zbarsky [:bz] from comment #39) > Comment on attachment 8687255 [details] [diff] [review] > bug1176214-part11-dom-except-webgl.patch > > >+++ b/dom/bindings/TypedArray.h > > >+ // About shared memory: > > So DataViews never map SharedArrayBuffers? That's right. The complexity seemed high and the benefits were hard to pin down, and nobody has argued for adding this (yet). > >+ // the view has been computed; the JS_GetTypedArraySharedness() > > That just works on typed arrays, right? Is there a way to check whether an > ArrayBuffer is shared or not? I guess JS_IsSharedArrayBuffer, right. Yes. > >+ // Callers of Obj() that do not opt into shared memory > > I'm not sure what it means for a caller of Obj() to opt into shared memory. I'll clarify the comment. It means that the caller of Obj() does not want to deal with the case where the buffer is shared memory. > >+++ b/dom/fmradio/FMRadio.cpp > >+ if (isShared) { > > How could it be shared? We just created it, and we didn't create it > shared... I was in "better safe than sorry" mode here. (Ditto for the others.) I'll tidy it up.

(In reply to Boris Zbarsky [:bz] from comment #40) > Comment on attachment 8687257 [details] [diff] [review] > bug1176214-part12-dom-webgl.patch > > Given an ArrayBufferView backed by a SharedArrayBuffer, can that > SharedArrayBuffer be detached from another thread? No. SharedArrayBuffers are not detachable from their TypedArrays. In principle we could allow this - I think we would end up reusing the existing machinery - but there's been no need to yet. > I'm assuming no: that > the other thread has a different SharedArrayBuffer with the same data > pointer, but that detaching that doesn't actually affect the thing we see, > right? Put another way, is it true that the valid data length can't > randomly change as we go along, as long as we don't call into JS? At the moment it can't change at all, ever, no matter what we do: once allocated the SharedArrayBuffer retains its allocated length (and memory locations), and a view that references the SharedArrayBuffer will always do so. > > >@@ -208,18 +208,20 @@ WebGL2Context::GetBufferSubDataT(GLenum > >+ // need to use safe operations when those become available. > > Can this comment please come with a bug number, with that bug depending on > the bug about safe operations? Yes. > And can other uses of DataAllowShared() please come with comments explaining > why they're safe? Will do.

> SharedArrayBuffers are not detachable from their TypedArrays. OK, good. Because this code in WebGL certainly assumes that can't happen in parallel. ;)

This introduces a second emptyElementsHeader called emptyElementsHeaderShared, to avoid making TypedArray instances any larger. This affects the VM only slightly, in that there needs to be code to install that header and to test against it, but the changes are localized and there are no API changes. I'll test this today but it passes testing locally so far. This change impacts the jit (part 5); updated patch coming, warrants re-review. No other patches are impacted by the adjustment.

Updated patch for fixes to test cases and perhaps some other drive-by fixes.

This is an update to the previous ion patch that avoids doubling the size of TypedArray to make slot for the IsSharedMemory flag. (The patch also has changes addressing your earlier comments.) Following Waldo's suggestion, it introduces a separate emptyObjectElements value that carries a shared-memory flag. That impacts the JIT slightly, in that whenever the JIT compares a pointer against emptyObjectElements it must compare it against both those values. This happens only in visitIteratorStart, and I've implemented it the obvious way given that that code is already quite large and branchy. I'll also be attaching a raw diff for these changes, so they'll be easier to see.

(In reply to Boris Zbarsky [:bz] from comment #39) > Comment on attachment 8687255 [details] [diff] [review] > bug1176214-part11-dom-except-webgl.patch > > I guess TCPSocket::Send is not a problem because it takes ArrayBuffer, not > ArrayBufferView? Yes. > But I think it would be good to change the necko patch to check for > sharedness in SetData(), not much later in ReadSegments(). The check is actually not needed, because the datum is an ArrayBuffer, not an ArrayBufferView, and code in SetData() checks that.

(In reply to Lars T Hansen [:lth] from comment #45) > (In reply to Boris Zbarsky [:bz] from comment #40) > > Comment on attachment 8687257 [details] [diff] [review] > > bug1176214-part12-dom-webgl.patch > > > > > And can other uses of DataAllowShared() please come with comments explaining > > why they're safe? > > Will do. They're not safe, BTW. They now carry comments explaining why they are not, and what needs to be done about it (where I have control - some paths disappear into GL) when the additional API lands.

Comment on attachment 8691305 [details] [diff] [review] Raw Ion changes pertaining to Part 5 v2 Review of attachment 8691305 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +7464,5 @@ > + masm.branchPtr(Assembler::NotEqual, > + Address(obj, NativeObject::offsetOfElements()), > + ImmPtr(js::emptyObjectElementsShared), > + ool->entry()); > + masm.bind(&emptyObj); Since we reuse this code multiple times, can we abstract this? @@ +9361,5 @@ > + // The shared-memory flag is a bit in the ObjectElements header > + // that is set if the TypedArray is mapping a SharedArrayBuffer. > + // The flag is set at construction and does not change subsequently. > + masm.loadPtr(masm.ToPayload(Address(obj, TypedArrayObject::offsetOfElements())), tmp); > + masm.load32(masm.ToPayload(Address(tmp, ObjectElements::offsetOfFlags())), tmp); I think we can remove the masm.ToPayload here? Tese are no Value's right?

Comment on attachment 8691304 [details] [diff] [review] bug1176214-part5-ion-changes-v2.patch Review of attachment 8691304 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I've given the remarks in the comment above this one.

(In reply to Lars T Hansen [:lth] from comment #55) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7030e951265f One Hazard error, which has been fixed.

Cleaned up as requested. No other changes.

Patch set rebased to today's mozilla-inbound.

Comment on attachment 8693458 [details] [diff] [review] bug1176214-part5-ion-changes-v3.patch Review of attachment 8693458 [details] [diff] [review]: ----------------------------------------------------------------- Looks good ::: js/src/jit/BaselineIC.cpp @@ +3947,5 @@ > if (!IsCacheableGetPropReadSlot(&globalLexical->global(), current, shape)) > return true; > > JitSpew(JitSpew_BaselineIC, " Generating GetName(GlobalName non-lexical) stub"); > + ICGetPropNativeCompiler compiler(cx, ICStub::GetName_Global, monitorStub, I think this is a rebase issue. But the old code is correct here. The new code should give errors on tip.

(In reply to Hannes Verschore [:h4writer] from comment #59) > Comment on attachment 8693458 [details] [diff] [review] > bug1176214-part5-ion-changes-v3.patch > > Review of attachment 8693458 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/BaselineIC.cpp > @@ +3947,5 @@ > > if (!IsCacheableGetPropReadSlot(&globalLexical->global(), current, shape)) > > return true; > > > > JitSpew(JitSpew_BaselineIC, " Generating GetName(GlobalName non-lexical) stub"); > > + ICGetPropNativeCompiler compiler(cx, ICStub::GetName_Global, monitorStub, > > I think this is a rebase issue. But the old code is correct here. The new > code should give errors on tip. Funny - the incorrect change was (incorrectly, I guess) reverted in a later patch in my queue, so the end result was correct code.

Comment on attachment 8691298 [details] [diff] [review] bug1176214-part3-core-changes-v2.patch Review of attachment 8691298 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfriendapi.h @@ +1778,5 @@ > { \ > MOZ_ASSERT(GetObjectClass(obj) == detail::Type ## ArrayClassPtr); \ > const JS::Value& lenSlot = GetReservedSlot(obj, detail::TypedArrayLengthSlot); \ > *length = mozilla::AssertedCast<uint32_t>(lenSlot.toInt32()); \ > + *isSharedMemory = JS_GetTypedArraySharedness(obj); \ With the change to make this method fallible, and relying on the class-assertion just above, this can be |obj->as<TypedArrayObject*>().<whatever method it is that gets sharedness>()|, looks like. ::: js/src/vm/ArrayBufferObject.cpp @@ +578,5 @@ > { > // Watch out for NULL data pointers in views. This means that the view > // is not fully initialized (in which case it'll be initialized later > // with the correct pointer). > + uint8_t* viewDataPointer = view->dataPointerUnshared(); While presumably this method asserts unsharedness, it might be good to do that explicitly in this method, to point out the problem slightly earlier, in a better method, for callers that screw up. @@ +591,5 @@ > MarkObjectStateChange(cx, view); > } > > +// BufferContents is specific to ArrayBuffer, hence it will not represent shared memory. > + Seems like we can make this an asertion inside changeContents, so an *effective* check in addition to pure documentation: MOZ_ASSERT(!isShared(), "BufferContents and contents-changing are only relevant to unshared buffers"); @@ +1203,5 @@ > /* static */ void > ArrayBufferViewObject::trace(JSTracer* trc, JSObject* objArg) > { > NativeObject* obj = &objArg->as<NativeObject>(); > + HeapSlot& bufSlot = obj->getReservedSlotRef(TypedArrayObject::BUFFER_SLOT); If there's a getFixedSlotRef, that'd be great to use. If not, it's not necessary to fuss about adding one, tho it might be nice to do so. @@ +1211,5 @@ > if (bufSlot.isObject()) { > + if (IsArrayBuffer(&bufSlot.toObject())) { > + ArrayBufferObject& buf = AsArrayBuffer(MaybeForwarded(&bufSlot.toObject())); > + int32_t offset = obj->getReservedSlot(TypedArrayObject::BYTEOFFSET_SLOT).toInt32(); > + MOZ_ASSERT(buf.dataPointer() != nullptr); Assert offset >= 0? @@ +1221,4 @@ > > + // Mark the object to move it into the tenured space. > + TraceManuallyBarrieredEdge(trc, &view, "typed array nursery owner"); > + MOZ_ASSERT(view->is<InlineTypedObject>() && view != obj); Make this two assertions, not one. @@ +1257,4 @@ > as<DataViewObject>().neuter(newData); > + } else if (is<TypedArrayObject>()) { > + if (as<TypedArrayObject>().isSharedMemory()) > + return; It seems unideal to have this method ever be called with shared memory -- it's really a category error. Could we fix the callers to never call this in that case, then have this method assert not-shared, as a quick followup? ::: js/src/vm/NativeObject.h @@ +248,5 @@ > + }; > + > + MOZ_CONSTEXPR ObjectElements(uint32_t capacity, uint32_t length, SharedMemory shmem) > + : flags(SHARED_MEMORY), initializedLength(0), capacity(capacity), length(length) > + {} These constructors are dubious wrt MOZ_CONSTEXPR, as I think someone just noted here (not gonna check). We might have to make the fields public in DEBUG builds so we can {}-initialize these and pick up constexpr semantics everywhere. But we can at least try this for now. @@ +294,5 @@ > static_assert(ObjectElements::VALUES_PER_HEADER * sizeof(HeapSlot) == sizeof(ObjectElements), > "ObjectElements doesn't fit in the given number of slots"); > > +/* Shared singletons for objects with no elements. > + * emptyObjectelementsShared is used only for TypedArrays, when the TA Either do // ... // ... or make this /* * ... * ... */ Also s/ctelem/ctElem/ ::: js/src/vm/TypedArrayObject.cpp @@ +344,5 @@ > return nullptr; > > + bool isSharedMemory = buffer && IsSharedArrayBuffer(buffer.get()); > + > + obj->setSlot(TypedArrayObject::BUFFER_SLOT, ObjectOrNullValue(buffer)); Make this setFixedSlot while you're changing things. @@ +356,5 @@ > > // If the buffer is for an inline typed object, the data pointer > // may be in the nursery, so include a barrier to make sure this > // object is updated if that typed object moves. > + if (!IsInsideNursery(obj) && cx->runtime()->gc.nursery.isInside(buffer->dataPointerEither())) Please add a MOZ_ASSERT(!isSharedMemory) within this |if|, because as typed arrays always have a buffer, this can't be true and we might as well assert it. @@ +365,5 @@ > memset(data, 0, len * sizeof(NativeType)); > } > > + obj->setSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(len)); > + obj->setSlot(TypedArrayObject::BYTEOFFSET_SLOT, Int32Value(byteOffset)); setFixedSlot again. @@ +375,5 @@ > uint32_t bufferByteLength = buffer->byteLength(); > + // Unwraps are safe: both are for the pointer value. > + if (IsArrayBuffer(buffer.get())) > + MOZ_ASSERT_IF(!AsArrayBuffer(buffer.get()).isNeutered(), > + buffer->dataPointerEither().unwrap(/*safe*/) <= obj->viewDataEither().unwrap(/*safe*/)); Brace this, as the body now covers multiple lines. @@ +448,5 @@ > * properties from the object, treating it as some sort of array. > * Note that offset and length will be ignored. Note that a > * shared array's values are copied here. > */ > + if (!UncheckedUnwrap(dataObj)->is<ArrayBufferObject>() && !UncheckedUnwrap(dataObj)->is<SharedArrayBufferObject>()) It might be worth making is<ArrayBufferObjectMaybeShared>() work correctly -- or at least make it an error so that people won't do it. @@ +556,5 @@ > } > > + Rooted<ArrayBufferObjectMaybeShared*> buffer(cx, IsArrayBuffer(bufobj) > + ? (ArrayBufferObjectMaybeShared*)&AsArrayBuffer(bufobj) > + : (ArrayBufferObjectMaybeShared*)&AsSharedArrayBuffer(bufobj)); static_cast<>s? Might be worth splitting the ternary into a separate assignment, too, for shorter lines. @@ +632,5 @@ > getIndex(JSObject* obj, uint32_t index) > { > TypedArrayObject& tarray = obj->as<TypedArrayObject>(); > MOZ_ASSERT(index < tarray.length()); > + return jit::AtomicOperations::loadSafeWhenRacy(tarray.viewDataEither().cast<NativeType*>()+index); Spaces around +. @@ +639,5 @@ > static void > setIndex(TypedArrayObject& tarray, uint32_t index, NativeType val) > { > MOZ_ASSERT(index < tarray.length()); > + jit::AtomicOperations::storeSafeWhenRacy(tarray.viewDataEither().cast<NativeType*>()+index, val); Ditto. @@ +2015,5 @@ > void > DataViewObject::neuter(void* newData) > { > + setSlot(TypedArrayObject::LENGTH_SLOT, Int32Value(0)); > + setSlot(TypedArrayObject::BYTEOFFSET_SLOT, Int32Value(0)); setFixedSlots ::: js/src/vm/TypedArrayObject.h @@ +68,5 @@ > > template<typename T> struct OfType; > > static bool sameBuffer(Handle<TypedArrayObject*> a, Handle<TypedArrayObject*> b) { > + return a->bufferObject() == b->bufferObject(); Bah. I just realized this code's wrong right now -- for two typed arrays using inline storage, it'll say they have the same buffer. Please file a followup to fix this? Looks like this only matters for optimization, tho, so technically it's not a safety concern.

Comment on attachment 8687242 [details] [diff] [review] bug1176214-part4-builtin-lib-changes.patch Review of attachment 8687242 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/AtomicsObject.cpp @@ +85,5 @@ > } > > static bool > +GetTypedArray(JSContext* cx, HandleValue v, > + MutableHandle<TypedArrayObject*> viewp) Why the name change? This seems to have the same sense as before (at least as long as the throw-if-not-shared behavior is here), and the old name seemed like the right one. ::: js/src/vm/SelfHosting.cpp @@ +684,5 @@ > return true; > } > > static bool > +intrinsic_IsSharedArrayBuffer(JSContext* cx, unsigned argc, Value* vp) This might be unnecessary (and require modifications to existing function-uses) with till's patching recently to have a testing function that takes a JSProtoKey. @@ +794,5 @@ > MOZ_ASSERT(count > 0, > "don't call this method if copying no elements, because then " > "the not-neutered requirement is wrong"); > > + if (tarray->hasBuffer() && tarray->isNeutered()) { I'd prefer the other way, ideally, because "neutered" -- "detached" in ES6 but we haven't fixed our terminology yet -- is a property of ArrayBuffers, not of views. @@ +957,3 @@ > unsafeSrcDataCrossCompartment + unsafeSrcByteLengthCrossCompartment; > + SharedMem<uint8_t*> targetDataLimit = > + target->viewDataEither().cast<uint8_t*>() + targetLength * targetElementSize; Unrelated to anything whatsoever in this patch (at least at this juncture when I'm writing this comment). We're going to have to be careful about overlapping cases where we make a copy of the input data, before writing to output. The fact that we've done this is *not* enough to let us use non-racy copying/moving operations, because a memcpy could have a read on another thread see a torn value. (I think we might do this now.) CONSTANT VIGILANCE!

Comment on attachment 8691299 [details] [diff] [review] bug1176214-part7-misc-engine-changes-v2.patch Review of attachment 8691299 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ +2154,5 @@ > + return false; > + } > + > +#ifdef JS_MORE_DETERMINISTIC > + args.rval().setInt32(0); Seems like returning cx->staticStrings().getUint(0) would be more consistent type-wise. @@ +2167,5 @@ > + return false; > + } > + char buffer[64]; > + JS_snprintf(buffer, sizeof(buffer), "%p", > + obj->as<SharedArrayBufferObject>().dataPointerShared().unwrap(/*safe enough...*/)); I am reminded of GetJSPrivateSafeish. :-) @@ +2169,5 @@ > + char buffer[64]; > + JS_snprintf(buffer, sizeof(buffer), "%p", > + obj->as<SharedArrayBufferObject>().dataPointerShared().unwrap(/*safe enough...*/)); > + > + JSString* str = JS_NewStringCopyZ(cx, buffer); Minor preference for capturing the number returned by JS_snprintf and using JS_NewStringCopyN with that. ::: js/src/ctypes/CTypes.cpp @@ +3240,5 @@ > // copy the array. > + const bool bufferShared = cls == ESClass_SharedArrayBuffer; > + uint32_t sourceLength = (bufferShared ? > + JS_GetSharedArrayBufferByteLength(valObj) : > + JS_GetArrayBufferByteLength(valObj)); Parenthesizing this isn't consistent with normal style. @@ +3280,5 @@ > JS::AutoCheckCannotGC nogc; > + bool isShared; > + SharedMem<void*> src = > + SharedMem<void*>::shared(JS_GetArrayBufferViewData(valObj, &isShared, nogc)); > + jit::AtomicOperations::memcpySafeWhenRacy(target, src, sourceLength); Seems to me, consistent with my random comment on the last patch, that memcpy that doesn't know the size of the elements being copied is going to have tearing issues. I have a feeling "memcpy" and "memmove" that take only an amount of memory, as opposed to a count of known-size elements, are not going to be the right primitive here. ::: js/src/shell/OSObject.cpp @@ +175,5 @@ > + // buffer.) > + JS_ReportError(cx, "can't read %s: shared memory buffer", pathname); > + return nullptr; > + } > + char* buf = (char*) ta.viewDataUnshared(); static_cast<> ::: js/src/vm/StructuredClone.cpp @@ -246,5 @@ > bool checkDouble(double d); > bool readTypedArray(uint32_t arrayType, uint32_t nelems, MutableHandleValue vp, > bool v1Read = false); > bool readDataView(uint32_t byteLength, MutableHandleValue vp); > - bool readSharedTypedArray(uint32_t arrayType, uint32_t nelems, MutableHandleValue vp); So this is all removals -- some other patch transmits the sharedness bit through cloning, correct? Seems like that would be missing here. ::: toolkit/components/ctypes/tests/unit/test_jsctypes.js @@ +1772,5 @@ > [new Float32Array(c_arraybuffer), ctypes.float32_t], > [new Float64Array(c_arraybuffer), ctypes.float64_t] > ]; > > + if (typeof SharedArrayBuffer != "undefined") { !== mildly better. @@ +1775,5 @@ > > + if (typeof SharedArrayBuffer != "undefined") { > + let c_shared_arraybuffer = new SharedArrayBuffer(256); > + typed_array_samples.push([new Int8Array(c_shared_arraybuffer), ctypes.int8_t], > + [new Uint8Array(c_shared_arraybuffer), ctypes.uint8_t], Uint8ClampedArray won't also work? Fine if not, just double-checking the omission is deliberate. Maybe worth a comment to indicate a considered omission. @@ +1794,5 @@ > let array_type = item_type.array(number_of_items); > > + // Int8Array on unshared memory is interconvertible with Int8Array on > + // shared memory, etc. > + if (i%8 != j%8) { Spaces around the percents.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #61) > Comment on attachment 8691298 [details] [diff] [review] > bug1176214-part3-core-changes-v2.patch > > Review of attachment 8691298 [details] [diff] [review]: > ----------------------------------------------------------------- Select followups. > ::: js/src/jsfriendapi.h > @@ +1778,5 @@ > > { \ > > MOZ_ASSERT(GetObjectClass(obj) == detail::Type ## ArrayClassPtr); \ > > const JS::Value& lenSlot = GetReservedSlot(obj, detail::TypedArrayLengthSlot); \ > > *length = mozilla::AssertedCast<uint32_t>(lenSlot.toInt32()); \ > > + *isSharedMemory = JS_GetTypedArraySharedness(obj); \ > > With the change to make this method fallible, and relying on the > class-assertion just above, this can be > |obj->as<TypedArrayObject*>().<whatever method it is that gets > sharedness>()|, looks like. Ideally yes, but this being jsfriendapi.h the necessary types are not available. (Also the preceding comment suggesting that the function is fallible was probably a merge error; the function is infallible and clients assume that it is.) > @@ +1211,5 @@ > > if (bufSlot.isObject()) { > > + if (IsArrayBuffer(&bufSlot.toObject())) { > > + ArrayBufferObject& buf = AsArrayBuffer(MaybeForwarded(&bufSlot.toObject())); > > + int32_t offset = obj->getReservedSlot(TypedArrayObject::BYTEOFFSET_SLOT).toInt32(); > > + MOZ_ASSERT(buf.dataPointer() != nullptr); > > Assert offset >= 0? Looking at code elsewhere, the correct type here is uint32_t, so I went that way instead. > ::: js/src/vm/NativeObject.h > @@ +248,5 @@ > > + }; > > + > > + MOZ_CONSTEXPR ObjectElements(uint32_t capacity, uint32_t length, SharedMemory shmem) > > + : flags(SHARED_MEMORY), initializedLength(0), capacity(capacity), length(length) > > + {} > > These constructors are dubious wrt MOZ_CONSTEXPR, as I think someone just > noted here (not gonna check). We might have to make the fields public in > DEBUG builds so we can {}-initialize these and pick up constexpr semantics > everywhere. But we can at least try this for now. I'm mystified by this comment; the code passes a full try run. Any more information you can dig up will be helpful.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #63) > Comment on attachment 8691299 [details] [diff] [review] > bug1176214-part7-misc-engine-changes-v2.patch > > Review of attachment 8691299 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +3280,5 @@ > > JS::AutoCheckCannotGC nogc; > > + bool isShared; > > + SharedMem<void*> src = > > + SharedMem<void*>::shared(JS_GetArrayBufferViewData(valObj, &isShared, nogc)); > > + jit::AtomicOperations::memcpySafeWhenRacy(target, src, sourceLength); > > Seems to me, consistent with my random comment on the last patch, that > memcpy that doesn't know the size of the elements being copied is going to > have tearing issues. I have a feeling "memcpy" and "memmove" that take only > an amount of memory, as opposed to a count of known-size elements, are not > going to be the right primitive here. There's a PodCopy variant coming (bug 1211432) but that's mostly to avoid multiplication and other size computation footguns; non-tearing copying of notably float64 data is simply not possible cross-platform without global locking - some plausible hardware is single-copy atomic only up to four bytes. That said, I think the shared memory spec needs to address this somehow, and I will investigate + file a followup bug if necessary. > ::: js/src/vm/StructuredClone.cpp > @@ -246,5 @@ > > bool checkDouble(double d); > > bool readTypedArray(uint32_t arrayType, uint32_t nelems, MutableHandleValue vp, > > bool v1Read = false); > > bool readDataView(uint32_t byteLength, MutableHandleValue vp); > > - bool readSharedTypedArray(uint32_t arrayType, uint32_t nelems, MutableHandleValue vp); > > So this is all removals -- some other patch transmits the sharedness bit > through cloning, correct? Seems like that would be missing here. Yes, that's already taken care of by the code that transmits the underlying buffer. "It just works", so far as I can tell. > @@ +1775,5 @@ > > > > + if (typeof SharedArrayBuffer != "undefined") { > > + let c_shared_arraybuffer = new SharedArrayBuffer(256); > > + typed_array_samples.push([new Int8Array(c_shared_arraybuffer), ctypes.int8_t], > > + [new Uint8Array(c_shared_arraybuffer), ctypes.uint8_t], > > Uint8ClampedArray won't also work? Fine if not, just double-checking the > omission is deliberate. Maybe worth a comment to indicate a considered > omission. Actually an oversight, because the test case I was modifying also did not test that type. May prefer to file a followup bug for this, I think I have other test cases to modify as well. Uint8ClampedArray is allowed to map shared memory /but/ atomic operations are not provided on Uint8ClampedArray. This will probably complicate the code further throughout the system since a simple "isShared" bit on TypedArray is not what we're looking for, we're looking for that plus an "allowsAtomics" bit, and they are not the same. Sigh.

(In reply to Lars T Hansen [:lth] from comment #66) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=a379b97faf78 Some jit-test red in that run, caused by failing to check the return value of a fallible accessor. (The oranges are all benign.) With a fix the tests pass locally.

https://hg.mozilla.org/integration/mozilla-inbound/rev/49a370926c908b8b24634981c0d8ec8b99e58b96 Bug 1176214 - Part 1: Remove obsolete files. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/f10d0e915f9605eb1dedd989c84cd07d40f4dc3d Bug 1176214 - Part 2: Preliminary adjustments. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/20570b526b355c4739a92072cb181ed8faf5bbaf Bug 1176214 - Part 3: VM core changes. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/97827dc3e6c4c374b03900feecdffa2dc9698609 Bug 1176214 - Part 4: VM built-in lib changes. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/36df961c4bdc55165397639f999cce8075af3326 Bug 1176214 - Part 5: Ion changes. r=h4writer https://hg.mozilla.org/integration/mozilla-inbound/rev/3b97e81dcbd768972a85bd4f8bb96d6f12ee3828 Bug 1176214 - Part 6: Odin changes. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4992c56a458d0dd28f68d72fb6a80a7750af2d Bug 1176214 - Part 7: Ctypes, shell, xpconnect, etc. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c872329eca28cd1cff11a2c06dbb9847b1434b Bug 1176214 - Part 8: jit-test changes. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf17cfb5fd50ab40c9c2262a15c422235f80897 Bug 1176214 - Part 9: tests changes. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/30bb21bc1381958c999902be418feba041c59c6b Bug 1176214 - Part 10: jsapi-tests changes. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/91fe35e7df1fa3124c266c249ad0907320dfd956 Bug 1176214 - Part 11: Changes to DOM, except for WebGL. r=bz, r=clb https://hg.mozilla.org/integration/mozilla-inbound/rev/4c204b62828c7a8b2629259443b7e15c8191dd1f Bug 1176214 - Part 12: Changes to WebGL. r=bz, r=clb https://hg.mozilla.org/integration/mozilla-inbound/rev/a20e7308c5ebec9fe7012ff0af018d47e695a076 Bug 1176214 - Part 13: Changes to ipc. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/0688879487f63abf197ca5e78c88425babf64187 Bug 1176214 - Part 14: Changes to netwerk. r=jduell https://hg.mozilla.org/integration/mozilla-inbound/rev/239195d791ceab09f2a28b76e94c6e438665ab78 Bug 1176214 - Part 15: Changes to xpcom. r=nfroyd

(In reply to Lars T Hansen [:lth] from comment #65) > (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #63) > > Comment on attachment 8691299 [details] [diff] [review] > > bug1176214-part7-misc-engine-changes-v2.patch > > > > Review of attachment 8691299 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > @@ +3280,5 @@ > > > JS::AutoCheckCannotGC nogc; > > > + bool isShared; > > > + SharedMem<void*> src = > > > + SharedMem<void*>::shared(JS_GetArrayBufferViewData(valObj, &isShared, nogc)); > > > + jit::AtomicOperations::memcpySafeWhenRacy(target, src, sourceLength); > > > > Seems to me, consistent with my random comment on the last patch, that > > memcpy that doesn't know the size of the elements being copied is going to > > have tearing issues. I have a feeling "memcpy" and "memmove" that take only > > an amount of memory, as opposed to a count of known-size elements, are not > > going to be the right primitive here. > > There's a PodCopy variant coming (bug 1211432) but that's mostly to avoid > multiplication and other size computation footguns; non-tearing copying of > notably float64 data is simply not possible cross-platform without global > locking - some plausible hardware is single-copy atomic only up to four > bytes. > > That said, I think the shared memory spec needs to address this somehow, and > I will investigate + file a followup bug if necessary. I think we're good here. The shared memory spec guarantees atomicity (non-tearing) for conflicting (ie simultaneous) atomic accesses of the same size to the same byte range in memory. Everything else creates a data race - even conflicting atomic accesses of different sizes or to overlapping but unidentical byte ranges - and data races cause garbage to be stored in memory. (In both cases assuming at least one access is a write.)

https://hg.mozilla.org/mozilla-central/rev/49a370926c90 https://hg.mozilla.org/mozilla-central/rev/f10d0e915f96 https://hg.mozilla.org/mozilla-central/rev/20570b526b35 https://hg.mozilla.org/mozilla-central/rev/97827dc3e6c4 https://hg.mozilla.org/mozilla-central/rev/36df961c4bdc https://hg.mozilla.org/mozilla-central/rev/3b97e81dcbd7 https://hg.mozilla.org/mozilla-central/rev/2a4992c56a45 https://hg.mozilla.org/mozilla-central/rev/e4c872329eca https://hg.mozilla.org/mozilla-central/rev/fcf17cfb5fd5 https://hg.mozilla.org/mozilla-central/rev/30bb21bc1381 https://hg.mozilla.org/mozilla-central/rev/91fe35e7df1f https://hg.mozilla.org/mozilla-central/rev/4c204b62828c https://hg.mozilla.org/mozilla-central/rev/a20e7308c5eb https://hg.mozilla.org/mozilla-central/rev/0688879487f6 https://hg.mozilla.org/mozilla-central/rev/239195d791ce

(In reply to Lars T Hansen [:lth] from comment #64) > > Assert offset >= 0? > > Looking at code elsewhere, the correct type here is uint32_t, so I went that > way instead. Fair enough, but we should assert <= INT32_MAX, then, to detect the negative-number case. (Typed array offsets/lengths/etc. all are int32_t-limited, precisely because of their being shoved into Values in reserved slots.) > > > + MOZ_CONSTEXPR ObjectElements(uint32_t capacity, uint32_t length, SharedMemory shmem) > > > + : flags(SHARED_MEMORY), initializedLength(0), capacity(capacity), length(length) > > > + {} > > > > These constructors are dubious wrt MOZ_CONSTEXPR, as I think someone just > > noted here (not gonna check). We might have to make the fields public in > > DEBUG builds so we can {}-initialize these and pick up constexpr semantics > > everywhere. But we can at least try this for now. > > I'm mystified by this comment; the code passes a full try run. Any more > information you can dig up will be helpful. The "problem" isn't one that shows up as a compile error. The issue is that objects allocated statically, that require a constructor be called, often trigger "static initializers" with some compilers. The effect is that at startup, you have a DLL mapped into memory, but some portions of it must be mapped into read-write memory -- either copy-on-write or by copying the entire page from the DLL. That itself is cheap, but then when the object's constructor is called at startup, you get a near-immediate, guaranteed page fault. On the other hand, if you allocate the object so that it can be brace-initialized, or so that its constructor is constexpr, the compiler is usually smart enough to make the object be fully initialized *without* requiring the constructor be called at startup. If MOZ_CONSTEXPR really is constexpr, then the two static ObjectElements are known by the compiler such that no constructor call has to happen (and no call typically *does* happen). But if it's not really constexpr, *some* compilers will include that constructor call at startup, slowing down overall initialization. Before this change, we *did* have TypedArrayLayout objects allocated statically. And their constructors weren't definitely constexpr, so we could have had static initializer constuctor calls slowing things down. So your patch doesn't *regress* things. I'm simply observing that this reliance on MOZ_CONSTEXPR really isn't necessary, if we made ObjectElements capable of being {}-initialized at effectively no cost in safety, in DEBUG builds. Anyway. Wall of text, we're discussing semantics technically beyond the C++ spec that are implementation choices, I can explain in person if it helps. :-) > non-tearing copying of notably float64 data is simply not possible > cross-platform without global locking - some plausible hardware is > single-copy atomic only up to four bytes. > > That said, I think the shared memory spec needs to address this somehow, and > I will investigate + file a followup bug if necessary. Eugh. For float64 it has to be impossible, but it seems like the vision of "you get old or new" for everything else *requires* this care here. And if you decided to conveniently throw out that vision, you're basically at "unspecified values" and most of the way back to C++ UB. Bah, why is 32-bit still thing any more. :-( > Uint8ClampedArray is allowed to map shared memory /but/ atomic operations > are not provided on Uint8ClampedArray. This will probably complicate the > code further throughout the system since a simple "isShared" bit on > TypedArray is not what we're looking for, we're looking for that plus an > "allowsAtomics" bit, and they are not the same. Sigh. Ugh. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=a379b97faf78 > > Some jit-test red in that run, caused by failing to check the return value > of a fallible accessor. (The oranges are all benign.) With a fix the tests > pass locally. I'm guessing the fix for this is what resulted in bug 1230337, for what it's worth. My bad missing that. :-( > I think we're good here. The shared memory spec guarantees atomicity > (non-tearing) for conflicting (ie simultaneous) atomic accesses of the same > size to the same byte range in memory. Everything else creates a data race > - even conflicting atomic accesses of different sizes or to overlapping but > unidentical byte ranges - and data races cause garbage to be stored in > memory. (In both cases assuming at least one access is a write.) If simultaneous accesses of the same size have to be atomic, doesn't that mean "memcpy/memmove" that's anything *but* by type isn't okay? Right now bug 1211432 doesn't seem to change this fundamental approach, tho. It really seems like the fundamental operations have to be podCopy1, podCopy2, podCopy4, podCopy8 and podMove* variants, and any operation that's not acting on elements of specific size is not going to prevent tearing.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #71) > Before this change, we *did* have TypedArrayLayout objects allocated > statically. And their constructors weren't definitely constexpr, so we > could have had static initializer constuctor calls slowing things down. So > your patch doesn't *regress* things. I'm simply observing that this > reliance on MOZ_CONSTEXPR really isn't necessary, if we made ObjectElements > capable of being {}-initialized at effectively no cost in safety, in DEBUG > builds. Brace initialization doesn't eliminate static initializers on some compilers...or at least it didn't when I was doing work to shrink the JitInfo structures. I can't recall whether we've upgraded those compilers and/or whether the upgrades have gained appropriate optimization capabilities in their upgrades.

(In reply to Nathan Froyd [:froydnj] from comment #72) > Brace initialization doesn't eliminate static initializers on some > compilers... "the compiler is usually smart enough", as I said. :-) My sense was the cases were where the initialization required a pointer value, that might be something affected by linkage, ergo requiring runtime initialization. That isn't an issue here for the pure integral/enums-backed-by-integral data here.

(In reply to Lars T Hansen [:lth] from comment #47) > Created attachment 8691298 [details] [diff] [review] > bug1176214-part3-core-changes-v2.patch > > This introduces a second emptyElementsHeader called > emptyElementsHeaderShared, to avoid making TypedArray instances any larger. > This affects the VM only slightly, in that there needs to be code to install > that header and to test against it, but the changes are localized and there > are no API changes. I'll test this today but it passes testing locally so > far. > > This change impacts the jit (part 5); updated patch coming, warrants > re-review. No other patches are impacted by the adjustment. Would it help any to allocate the two emptyElementsHeaders adjacent in memory so you can do a range-check?

(In reply to Steve Fink [:sfink, :s:] from comment #74) > (In reply to Lars T Hansen [:lth] from comment #47) > > Created attachment 8691298 [details] [diff] [review] > > bug1176214-part3-core-changes-v2.patch > > > > This introduces a second emptyElementsHeader called > > emptyElementsHeaderShared, to avoid making TypedArray instances any larger. > > This affects the VM only slightly, in that there needs to be code to install > > that header and to test against it, but the changes are localized and there > > are no API changes. I'll test this today but it passes testing locally so > > far. > > > > This change impacts the jit (part 5); updated patch coming, warrants > > re-review. No other patches are impacted by the adjustment. > > Would it help any to allocate the two emptyElementsHeaders adjacent in > memory so you can do a range-check? Probably not. As it is, everywhere I ran into it it became a sequence of: cmp hdr1 je across cmp hdr2 jne out-of-line across: and I think I'd need two compares and branches for the range check anyhow. (If there were three such headers I might feel differently about it - but there weren't too many cases and they're not that hot, so even for three it could be a wash.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #71) > (In reply to Lars T Hansen [:lth] from comment #64) > > > Assert offset >= 0? > > > > Looking at code elsewhere, the correct type here is uint32_t, so I went that > > way instead. > > Fair enough, but we should assert <= INT32_MAX, then, to detect the > negative-number case. (Typed array offsets/lengths/etc. all are > int32_t-limited, precisely because of their being shoved into Values in > reserved slots.) OK, I can fix that. BTW, bug 1205616 is probably relevant here. > > > > + MOZ_CONSTEXPR ObjectElements(uint32_t capacity, uint32_t length, SharedMemory shmem) > > > > + : flags(SHARED_MEMORY), initializedLength(0), capacity(capacity), length(length) > > > > + {} > > The issue is that objects allocated statically, that require a constructor > be called, often trigger "static initializers" with some compilers. I plead innocence since there was already a constructor for this object and this adds another. And I did get a compile error on my build platform (Linux) when I mistakenly made the constructor body non-empty. (I also think that if this is a real concern then we need some style guidelines to avoid the problem generally, with teeth.) > > (memcpy and tearing) We discussed this in person at some length. Also see below. > > Uint8ClampedArray is allowed to map shared memory /but/ atomic operations > > are not provided on Uint8ClampedArray. This will probably complicate the > > code further throughout the system since a simple "isShared" bit on > > TypedArray is not what we're looking for, we're looking for that plus an > > "allowsAtomics" bit, and they are not the same. Sigh. > > Ugh. This turned out to be a red herring, fortunately. > > I think we're good here. The shared memory spec guarantees atomicity > > (non-tearing) for conflicting (ie simultaneous) atomic accesses of the same > > size to the same byte range in memory. Everything else creates a data race > > - even conflicting atomic accesses of different sizes or to overlapping but > > unidentical byte ranges - and data races cause garbage to be stored in > > memory. (In both cases assuming at least one access is a write.) > > If simultaneous accesses of the same size have to be atomic, doesn't that > mean "memcpy/memmove" that's anything *but* by type isn't okay? Right now > bug 1211432 doesn't seem to change this fundamental approach, tho. It > really seems like the fundamental operations have to be podCopy1, podCopy2, > podCopy4, podCopy8 and podMove* variants, and any operation that's not > acting on elements of specific size is not going to prevent tearing. We also discussed this in person. Basically the user program has to synchronize, or any kind of read/write conflict to shared memory is a race. Whether we move one byte of four bytes has no real bearing on this, and the spec currently has no guarantees about single-copy atomicity.

(In reply to Lars T Hansen [:lth] from comment #75) > (In reply to Steve Fink [:sfink, :s:] from comment #74) > > Would it help any to allocate the two emptyElementsHeaders adjacent in > > memory so you can do a range-check? > > Probably not. As it is, everywhere I ran into it it became a sequence of: > > cmp hdr1 > je across > cmp hdr2 > jne out-of-line > across: > > and I think I'd need two compares and branches for the range check anyhow. > > (If there were three such headers I might feel differently about it - but > there weren't too many cases and they're not that hot, so even for three it > could be a wash.) That's sort of what I expected too, but I was thinking of something more like if (uint32_t(ptr) - uint32_t(&firstHeader) < sizeof(two headers)) ...figure out which header...

(In reply to Steve Fink [:sfink, :s:] from comment #77) > (In reply to Lars T Hansen [:lth] from comment #75) > > (In reply to Steve Fink [:sfink, :s:] from comment #74) > > That's sort of what I expected too, but I was thinking of something more like > > if (uint32_t(ptr) - uint32_t(&firstHeader) < sizeof(two headers)) > ...figure out which header... Sweet. The two headers would be forced into some dynamically allocated objects to enforce co-location or I'd have to use array-of-length-2 and brace initialization, possibly.

(In reply to Lars T Hansen [:lth] from comment #78) > (In reply to Steve Fink [:sfink, :s:] from comment #77) > > (In reply to Lars T Hansen [:lth] from comment #75) > > > (In reply to Steve Fink [:sfink, :s:] from comment #74) > > > > That's sort of what I expected too, but I was thinking of something more like > > > > if (uint32_t(ptr) - uint32_t(&firstHeader) < sizeof(two headers)) > > ...figure out which header... > > Sweet. The two headers would be forced into some dynamically allocated > objects to enforce co-location or I'd have to use array-of-length-2 and > brace initialization, possibly. Yes, I was thinking array of length 2. Header emptyHeaders[2] { {0, 0}, {0, 0} }; Header* emptyHeader = &emptyHeaders[0]; Header* emptyHeaderShared = &emptyHeaders[1]; if that's possible with some syntax. I don't know if those references are constexpr. I suppose you could do #define emptyHeader (&emptyHeaders[0]) if nothing else.

EmptyHeader optimization filed as bug 1231922.

https://hg.mozilla.org/mozilla-central/rev/8c30c3eedfe7 https://hg.mozilla.org/mozilla-central/rev/cdae79139898