894881 - Accessing typed array properties like byteLength is very slow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Noticed this while looking at bug 862249 (the benchTypedArraySet test there).

Accessing a typed array property like byteLength is super slow:
--
function f(arr) {
    var res = 0;
    for (var i=0; i<1000000; i++) {
	res = arr.byteLength;
    }
    return res;
}

var t = new Date;
f(new Int32Array(1000));
print(new Date - t);
--

d8        :  16 ms
js no-ion : 171 ms
js        : 226 ms

Note that Ion knows how to optimize .length and indeed that's very fast:

js  :  1 ms
d8  : 16 ms

I think we should:

(1) Make the getter IC work here; it's probably failing because typed arrays are non-native.
(2) Have Ion inline .byteOffset/.byteLength/etc like .length. These are just int32 values stored in fixed slots; should be straight-forward.

Comment 1

[Jan de Mooij [:jandem]]

Note: I know LICM is hoisting the .length access out of the loop. But that's the point: inlining .byteLength etc allows us to do the same for these other properties.

Comment 2

[Katelyn Gadd (:kael)]

This explains a lot! I never considered the possibility that only certain properties got 'special' optimization treatment. Is there a list of them anywhere?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Kevin Gadd (:kael) from comment #2)
> This explains a lot! I never considered the possibility that only certain
> properties got 'special' optimization treatment. Is there a list of them
> anywhere?

We inline .length on arrays, typed arrays, strings, arguments..

Fixing the getter IC to work with typed arrays brings this down to 12 ms, 4 ms faster than v8. I also want to do (2) in comment 0 but (1) should fix the worst problems.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Fixes the Baseline ICs to cache properties on typed arrays, and fixes IonBuilder so that our inlined-getter-call code works for getters (like byteLength) on typed arrays.

Ion should really inline these properties directly without calling the getter (they just load a value from a fixed slot), but for now this is much better than what we were doing. For the micro-benchmark in comment 0:

before: 220 ms
after:   10 ms

(We're not hoisting the property access, once we optimize this property in Ion we can do that.)

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 8333772 [details] [diff] [review]
Patch

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

::: js/src/jit/IonBuilder.cpp
@@ +5932,5 @@
> +{
> +    if (IsTypedArrayClass(clasp)) {
> +        // Typed arrays have a lookupGeneric hook, but it only handles
> +        // integers, not names.
> +        JS_ASSERT(name);

Maybe move this assert above the condition?

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef1eb4f5d57

(In reply to Brian Hackett (:bhackett) from comment #5)
> Maybe move this assert above the condition?

I think it's better to keep it below the comment, to make it clear why the assert is there.

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> I think it's better to keep it below the comment, to make it clear why the
> assert is there.

Yeah, though when I see asserts inside a conditional I generally figure that assert might not hold in the other branch, whereas in this case name should always be non-NULL.  I guess both the assert and comment could be hoisted.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ef1eb4f5d57