Bug 172216 – [FTL] Support GetByVal with ArrayStorage and SlowPutArrayStorage

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Let's do this to

1. pave the way to handle mandelbrot.js in FTL
2. support ArrayStorage / SlowPutArrayStorage arrays, like, tagged template call site object (SixSpeed)

[Yusuke Suzuki]

Created attachment 310383 [details]
Patch

WIP: it works! Need tests

[Build Bot]

Comment on attachment 310383 [details]
Patch

Attachment 310383 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3762326

New failing tests:
jsc-layout-tests.yaml/js/script-tests/array-iterate-backwards.js.layout-ftl-eager-no-cjit

[Yusuke Suzuki]

Created attachment 310388 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 310390 [details]
Patch

WIP

[Build Bot]

Comment on attachment 310390 [details]
Patch

Attachment 310390 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3764007

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 310422 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 310768 [details]
Patch

[Saam Barati]

Comment on attachment 310768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310768&action=review

LGTM, just want clarification on your use of a particular operation.

> Source/JavaScriptCore/ChangeLog:12
> +        This patch adds GetByVal support for ArrayStorage and SlowPutArrayStorage.
> +        To lower CheckInBounds in FTL, we add a new GetVectorLength op. It only accepts
> +        ArrayStorage and SlowPutArrayStorage, then it produces vector length.
> +        CheckInBounds uses this vector length to perform bound checking for ArrayStorage
> +        and SlowPutArrayStorage.

Do we need something like this for the DFG as well, or is that already done?

> Source/JavaScriptCore/dfg/DFGClobberize.h:1143
> +    case GetVectorLength: {

You should add a validation rule for GetVectorLength

> Source/JavaScriptCore/ftl/FTLCapabilities.cpp:351
> +        default:
> +            return CannotCompile;

Isn't this invalid form?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3601
> +                vmCall(Int64, m_out.operation(operationGetByValArrayInt), m_callFrame, base, index));

How do we know the base is an array? What if it's just a normal object w/ this kind of indexing type? Maybe this bug already exists in the FTL today?

[Saam Barati]

Maybe we already have a bug. This program:

```
function foo(a, i) {
    return a[i];
}
noInline(foo);

let array = {};
array[0] = 10;
array[1] = 20;
array.foo = 20;

for (let i = 0; i < 10000; ++i) {
    foo(array, (Math.random() * 3) | 0);
}
```

Produces this FTL code for `foo`:
```
  19:<!0:->	ExitOK(MustGen, W:SideState, bc#0)
  23:< 3:->	GetStack(JS|PureInt, Final, arg1, machine:arg1, FlushedCell, R:Stack(6), bc#0)
  24:<!0:->	CheckArray(Cell:@23, MustGen, Int32+NonArray+OutOfBounds+AsIs, R:JSCell_indexingType,JSCell_structureID,JSCell_typeInfoType, Exits, bc#0)
  21:< 1:->	GetStack(JS|PureInt, BoolInt32, arg2, machine:arg2, FlushedInt32, R:Stack(7), bc#0)
  15:<!0:->	KillStack(MustGen, loc0, W:SideState, ClobbersExit, bc#0)
   4:<!0:->	ZombieHint(MustGen, loc0, W:SideState, ClobbersExit, bc#0, ExitInvalid)
  13:<!0:->	KillStack(MustGen, loc1, W:SideState, ClobbersExit, bc#0, ExitInvalid)
   6:<!0:->	ZombieHint(MustGen, loc1, W:SideState, ClobbersExit, bc#0, ExitInvalid)
  11:<!0:->	KillStack(MustGen, loc2, W:SideState, ClobbersExit, bc#0, ExitInvalid)
   8:<!0:->	ZombieHint(MustGen, loc2, W:SideState, ClobbersExit, bc#0, ExitInvalid)
   9:<!0:->	KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#0, ExitInvalid)
  10:<!0:->	ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#0, ExitInvalid)
   7:<!0:->	KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#0, ExitInvalid)
  12:<!0:->	ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#0, ExitInvalid)
   5:<!0:->	KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#0, ExitInvalid)
  14:<!0:->	ZombieHint(MustGen, loc5, W:SideState, ClobbersExit, bc#0, ExitInvalid)
   2:<!0:->	KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#1)
  18:<!0:->	ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#1, ExitInvalid)
   1:<!0:->	KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#3)
  20:<!0:->	ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#3, ExitInvalid)
  22:<!0:->	CheckTraps(MustGen, W:Watchpoint_fire, Exits, ClobbersExit, bc#6)
  29:<!0:->	InvalidationPoint(MustGen, W:SideState, Exits, bc#7)
  30:< 1:->	GetButterfly(Cell:@23, Storage|PureInt, R:JSObject_butterfly, Exits, bc#7)
  25:<!2:->	GetByVal(KnownCell:Kill:@23, Int32:Kill:@21, Untyped:Kill:@30, JS|MustGen|UseAsOther, Int32, Int32+NonArray+OutOfBounds+AsIs, R:World, W:Heap, Exits, ClobbersExit, bc#7)  predicting NonBoolInt32
   0:<!0:->	KillStack(MustGen, loc6, W:SideState, ClobbersExit, bc#7, ExitInvalid)
  26:<!0:->	MovHint(Untyped:@25, MustGen, loc6, W:SideState, ClobbersExit, bc#7, ExitInvalid)
  27:<!0:->	InvalidationPoint(MustGen, W:SideState, Exits, bc#7, exit: bc#13)
  28:<!0:->	Return(Untyped:Kill:@25, MustGen, W:SideState, Exits, bc#13)
```

Digging into the CheckArray code, I don't think it guarantees the speculation fails if the thing is not an array. It's more like check indexing shape. My worry i the GetByVal code calls operationGetByValArrayInt even when we haven't proven it's an array. Let me verify this actually happens, I'm building now.

