1380344 - Ion-inline IsPackedArray

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Comment 1

[André Bargull [:anba]]

Attachment: bug1380344.patch

Jan, do you think it makes sense to inline IsPackedArray the way this patch does?

The patch improves this µ-benchmark from ~150ms to ~105ms for me:

    var a = [1];
    var q = 0;
    var t = Date.now();
    for (var i = 0; i < 1000000; ++i) {
        var c = [].concat(a);
        q += c.length;
    }
    print(Date.now() - t, q);

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8885754 [details] [diff] [review]
bug1380344.patch

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

Yes this makes a lot of sense!

::: js/src/jit/CodeGenerator.cpp
@@ +12747,5 @@
> +
> +    Label notPacked, done;
> +
> +    // Load elements and length.
> +    RegisterOrInt32Constant key = RegisterOrInt32Constant(lengthTemp);

Nit: you can use the register directly instead of RegisterOrInt32Constant. I'd also use |output| instead of lengthTemp so things are a bit easier for the register allocator.

::: js/src/jit/MCallOptimize.cpp
@@ +1766,5 @@
> +        return InliningStatus_NotInlined;
> +
> +    const Class* clasp = arrayTypes->getKnownClass(constraints());
> +    if (clasp != &ArrayObject::class_)
> +        return InliningStatus_NotInlined;

It might be interesting to check if we ever see cases where clasp is not nullptr but some other Class. In that case we can fold this to |false| at compile time.

::: js/src/jit/MIR.h
@@ +13807,5 @@
> +class MIsPackedArray
> +  : public MUnaryInstruction,
> +    public SingleObjectPolicy::Data
> +{
> +    MIsPackedArray(MDefinition* array)

Nit: explicit

@@ +13816,5 @@
> +
> +  public:
> +    INSTRUCTION_HEADER(IsPackedArray)
> +    TRIVIAL_NEW_WRAPPERS
> +    NAMED_OPERANDS((0, array))

Nit: can override getAliasSet here and call setMovable() in the constructor. See MInitializedLength for instance.

Comment 3

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> ::: js/src/jit/MCallOptimize.cpp
> @@ +1766,5 @@
> > +        return InliningStatus_NotInlined;
> > +
> > +    const Class* clasp = arrayTypes->getKnownClass(constraints());
> > +    if (clasp != &ArrayObject::class_)
> > +        return InliningStatus_NotInlined;
> 
> It might be interesting to check if we ever see cases where clasp is not
> nullptr but some other Class. In that case we can fold this to |false| at
> compile time.

Hmm, IsPackedArray is currently only called in four places in self-hosted code (Array.p.concat, Array.p.reduce[Right], and Reflect.construct) and I have a hard time predicting which of these callers, if any, may use a non-Array object. So, I'm not really sure if we need to handle this case. :-/

Comment 4

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> @@ +13816,5 @@
> > +
> > +  public:
> > +    INSTRUCTION_HEADER(IsPackedArray)
> > +    TRIVIAL_NEW_WRAPPERS
> > +    NAMED_OPERANDS((0, array))
> 
> Nit: can override getAliasSet here and call setMovable() in the constructor.
> See MInitializedLength for instance.

I also needed to add Op_IsPackedArray to GetObject in AliasAnalysisShared.cpp to avoid this assertion: http://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/js/src/jit/AliasAnalysisShared.cpp#158

Comment 5

[André Bargull [:anba]]

Attachment: bug1380344.patch

I've updated the patch with your suggestions from comment #2.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8886535 [details] [diff] [review]
bug1380344.patch

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

Thanks!

Comment 7

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d929d586209513f21fe082c7d03f2e4e97c43e68

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/477268fa767f
Add Ion support for IsPackedArray. r=jandem

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/477268fa767f