1383436 - Add support for ToInteger when called with undefined and doubles

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[André Bargull [:anba]]

jQuery's inArray() function [1] calls Array.prototype.indexOf with fromIndex == undefined, but we don't support inlining ToInteger with undefined, so we always call into the VM.

This issue can be observed in Speedometer BackboneJS-TodoMVC and Flight-TodoMVC.



[1] https://github.com/WebKit/webkit/blob/ac12a7e5187b9ec6ebeee570972b6c91335254fe/PerformanceTests/Speedometer/resources/todomvc/architecture-examples/backbone/node_modules/jquery/dist/jquery.js#L408-L410

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

So, I pulled together a quick small benchmark to see this in-situ. I tried to bake this directly from the above linked jQuery code. 

IndexOfUndefined.js:
    
    inArray = function( elem, arr, i ) {
                    return arr == null ? -1 : arr.indexOf(elem); 
    }
    
    function test(y) {
        var a = new Array(1000).fill(1);
    
        var q = 0;
        var t = dateNow();
        for (var i = 0; i < 100; ++i) {
            for (var j = 0, len = a.length; j < len; ++j) {
                    inArray(y, a, 0);
            }
        }
        return [dateNow() - t, q];
    }
    
    print(test(1));
    print(test(2));
    print(test(undefined));
    

Running this, I definitely see an issue with undefined: 

    build/dist/bin/js  ~/bugs/1383436/IndexOfUndefined.js 
    16.861083984375,0
    61.501953125,0
    7748.599853515625,0

What I've not found is where we're doing the to-integer conversion. Trying to parse through the spec, it seems like maybe what's being referred to above is having the second arg to indexOf be undefined (https://tc39.github.io/ecma262/#sec-array.prototype.indexof, step 4); tweaking the above test case to do that: 

    arr.indexOf(elem,undefined)

I get results that look pretty much identical: 

    26.718994140625,0
    81.098876953125,0
    7398.932861328125,0
 
Am I looking in the wrong place?

Comment 2

[Jeff Walden [:Waldo]]

That's the right place.  Code for a self-hosted function like this should be in builtin/*.js for the relevant name.

Comment 3

[André Bargull [:anba]]

Here's a better µ-benchmark to show the ToInteger inlining issue:
---
function test(fromIndex) {
    var a = [1];
    var t = dateNow();
    for (var i = 0; i < 10000000; ++i) {
        a.indexOf(2, fromIndex);
    }
    return dateNow() - t;
}
print(test(0));
print(test()); // should be ~4x slower than `test(0)`.
---

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 1: Remove unnecessary object-type check in convertValueToInt. r=jandem!

Avoid emitting object type test when already bailing out for any non-undefined type.

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 2: Update step comments in self-hosted functions calling ToInteger. r=jandem!

Depends on D37284

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 3: Add ToIntegerPositiveZero as a new variant of ToInteger. r=jandem!

ToIntegerPositiveZero has almost identical semantics as ToInteger with the sole
exception that negative zero is converted to positive zero.

The next patches will update callers to use this function instead of ToInteger
where possible and improve the inlining of this function.

Depends on D37285

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 4: Update self-hosted callers to use ToIntegerPositiveZero. r=jandem!

All self-hosted functions calling ToInteger can be updated to use
ToIntegerPositiveZero, because there were either already explicit or implicit
coercions from negative to positive zero present, e.g. addition with +0 or a
call to Math.max(v, 0), or the distinction between negative and positive
zero isn't observable, e.g. when indexing into an object obj[index].

Depends on D37286

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 5: Move inlining support from ToInteger to ToIntegerPositiveZero. r=jandem!

And remove ToInteger from the self-hosting global, because it's no longer used.

Depends on D37287

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 6: Inline ToIntegerPositiveZero when returning Double values. r=jandem!

MNearbyInt and MMathFunction both support |undefined| input arguments, so only
disallow |undefined| when returning an Int32.

Depends on D37288

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 7: Fix branchTruncate{Double,Float}ToInt32 for x64 and avoid negative zero check for ARM64. r=jandem!

Depends on D37289

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 8: Add MToIntegerInt32 to specialise ToInteger operations. r=jandem!

The code is mostly based on the existing MToNumberInt32 class.

Depends on D37290

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 9: Use MToIntegerInt32 when inlining ToIntegerPositiveZero. r=jandem!

Depends on D37291

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1383436 - Part 10: Use MToIntegerInt32 when inlining charAt and charCodeAt. r=jandem!

This matches the spec better and relaxes the input argument constraints for both functions.

Depends on D37292

Comment 14

[André Bargull [:anba]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f8004b8bbb5
Part 1: Remove unnecessary object-type check in convertValueToInt. r=jandem
https://hg.mozilla.org/integration/autoland/rev/15ca7dc95546
Part 2: Update step comments in self-hosted functions calling ToInteger. r=jandem
https://hg.mozilla.org/integration/autoland/rev/aba15e0b2bed
Part 3: Add ToIntegerPositiveZero as a new variant of ToInteger. r=jandem
https://hg.mozilla.org/integration/autoland/rev/9fc3acf1e4c5
Part 4: Update self-hosted callers to use ToIntegerPositiveZero. r=jandem
https://hg.mozilla.org/integration/autoland/rev/43d790758a05
Part 5: Move inlining support from ToInteger to ToIntegerPositiveZero. r=jandem
https://hg.mozilla.org/integration/autoland/rev/edd4b486ee14
Part 6: Inline ToIntegerPositiveZero when returning Double values. r=jandem
https://hg.mozilla.org/integration/autoland/rev/e0aa46fb1ed1
Part 7: Fix branchTruncate{Double,Float}ToInt32 for x64 and avoid negative zero check for ARM64. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a901d5cfc7d2
Part 8: Add MToIntegerInt32 to specialise ToInteger operations. r=jandem
https://hg.mozilla.org/integration/autoland/rev/36477b4e736a
Part 9: Use MToIntegerInt32 when inlining ToIntegerPositiveZero. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d9d678e7422e
Part 10: Use MToIntegerInt32 when inlining charAt and charCodeAt. r=jandem

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1f8004b8bbb5
https://hg.mozilla.org/mozilla-central/rev/15ca7dc95546
https://hg.mozilla.org/mozilla-central/rev/aba15e0b2bed
https://hg.mozilla.org/mozilla-central/rev/9fc3acf1e4c5
https://hg.mozilla.org/mozilla-central/rev/43d790758a05
https://hg.mozilla.org/mozilla-central/rev/edd4b486ee14
https://hg.mozilla.org/mozilla-central/rev/e0aa46fb1ed1
https://hg.mozilla.org/mozilla-central/rev/a901d5cfc7d2
https://hg.mozilla.org/mozilla-central/rev/36477b4e736a
https://hg.mozilla.org/mozilla-central/rev/d9d678e7422e