937391 - Refactor typed object element / property access

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Niko Matsakis [:nmatsakis]]

There is some duplicated code in the optimization paths for typed object elements and properties. In addition, the optimizations do not always compose and thus we are sometimes left with residual `DerivedTypedObject` instructions in the MIR. These strain the GC and can lead to bad performance. 

Attached is a test where typed objects performs 10x slower than standard JS, today but with this patch they are 2x faster.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug937391.diff

Factor out some common from elem/prop accesses, in the process optimizing chained accesses better (previously we failed to remove some MNewDerivedTypeObjects)

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: jaswanth.js

Example test (I plan to submit something like this to awfy)

Comment 3

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

Comment on attachment 830580 [details] [diff] [review]
Bug937391.diff

Review of attachment 830580 [details] [diff] [review]:
-----------------------------------------------------------------

It all looks good, if only from a code-cleanup perspective.

For my own edification: There are clearly performance benefits you are noting from this.  I cannot tell from the patch alone which part makes the DerivedTypedObject intermediate instructions go away in the MIR.  Is it some artifact of your gathering up *all* of the |obj->toNewDerivedTypeObject()| invocations behind |loadTypedObjectData| (potentially indirectly via |loadTypedObjectElements| ?

I guess just point me at the part of the patch where I was failing to allow the optimizations to compose, if possible.

::: js/src/jit/IonBuilder.cpp
@@ +6559,5 @@
>  
> +    // Find location within the owner object.
> +    MDefinition *elements, *scaledOffset;
> +    loadTypedObjectElements(obj, indexAsByteOffset, elemTypeRepr->alignment(),
> +                            &elements, &scaledOffset);

1. You could say just |alignment| here instead of repeating |elemTypeRepr->alignment()|.

2. I assume that the distinction (in performance) is immaterial between "scaling owner offset to alignment-units at outset before adding to index" and "scaling index to byte-units, adding to owner offset, and then scaling sum back to alignment units".

@@ +9586,5 @@
> +
> +// Takes as input a typed object, an offset into that typed object's
> +// memory, and the type repr of the data found at that offset. Returns
> +// the elements pointer and a scaled offset. The scaled offset is
> +// expressed in units of `unit`; when working with typed array MIR,

Isn't the convention for |unit| rather than `unit`?  (I know you use backquotes elsewhere in this file; I figured you might want the chance to "fix" it across the board before others note it.)

@@ +9661,5 @@
> +    MInstruction *elemType = MLoadFixedSlot::New(typeObj, JS_TYPEOBJ_SLOT_ARRAY_ELEM_TYPE);
> +    current->add(elemType);
> +
> +    MInstruction *unboxElemType = MUnbox::New(elemType, MIRType_Object, MUnbox::Infallible);
> +    current->add(unboxElemType);

ah good catch, this must have been missing from my own array handling code.

Comment 4

[Niko Matsakis [:nmatsakis]]

Comment on attachment 830580 [details] [diff] [review]
Bug937391.diff

Review of attachment 830580 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ -8762,5 @@
> -    // OK!
> -    *emitted = true;
> -
> -    MTypedObjectElements *elements = MTypedObjectElements::New(obj);
> -    current->add(elements);

Felix, Jandem -- The bug is here. It's actually in old code, I think. This code fails to account for the possibility that `obj` is a derived typed object.

Comment 5

[Niko Matsakis [:nmatsakis]]

> 2. I assume that the distinction (in performance) is immaterial between "scaling owner offset to alignment-units at outset before adding to index" and "scaling index to byte-units, adding to owner offset, and then scaling sum back to alignment units".

I'm not happy about it, but it doesn't seem to make much difference. I figured I'd do a follow up patch sometime that converts all units uniformly to bytes.

Comment 6

[Niko Matsakis [:nmatsakis]]

> Isn't the convention for |unit| rather than `unit`?  (I know you use backquotes elsewhere
> in this file; I figured you might want the chance to "fix" it across the board before others note it.)

I suppose. Conventions throughout js seem to be somewhat mixed between separating identifiers and code fragments using "", ||, and just nothing, but I may be the only one that habitually places backticks.

Comment 7

[Niko Matsakis [:nmatsakis]]

Try run (including 3 other bugs whose patches are already r+):

https://tbpl.mozilla.org/?tree=Try&rev=ad8fccfc779f

All green except for one mysterious build failure in B2G emulator DOM code, which I suspect is unrelated.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 830580 [details] [diff] [review]
Bug937391.diff

Review of attachment 830580 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ +9605,5 @@
> +
> +    // Scale to a different unit for compat with typed array MIRs.
> +    if (unit != 1) {
> +        MDiv *scaledOffset = MDiv::NewAsmJS(ownerOffset, constantInt(unit),
> +                                            MIRType_Int32);

Nit: should fit on 1 line.

@@ +9704,5 @@
> +    // Find location within the owner object.
> +    MDefinition *elements, *scaledOffset;
> +    loadTypedObjectElements(typedObj, offset, typeRepr->alignment(), &elements, &scaledOffset);
> +
> +    // Clamp value to [0, 255] for Uint8ClampedArray.

s/Uint8ClampedArray/TypedObject.uint8Clamped or something (IIUC).

Comment 9

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e31629cfa31c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e31629cfa31c