Opaque typed objects never have their buffer accessed, so avoiding creating ArrayBuffer objects explicitly for such objects saves a lot of memory (and will, with forthcoming patches for bug 1058340, improve performance when accessing the objects). The attached patch allocates opaque typed objects with their data inline --- immediately following the object's header --- when that data fits in one of the JSObject size classes.

Comment on attachment 8491914 [details] [diff] [review] patch Review of attachment 8491914 [details] [diff] [review]: ----------------------------------------------------------------- Niko, are you ok with reviewing the JIT bits? I'm not (they look ok to me, but my opinion isn't worth much.) If you aren't, we probably ought to additionally r? jandem or somebody. ::: js/src/builtin/TypedObject.cpp @@ +2824,5 @@ > > + JS_ASSERT(typedObj.isAttached()); > + int32_t oldOffset = typedObj.offset(); > + > + typedObj.setPrivate(typedObj.typedMem() + (offset - oldOffset)); How is this used? If this is called twice, the first time with a nonzero offset, this will do the wrong thing. This is invoked via the self-hosted RedirectPointer, which is itself used by some kind of parallel craziness, so I'll let Niko look at this. But it looks wrong to me. ::: js/src/builtin/TypedObject.h @@ +73,5 @@ > + * object (opaque objects only). Outline typed objects may be attached or > + * unattached. An unattached typed object has no memory associated with it. > + * When first created, objects are always attached, but they can become > + * unattached if their buffer is neutered (note that this implies that typed > + * objects of opaque types can't be unattached). ES6 has renamed "neutered" to "detached", though that makes the prose here a little weird. Do you intend to ever support inline transparent typed objects? Seems like they would be fairly common. ::: js/src/jit-test/tests/TypedObject/inlineopaque.js @@ +1,2 @@ > +var TO = TypedObject; > + I think you need something like if (!this.hasOwnProperty("TypedObject")) quit(); to keep this from breaking on uplift.

(In reply to Steve Fink [:sfink] from comment #1) > ::: js/src/builtin/TypedObject.cpp > @@ +2824,5 @@ > > > > + JS_ASSERT(typedObj.isAttached()); > > + int32_t oldOffset = typedObj.offset(); > > + > > + typedObj.setPrivate(typedObj.typedMem() + (offset - oldOffset)); > > How is this used? If this is called twice, the first time with a nonzero > offset, this will do the wrong thing. This is invoked via the self-hosted > RedirectPointer, which is itself used by some kind of parallel craziness, so > I'll let Niko look at this. But it looks wrong to me. It should be able to be called any number of times with any (valid) offsets. This code looks right to me because typedObj.typedMem() is equal to its owner's base data pointer + the old offset. So we subtract off the old offset to get the owner's base data pointer, then add the new offset. I can add a comment if that will help with clarity. FWIW this code will be changing again soon, as I need to kill |offset| as a slot/member of outline typed objects, so they only have their owner and the internal data pointer, and will need to go through more rigmarole to change the offset. > Do you intend to ever support inline transparent typed objects? Seems like > they would be fairly common. We probably will need to, though doing so will be a lot more complicated than inlining opaque typed objects, since the array buffer for a transparent typed object can be accessed and, uh, detached. Hopefully the spec will guide people away from transparent typed objects by making opaque the default, but there might be cases where transparent typed objects do need good performance. For example, I think right now SIMD objects are transparent, though I don't know if that will change in the future.

(In reply to Steve Fink [:sfink] from comment #1) > Comment on attachment 8491914 [details] [diff] [review] > patch > > Review of attachment 8491914 [details] [diff] [review]: > ----------------------------------------------------------------- > > Niko, are you ok with reviewing the JIT bits? I'm not (they look ok to me, > but my opinion isn't worth much.) If you aren't, we probably ought to > additionally r? jandem or somebody. I think so, I am planning to do the review today. I'll tag out to jandem if I feel out of my depth.

(In reply to Brian Hackett (:bhackett) from comment #2) > (In reply to Steve Fink [:sfink] from comment #1) > > ::: js/src/builtin/TypedObject.cpp > > @@ +2824,5 @@ > > > > > > + JS_ASSERT(typedObj.isAttached()); > > > + int32_t oldOffset = typedObj.offset(); > > > + > > > + typedObj.setPrivate(typedObj.typedMem() + (offset - oldOffset)); > > > > How is this used? If this is called twice, the first time with a nonzero > > offset, this will do the wrong thing. This is invoked via the self-hosted > > RedirectPointer, which is itself used by some kind of parallel craziness, so > > I'll let Niko look at this. But it looks wrong to me. > > It should be able to be called any number of times with any (valid) offsets. > This code looks right to me because typedObj.typedMem() is equal to its > owner's base data pointer + the old offset. So we subtract off the old > offset to get the owner's base data pointer, then add the new offset. I can > add a comment if that will help with clarity. Ah! You're right, of course. Somehow, the name typedMem() led me to believe it was at offset zero. I don't think it needs a comment, but I would probably understand more easily if it were (typedObj.typedMem() - oldOffset) + offset > > Do you intend to ever support inline transparent typed objects? Seems like > > they would be fairly common. > > We probably will need to, though doing so will be a lot more complicated > than inlining opaque typed objects, since the array buffer for a transparent > typed object can be accessed and, uh, detached. Hopefully the spec will > guide people away from transparent typed objects by making opaque the > default, but there might be cases where transparent typed objects do need > good performance. For example, I think right now SIMD objects are > transparent, though I don't know if that will change in the future. You need transparent typed objects for casting, right? Although I guess that's sort of a reinterpret_cast, which should be needed much.

(In reply to Steve Fink [:sfink] from comment #4) > You need transparent typed objects for casting, right? Although I guess > that's sort of a reinterpret_cast, which should be needed much. which *shouldn't* be needed much.

Comment on attachment 8491914 [details] [diff] [review] patch Review of attachment 8491914 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. I saw one problem (the ParallelFucntions.cpp check) but the rest makes total sense. ::: js/src/jit/CodeGenerator.cpp @@ +4377,5 @@ > Register temp = ToRegister(lir->temp()); > > + Label notOutline; > + masm.loadObjClass(obj, temp); > + masm.branchPtr(Assembler::Equal, temp, ImmPtr(&InlineOpaqueTypedObject::class_), &notOutline); Conceivably, we could use TI information to avoid this branch in many cases, right? (Not suggesting we need to do it now.) ::: js/src/jit/ParallelFunctions.cpp @@ +104,5 @@ > // Also check whether owner is thread-local. > + if (typedObj.is<OutlineTypedObject>() && !cx->isThreadLocal(&typedObj.as<OutlineTypedObject>().owner())) > + return false; > + > + return true; This doesn't seem quite right. We're supposed to be testing whether the object is thread-local, and all inline opaque typed objects are not necessarily thread-local. I think this `return true` should be `return cx->isThreadLocal(typedObj)`

Comment on attachment 8491914 [details] [diff] [review] patch Review of attachment 8491914 [details] [diff] [review]: ----------------------------------------------------------------- Niko reverted my r+. :)

Followup to use the OutlineTypedObject naming in the patch, I accidentally pushed a different version that used a more confusing OwnedTypedObject name. https://hg.mozilla.org/integration/mozilla-inbound/rev/9285d6b729bd