1101258 - Inline intrinsics IsTypedArray and TypedArrayLength

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: inline-typed-array

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8540179 [details] [diff] [review]
inline-typed-array

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

::: js/src/builtin/TypedArray.js
@@ +92,5 @@
>      if (len === 0)
>          return -1;
>  
>      // Steps 7-8.
> +    var n = fromIndex === undefined ? 0 : ToInteger(fromIndex);

Not sure why this change 1) makes sense in this patch/bug, or 2) is desirable at all.  There's no change in semantics, so is it just a step to avoid inlining ToInteger code at all or something?

::: js/src/jit/MCallOptimize.cpp
@@ +1949,5 @@
>  IonBuilder::InliningStatus
> +IonBuilder::inlineIsTypedArray(CallInfo &callInfo)
> +{
> +    if (callInfo.constructing() || callInfo.argc() != 1)
> +        return InliningStatus_NotInlined;

This isn't something to start changing in this bug, perhaps.  But why do we permit any intrinsic to be called in a way not designed for it?  Seems like every intrinsic that isn't meant to be constructible should assert not-constructing, every inlining of that call should assert not-constructing, every intrinsic meant to be called with one exact arity should assert that arity in the constructor and in the inlining, every intrinsic requiring an object/string/number/&c. for a parameter should assert that, &c.

@@ +1965,5 @@
> +
> +    bool result = false;
> +    switch (types->forAllClasses(IsTypedArrayClass)) {
> +    case types::TemporaryTypeSet::ForAllResult::ALL_FALSE:
> +    case types::TemporaryTypeSet::ForAllResult::EMPTY:

|case|s should be indented two more spaces.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8540179 [details] [diff] [review]
inline-typed-array

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

::: js/src/builtin/TypedArray.js
@@ +92,5 @@
>      if (len === 0)
>          return -1;
>  
>      // Steps 7-8.
> +    var n = fromIndex === undefined ? 0 : ToInteger(fromIndex);

@Waldo: it's probably because ToInteger is not inlined if the input might be |undefined|... Please file a bug to fix this, we have too many of those perf cliffs.

::: js/src/jit/MCallOptimize.cpp
@@ +1948,5 @@
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineIsTypedArray(CallInfo &callInfo)
> +{
> +    if (callInfo.constructing() || callInfo.argc() != 1)

What Waldo said, we can turn these into asserts. inlineIsConstructing does this already, so it should be fine.

@@ +1960,5 @@
> +    // information.
> +
> +    types::TemporaryTypeSet *types = callInfo.getArg(0)->resultTypeSet();
> +    if (!types)
> +        return InliningStatus_NotInlined;

Can you file a follow-up bug to inline this even if we don't have type information? Should be pretty straight-forward and avoids another potential perf cliff.

@@ +1984,5 @@
> +
> +IonBuilder::InliningStatus
> +IonBuilder::inlineTypedArrayLength(CallInfo &callInfo)
> +{
> +    if (callInfo.constructing() || callInfo.argc() != 1)

Same here.

Comment 4

[Jan de Mooij [:jandem]]

And btw, self-hosting those typed array methods is pretty exciting :) I wonder how fast those functions will be when they are used on different array types, but especially after inlining them it will be fine hopefully.

Comment 5

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebc1e5878fa2
https://hg.mozilla.org/integration/mozilla-inbound/

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/454c2dd6b929