Inline Array.isArray properly in Ion

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Posted patch PatchSplinter Review
I noticed Speedometer's AngularJS test was calling Array.isArray about 400,000 times. Most of these calls are from Ion because we failed to inline them.

Ion can fold/nop Array.isArray in some cases, but this is pretty brittle. I'm attaching a patch to make isArray inlining a lot more robust - worst case we now use MIsArray which is still pretty fast and has an OOL call for proxies.

A few random micro-benchmarks to demonstrate this:

* testValues tests the case where the input is either an object or primitive and we use MIsArray. This is pretty common in the wild and this microbench improves from 320 ms -> 122 ms.

* testNonArrayObjects is when TI knows statically that all objects are definitely not arrays. We handle this more robustly now: 346 ms -> 41 ms.

Speedometer's AngularJS test also improves a bit but it's pretty noisy so let's see what AWFY says.

function testValues() {
    var arr = [1, null, [], {}];
    var res = 0;
    var t = new Date;
    for (var i = 0; i < 10000000; i++) {
        for (var j = 0; j < 4; j++) {
            if (Array.isArray(arr[j]))
                res++;
        }
    }
    print(new Date - t);
    return res;
}
testValues();

function testNonArrayObjects() {
    var arr = [{}, {}, Math, this];
    var res = 0;
    var t = new Date;
    for (var i = 0; i < 10000000; i++) {
        for (var j = 0; j < 4; j++) {
            if (Array.isArray(arr[j]))
                res++;
        }
    }
    print(new Date - t);
    return res;
}
testNonArrayObjects();
Attachment #8881643 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8881643 [details] [diff] [review]
Patch

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

Good catch!

::: js/src/jit/MCallOptimize.cpp
@@ +600,3 @@
>      }
>  
> +    using ForAllResult = TemporaryTypeSet::ForAllResult;

nit: using TemporaryTypeSet::ForAllResult;
Attachment #8881643 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 2

2 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> > +    using ForAllResult = TemporaryTypeSet::ForAllResult;
> 
> nit: using TemporaryTypeSet::ForAllResult;

I just tried that one, but the compiler doesn't like it:

MCallOptimize.cpp:602:29: error: using declaration cannot refer to class member
    using TemporaryTypeSet::ForAllResult;
          ~~~~~~~~~~~~~~~~~~^
MCallOptimize.cpp:602:11: note: use an alias declaration instead
    using TemporaryTypeSet::ForAllResult;
          ^
          ForAllResult =

Comment 3

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef43b448dab
Improve Ion-inlining of Array.isArray. r=nbp

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ef43b448dab
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.