Handle out-of-bounds accesses on lazy arguments better

NEW
Unassigned

Status

()

Core
JavaScript Engine
3 years ago
11 months ago

People

(Reporter: jandem, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

3 years ago
Traceur (see bug 729376) has two functions that look like this:

    eatId_: function() {
      var expected = arguments[0];
      ...
      if (expected)

The first argument is optional. Problem is that our lazy arguments optimization doesn't handle this pattern: if argc == 0, arguments[0] will be out-of-bounds and we call argumentsOptimizationFailed.

I think we should be able to do a pure lookup on the prototype, if we see an indexed property or getter we call argumentsOptimizationFailed, else we can just return undefined.

Note that V8 has a similar issue, but the difference is smaller for them:

SM:

In bounds: 3
Out of bounds: 3985

V8:

In bounds: 13
Out of bounds: 894

function g1() { x1 = arguments[0]; }
function g2() { x2 = arguments[0]; }
var count = 10000000;
function f1() {
    var t = new Date;
    for (var i=0; i<count; i++)
        g1(i);
    print("In bounds:", new Date - t);
}
f1();
function f2() {
    var t = new Date;
    for (var i=0; i<count; i++)
        g2();
    print("Out of bounds:", new Date - t);
}
f2();
(Reporter)

Comment 1

3 years ago
Note: for that Traceur benchmark this is not our biggest issue, it costs us ~5% or so but it will be more after we fix the more serious problems. Also fixing this will reduce GC pressure, so the win might be bigger.

And it likely affects more JS code.
(Reporter)

Updated

3 years ago
Blocks: 1177735
(Reporter)

Comment 2

3 years ago
This pattern is actually pretty common, Babel does something like this for a default argument like foo = 1:

  var foo = arguments[0] === undefined ? 1 : arguments[0];

While we could ask them to use arguments.length or something, this seems like a pretty reasonable pattern and I'm sure Traceur and Babel are not the only ones.

Will try to fix this soon.
(In reply to Jan de Mooij [:jandem] from comment #2)
> While we could ask them to use arguments.length or something, this seems
> like a pretty reasonable pattern and I'm sure Traceur and Babel are not the
> only ones.
> 

Note that using arguments.length wouldn't be correct: the default value must be used if the argument is `undefined`, not only if it's really missing.
(Reporter)

Comment 4

3 years ago
(In reply to Till Schneidereit [:till] from comment #3)
> Note that using arguments.length wouldn't be correct: the default value must
> be used if the argument is `undefined`, not only if it's really missing.

Ah, fair enough.

They could check arguments.length and *then* check if it's undefined but that's a bit unfortunate.
Luckily they updated Babel and now they check arguments.length first: https://github.com/babel/babel/issues/1845. Hopefully they are going to update six-speed soon.

Comment 6

11 months ago
We should fix this sometime.
You need to log in before you can comment on or make changes to this bug.