[Saam Barati]

(In reply to Saam Barati from comment #9)
> Maybe we already have a bug. This program:
> 
> ```
> function foo(a, i) {
>     return a[i];
> }
> noInline(foo);
> 
> let array = {};
> array[0] = 10;
> array[1] = 20;
> array.foo = 20;
> 
> for (let i = 0; i < 10000; ++i) {
>     foo(array, (Math.random() * 3) | 0);
> }
> ```
> 
> Produces this FTL code for `foo`:
> ```
>   19:<!0:->	ExitOK(MustGen, W:SideState, bc#0)
>   23:< 3:->	GetStack(JS|PureInt, Final, arg1, machine:arg1, FlushedCell,
> R:Stack(6), bc#0)
>   24:<!0:->	CheckArray(Cell:@23, MustGen, Int32+NonArray+OutOfBounds+AsIs,
> R:JSCell_indexingType,JSCell_structureID,JSCell_typeInfoType, Exits, bc#0)
>   21:< 1:->	GetStack(JS|PureInt, BoolInt32, arg2, machine:arg2,
> FlushedInt32, R:Stack(7), bc#0)
>   15:<!0:->	KillStack(MustGen, loc0, W:SideState, ClobbersExit, bc#0)
>    4:<!0:->	ZombieHint(MustGen, loc0, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>   13:<!0:->	KillStack(MustGen, loc1, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>    6:<!0:->	ZombieHint(MustGen, loc1, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>   11:<!0:->	KillStack(MustGen, loc2, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>    8:<!0:->	ZombieHint(MustGen, loc2, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>    9:<!0:->	KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>   10:<!0:->	ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>    7:<!0:->	KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>   12:<!0:->	ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>    5:<!0:->	KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>   14:<!0:->	ZombieHint(MustGen, loc5, W:SideState, ClobbersExit, bc#0,
> ExitInvalid)
>    2:<!0:->	KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#1)
>   18:<!0:->	ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#1,
> ExitInvalid)
>    1:<!0:->	KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#3)
>   20:<!0:->	ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#3,
> ExitInvalid)
>   22:<!0:->	CheckTraps(MustGen, W:Watchpoint_fire, Exits, ClobbersExit, bc#6)
>   29:<!0:->	InvalidationPoint(MustGen, W:SideState, Exits, bc#7)
>   30:< 1:->	GetButterfly(Cell:@23, Storage|PureInt, R:JSObject_butterfly,
> Exits, bc#7)
>   25:<!2:->	GetByVal(KnownCell:Kill:@23, Int32:Kill:@21, Untyped:Kill:@30,
> JS|MustGen|UseAsOther, Int32, Int32+NonArray+OutOfBounds+AsIs, R:World,
> W:Heap, Exits, ClobbersExit, bc#7)  predicting NonBoolInt32
>    0:<!0:->	KillStack(MustGen, loc6, W:SideState, ClobbersExit, bc#7,
> ExitInvalid)
>   26:<!0:->	MovHint(Untyped:@25, MustGen, loc6, W:SideState, ClobbersExit,
> bc#7, ExitInvalid)
>   27:<!0:->	InvalidationPoint(MustGen, W:SideState, Exits, bc#7, exit: bc#13)
>   28:<!0:->	Return(Untyped:Kill:@25, MustGen, W:SideState, Exits, bc#13)
> ```
> 
> Digging into the CheckArray code, I don't think it guarantees the
> speculation fails if the thing is not an array. It's more like check
> indexing shape. My worry i the GetByVal code calls operationGetByValArrayInt
> even when we haven't proven it's an array. Let me verify this actually
> happens, I'm building now.

Yeah we totally call this function here. Anyways, this is a benign bug, since it immediately lowers the type to cell:

```
ALWAYS_INLINE EncodedJSValue getByValCellInt(ExecState* exec, JSCell* base, int32_t index)
{
    VM* vm = &exec->vm();
    NativeCallFrameTracer tracer(vm, exec);
    
    if (index < 0) {
        // Go the slowest way possible becase negative indices don't use indexed storage.
        return JSValue::encode(JSValue(base).get(exec, Identifier::from(exec, index)));
    }

    // Use this since we know that the value is out of bounds.
    return JSValue::encode(JSValue(base).get(exec, static_cast<unsigned>(index)));
}

EncodedJSValue JIT_OPERATION operationGetByValArrayInt(ExecState* exec, JSArray* base, int32_t index)
{
    return getByValCellInt(exec, base, index);
}
```

That said, lets please rename this function since it's clearly being misused. Perhaps we can just pass an object base.

[Saam Barati]

r=me with function rename and type modification.

[Yusuke Suzuki]

Comment on attachment 310768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310768&action=review

Thanks. OK, I'll rename the function to operationGetByValObjectInt.

>> Source/JavaScriptCore/ChangeLog:12
>> +        and SlowPutArrayStorage.
> 
> Do we need something like this for the DFG as well, or is that already done?

Yeah, DFG already has this. FTL missed it, then this patch adds it.

[Yusuke Suzuki]

Comment on attachment 310768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310768&action=review

>> Source/JavaScriptCore/dfg/DFGClobberize.h:1143
>> +    case GetVectorLength: {
> 
> You should add a validation rule for GetVectorLength

It's nice idea. Added.

>> Source/JavaScriptCore/ftl/FTLCapabilities.cpp:351
>> +            return CannotCompile;
> 
> Isn't this invalid form?

Yes, I've changed it to RELEASE_ASSERT_NOT_REACHED().

[Yusuke Suzuki]

Committed r217202: <http://trac.webkit.org/changeset/217202>