Bug 898349 optimized property access to typed objects but not array element access. Analogous code paths can be added for the array element case.

(still need to clean up patch A little bit; I'm just getting this up here because its the quickest way for me to share between my hg moz-inbound checkouts.)

I changed my mind: we can go ahead and start the review process now. (The patch wasn't quite as ugly as I had remembered; its possible some refactoring is possible, but it doesn't seem like the necessity I had thought it would be.) I freely admit that I copy-and-pasted at least one comment about type-barriers from elsewhere in the getElem without being 100% certain that the comment was relevant. I will happily remove it if desired.

Comment on attachment 827490 [details] [diff] [review] patch A v1: jit-support for getElem on TypedObjects Here are the changes for jit-support on array-access of TypedObjects. The changes were originally written atop Niko's branch with the Sized/Unsized support, so some of the changes here may seem silly (e.g. the introduction of |allOfArrayKind()|) but they act as placeholders for code that will be necessary later on.

Comment on attachment 827491 [details] [diff] [review] patch B v1: tests for patch A tests for jit-support for array access in TypedObjects. Note that the version of TypedObjects on mozilla-inbound does not yet support the nicer |TypedObject.uintNN.array(length)| function syntax. I've left that version in a comment in the tests where I had originally used it, under the assumption that we eventually would add that function in.

Comment on attachment 827490 [details] [diff] [review] patch A v1: jit-support for getElem on TypedObjects Review of attachment 827490 [details] [diff] [review]: ----------------------------------------------------------------- made some comments. I agree some consolidation of code is in order, but since we'd like this to work sooner rather than later, we can file a second bug for that cleanup. ::: js/src/jit/IonBuilder.cpp @@ +6530,5 @@ > + > + // Get the length. > + MInstruction *length; > + size_t lenOfAll; > + switch (objTypeReprs.kind()) { You already asserted that objTypeReprs is an array, this switch seems gratuitous. @@ +6553,5 @@ > + > + // Find location within the owner object. > + MDefinition *owner; > + MDefinition *indexFromOwner; > + if (obj->isNewDerivedTypedObject()) { Can we combine these calculations with `loadTypedObjectData()`? I see that `loadTypedObjectData()` takes an int32_t and that doesn't work here, but we could easily make `loadTypedObjectData()` a thin wrapper around another func that takes an `MDefinition*`. (This can be deferred for a later patch) @@ +6599,5 @@ > + // be valid and unbarriered. > + load->setResultType(knownType); > + load->setResultTypeSet(resultTypes); > + > + *emitted = true; I would promote this line to the point where the optimization becomes irreversible (usually the point where instructions are added) @@ +6620,5 @@ > + > + // Get the length. > + MInstruction *length; > + size_t lenOfAll; > + switch (objTypeReprs.kind()) { Again, you already asserted that it's an array @@ +6653,5 @@ > + > + // Find location within the owner object. > + MDefinition *owner; > + MDefinition *indexAsByteOffsetFromOwner; > + if (obj->isNewDerivedTypedObject()) { Cleanup as before (can be deferred to a later patch). @@ +6683,5 @@ > + types::TemporaryTypeSet *resultTypes = bytecodeTypes(pc); > + derived->setResultTypeSet(resultTypes); > + current->add(derived); > + current->push(derived); > + *emitted = true; Promote to irreversible point

Comment on attachment 827491 [details] [diff] [review] patch B v1: tests for patch A Review of attachment 827491 [details] [diff] [review]: ----------------------------------------------------------------- It'd be good to add a test of multidimensional arrays. ::: js/src/jit-test/tests/TypedObject/jit-read-int-from-int-array-in-struct.js @@ +1,1 @@ > +if (!this.hasOwnProperty("TypedObject")) Add a public domain license header. We're not consistent about it but apparently it's the right thing. There's an example or two in tests/ecma_6/TypedObject/*.js @@ +1,4 @@ > +if (!this.hasOwnProperty("TypedObject")) > + quit(); > + > +// var Vec3u16Type = TypedObject.uint16.array(3); Just remove these, I'll update the tests to the most recent syntax once unsized array patch lands @@ +2,5 @@ > + quit(); > + > +// var Vec3u16Type = TypedObject.uint16.array(3); > +var Vec3u16Type = new TypedObject.ArrayType(TypedObject.uint16, 3); > +var PairVec3u16Type = new TypedObject.StructType({fst: Vec3u16Type, Move these type definitions into the functions if they only apply to one test, otherwise they may accidentally be referenced from elsewhere. Better yet, break the tests into distinct files. @@ +18,5 @@ > +} > + > +foo_u16(); > + > +// var Vec3u32Type = TypedObject.uint32.array(3); Here too @@ +25,5 @@ > + snd: Vec3u32Type}); > + > +function foo_u32() { > + for (var i = 0; i < 15000; i += 6) { > + var p = new PairVec3u16Type({fst: [i, i+1, i+2], Case in point: Did you mean PairVec3u32Type? ::: js/src/jit-test/tests/TypedObject/jit-read-int-from-int-array.js @@ +1,5 @@ > +if (!this.hasOwnProperty("TypedObject")) > + quit(); > + > +// var Vec3u16Type = TypedObject.uint16.array(3); > +var Vec3u16Type = new TypedObject.ArrayType(TypedObject.uint16, 3); As in the other file, move this into the function and remove commented out code.

(In reply to Niko Matsakis [:nmatsakis] from comment #7) > Comment on attachment 827490 [details] [diff] [review] > > + switch (objTypeReprs.kind()) { > > You already asserted that objTypeReprs is an array, this switch seems > gratuitous. Yeah, I think the switch was an artifact left over from when I was trying to support both Sized and Unsized arrays when I was developing off your github branch. Will remove. I'll incorporate the other short-term feedback (promoting the set of *emitted) and file a bug for follow-up cleanup.

(In reply to Niko Matsakis [:nmatsakis] from comment #7) > Comment on attachment 827490 [details] [diff] [review] > patch A v1: jit-support for getElem on TypedObjects > > > + *emitted = true; > > I would promote this line to the point where the optimization becomes > irreversible (usually the point where instructions are added) AFAICT, that is a new pattern that you (Niko) injected with getElemTry{Scalar,Complex}ElemOfTypedObject. The rest of the code tends to set *emitted at the end of each control flow before returning true, which is the pattern I was trying to emulate. Having said that, I don't particularly care about one choice or the other. As long as there's no way to return false after setting *emitted (or equivalently, as long as all callers make sure to check the return value before looking at *emitted), it seems okay to set it earlier in the control flow as you suggest.

(In reply to Niko Matsakis [:nmatsakis] from comment #8) > Comment on attachment 827491 [details] [diff] [review] > patch B v1: tests for patch A > > Review of attachment 827491 [details] [diff] [review]: > ----------------------------------------------------------------- > > +function foo_u32() { > > + for (var i = 0; i < 15000; i += 6) { > > + var p = new PairVec3u16Type({fst: [i, i+1, i+2], > > Case in point: Did you mean PairVec3u32Type? Aaiiee, almost certainly. Okay, file-refactoring has been motivated. :) (And I'll add a multidimensional test too. Though I guess none of these are actually testing that they are being jit-compiled, as you and I discussed earlier.)

(inherits r+ from niko from previous patch)

(inherits r+ from niko from previous patch.)

> AFAICT, that is a new pattern that you (Niko) injected with > getElemTry{Scalar,Complex}ElemOfTypedObject. The rest of the code tends to set > *emitted at the end of each control flow before returning true, which is the pattern > I was trying to emulate. Hmm, true. And you are correct that some code returns "emitted" to indicate if there is an error, so I guess I am mistaken and we should move the *emitted = true to the end. Nonetheless, I'd appreciate at least a comment to mark the phase change.

try run: https://tbpl.mozilla.org/?tree=Try&rev=e6d94ac4408c

pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/669ccf5fb31e https://hg.mozilla.org/integration/mozilla-inbound/rev/3ccaefa09332

https://hg.mozilla.org/mozilla-central/rev/669ccf5fb31e https://hg.mozilla.org/mozilla-central/rev/3ccaefa09332