1131996 - Handle out-of-bounds accesses on lazy arguments better

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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();

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Till Schneidereit [:till]]

(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.

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[Tom S [:evilpie]]

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

[Tom S [:evilpie]]

We should fix this sometime.

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1131996 - Part 1: Move ClassCanHaveExtraProperties and CanAttachDenseElementHole. r=iain!

Moves ClassCanHaveExtraProperties and CanAttachDenseElementHole in preparation
for the next part

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1131996 - Part 2: Support out-of-bounds read access on arguments objects in CacheIR. r=iain!

The ELEMENT_OVERRIDDEN_BIT flag is set whenever any element is defined on an
arguments object, irrespective of whether the element is in-bounds or out-of-bounds.
That means that flag can also be used to determine if an arguments object has any
elements besides the frame arguments.

When reading a possible out-of-bounds index, we can therefore use the following
approach:

1. Fail whenever ELEMENT_OVERRIDDEN_BIT is set.
2. If the index is in-bounds:
   a. Return the in-bounds element unless it's FORWARD_TO_CALL_SLOT.
3. Else,
   a. Fail if the index is less than zero.
   b. Return undefined.

Plus a prototype guard check to ensure the element isn't present on any object
of the prototype chain.

Depends on D129619

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1131996 - Part 3: Support out-of-bounds read access on arguments objects in Warp. r=iain!

Transpile the CacheIR operation from part 2.

Depends on D129620

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1131996 - Part 4: Support scalar replacement for arguments objects with out-of-bounds read access. r=iain!

The follows the existing implementations of MGetInlinedArgument and
MGetFrameArgument. There are only two differences:

1. A bailout occurs when the index is negative. This implies both instructions
   must be guards.
2. Undefined is returned when the index is larger than the arguments length.

Depends on D129621

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1131996 - Part 5: Use scoped enums for CanAttachDenseElementHole. r=iain!

Use scoped enums for CanAttachDenseElementHole to improve the readibility
compared to using three plain bool parameters.

Depends on D129622

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c8232344e98
Part 1: Move ClassCanHaveExtraProperties and CanAttachDenseElementHole. r=iain
https://hg.mozilla.org/integration/autoland/rev/c31e81d4c5c9
Part 2: Support out-of-bounds read access on arguments objects in CacheIR. r=iain
https://hg.mozilla.org/integration/autoland/rev/cdc0ba4d147b
Part 3: Support out-of-bounds read access on arguments objects in Warp. r=iain
https://hg.mozilla.org/integration/autoland/rev/f7cb0f166402
Part 4: Support scalar replacement for arguments objects with out-of-bounds read access. r=iain
https://hg.mozilla.org/integration/autoland/rev/0cf5997d55ea
Part 5: Use scoped enums for CanAttachDenseElementHole. r=iain

Comment 13

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/2c8232344e98
https://hg.mozilla.org/mozilla-central/rev/c31e81d4c5c9
https://hg.mozilla.org/mozilla-central/rev/cdc0ba4d147b
https://hg.mozilla.org/mozilla-central/rev/f7cb0f166402
https://hg.mozilla.org/mozilla-central/rev/0cf5997d55ea