933269 - Optimize array indices in typed objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Niko Matsakis [:nmatsakis]]

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

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: jit-support for getElem on TypedObjects

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v1: tests for patch A

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

(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.)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

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 5

[Felix S. Klock II [:pnkfelix, :fklock]]

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 6

[Felix S. Klock II [:pnkfelix, :fklock]]

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 7

[Niko Matsakis [:nmatsakis]]

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 8

[Niko Matsakis [:nmatsakis]]

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.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(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.

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(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.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

(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.)

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v2: jit-support for getElem on TypedObjects

(inherits r+ from niko from previous patch)

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v2: tests for patch A

(inherits r+ from niko from previous patch.)

Comment 14

[Niko Matsakis [:nmatsakis]]

> 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.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

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

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

pushed to inbound:

      https://hg.mozilla.org/integration/mozilla-inbound/rev/669ccf5fb31e

      https://hg.mozilla.org/integration/mozilla-inbound/rev/3ccaefa09332

Comment 17

[Carsten Book [:Tomcat]]

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