Bug 115805 – DFGArrayMode::fromObserved is too liberal when it sees different Array and NonArray shapes

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

It checks the observed ArrayModes to see if we have seen any ArrayWith* first. If so, it assumes it's an Array::Array, even if we've also observed any NonArrayWith* in the ArrayProfile. This leads to the code generated by jumpSlowForUnwantedArrayMode to check the indexing type against (shape | IsArray) instead of just shape, which can cause us to exit a lot in the case that we saw have a NonArray.

To fix, we simply need to change the order of the if-statements in fromObserved to check for NonArray ArrayModes first.

[Mark Hahnenberg]

Created attachment 201076 [details]
Patch

[Filip Pizlo]

Comment on attachment 201076 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:120
> -        if (observed & (asArrayModes(ArrayWithUndecided) | asArrayModes(ArrayWithInt32) | asArrayModes(ArrayWithDouble) | asArrayModes(ArrayWithContiguous) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)))
> -            arrayClass = Array::Array;
> -        else if (observed & (asArrayModes(NonArray) | asArrayModes(NonArrayWithInt32) | asArrayModes(NonArrayWithDouble) | asArrayModes(NonArrayWithContiguous) | asArrayModes(NonArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage)))
> +        if (observed & (asArrayModes(NonArray) | asArrayModes(NonArrayWithInt32) | asArrayModes(NonArrayWithDouble) | asArrayModes(NonArrayWithContiguous) | asArrayModes(NonArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage)))
>              arrayClass = Array::NonArray;
> +        else if (observed & (asArrayModes(ArrayWithUndecided) | asArrayModes(ArrayWithInt32) | asArrayModes(ArrayWithDouble) | asArrayModes(ArrayWithContiguous) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)))
> +            arrayClass = Array::Array;
>          else
>              arrayClass = Array::PossiblyArray;

Actually, I think the bug here is that PossiblyArray wasn't working right.  PossiblyArray is supposed to represent "this could be an Array or a NonArray".  So it appears that the problem here is that PossiblyArray isn't being compiled to do the right thing (i.e. accept either Array or NonArray).

[Mark Hahnenberg]

(In reply to comment #2)
> (From update of attachment 201076 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201076&action=review
> 
> > Source/JavaScriptCore/dfg/DFGArrayMode.cpp:120
> > -        if (observed & (asArrayModes(ArrayWithUndecided) | asArrayModes(ArrayWithInt32) | asArrayModes(ArrayWithDouble) | asArrayModes(ArrayWithContiguous) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)))
> > -            arrayClass = Array::Array;
> > -        else if (observed & (asArrayModes(NonArray) | asArrayModes(NonArrayWithInt32) | asArrayModes(NonArrayWithDouble) | asArrayModes(NonArrayWithContiguous) | asArrayModes(NonArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage)))
> > +        if (observed & (asArrayModes(NonArray) | asArrayModes(NonArrayWithInt32) | asArrayModes(NonArrayWithDouble) | asArrayModes(NonArrayWithContiguous) | asArrayModes(NonArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage)))
> >              arrayClass = Array::NonArray;
> > +        else if (observed & (asArrayModes(ArrayWithUndecided) | asArrayModes(ArrayWithInt32) | asArrayModes(ArrayWithDouble) | asArrayModes(ArrayWithContiguous) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)))
> > +            arrayClass = Array::Array;
> >          else
> >              arrayClass = Array::PossiblyArray;
> 
> Actually, I think the bug here is that PossiblyArray wasn't working right.  PossiblyArray is supposed to represent "this could be an Array or a NonArray".  So it appears that the problem here is that PossiblyArray isn't being compiled to do the right thing (i.e. accept either Array or NonArray).

Gotcha. So we need to check if there's both NonArray and Array stuff first and if so use PossiblyArray, otherwise do what we were doing.

[Mark Hahnenberg]

Created attachment 201129 [details]
Patch

[Geoffrey Garen]

Comment on attachment 201129 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:11
> +        instead of just shape, which can cause us to exit a lot in the case that we saw have a NonArray.

Typo: Should be "...case that we saw...".

[Mark Hahnenberg]

Committed r149834: <http://trac.webkit.org/changeset/149834>