Bug 115805 - DFGArrayMode::fromObserved is too liberal when it sees different Array and NonArray shapes
Summary: DFGArrayMode::fromObserved is too liberal when it sees different Array and No...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-08 08:02 PDT by Mark Hahnenberg
Modified: 2013-05-09 12:37 PDT (History)
0 users

See Also:

Attachments
Patch (5.93 KB, patch)
2013-05-08 08:09 PDT, Mark Hahnenberg 		no flags Details | Formatted Diff | Diff
Patch (6.40 KB, patch)
2013-05-08 19:43 PDT, Mark Hahnenberg 		ggaren: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2013-05-08 08:02:55 PDT 
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.
Comment 1 Mark Hahnenberg 2013-05-08 08:09:24 PDT 
Created attachment 201076 [details]
Patch
Comment 2 Filip Pizlo 2013-05-08 16:42:53 PDT 
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).
Comment 3 Mark Hahnenberg 2013-05-08 16:44:16 PDT 
(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.
Comment 4 Mark Hahnenberg 2013-05-08 19:43:01 PDT 
Created attachment 201129 [details]
Patch
Comment 5 Geoffrey Garen 2013-05-09 12:18:51 PDT 
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...".
Comment 6 Mark Hahnenberg 2013-05-09 12:37:43 PDT 
Committed r149834: <http://trac.webkit.org/changeset/149834>