Closed Bug 1383436 Opened 3 years ago Closed 3 months ago

Add support for ToInteger when called with undefined and doubles

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(10 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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
Priority: -- → P3
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?
Flags: needinfo?(andrebargull)
That's the right place.  Code for a self-hosted function like this should be in builtin/*.js for the relevant name.
Flags: needinfo?(andrebargull)
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)`.
---
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Summary: Add support for ToInteger when called with undefined → Add support for ToInteger when called with undefined and doubles

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

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

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

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

Depends on D37287

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

Depends on D37288

The code is mostly based on the existing MToNumberInt32 class.

Depends on D37290

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

Depends on D37292

Pushed by btara@mozilla.com:
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

Keywords: checkin-needed
Regressions: 1591019
You need to log in before you can comment on or make changes to this bug.