Accessing typed array properties like byteLength is very slow

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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

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

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
Created attachment 8333772 [details] [diff] [review]
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.)
Attachment #8333772 - Flags: review?(bhackett1024)
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?
Attachment #8333772 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

4 years ago
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/5ef1eb4f5d57
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.