Bug 211914 - Undecided Arrays shouldn't need to be OriginalArray to covert to GetArrayLength
Summary: Undecided Arrays shouldn't need to be OriginalArray to covert to GetArrayLength
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
: 211301 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-14 12:20 PDT by Keith Miller
Modified: 2020-05-15 00:23 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.58 KB, patch)
2020-05-14 12:22 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (4.83 KB, patch)
2020-05-14 12:52 PDT, Keith Miller
sbarati: review+
keith_miller: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-05-14 12:20:37 PDT
Undecided Arrays shouldn't need to be OriginalArray to covert to GetArrayLength
Comment 1 Keith Miller 2020-05-14 12:22:37 PDT
Created attachment 399390 [details]
Patch
Comment 2 Saam Barati 2020-05-14 12:37:36 PDT
Comment on attachment 399390 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        Also, fix a bug that arrayModesThatPassFiltering() can't handle
> +        Undecided arrays.

it'd be great if you could explain what's going on here.

Are you fixing correctness, perf, both? How?
Comment 3 Mark Lam 2020-05-14 12:38:34 PDT
Comment on attachment 399390 [details]
Patch

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

> JSTests/stress/undecided-arrays-should-not-need-original-array-for-length.js:11
> +const nonUndecidedFrequency = 1000

What is this for?  Is it needed?
Comment 4 Keith Miller 2020-05-14 12:50:14 PDT
Comment on attachment 399390 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        Undecided arrays.
> 
> it'd be great if you could explain what's going on here.
> 
> Are you fixing correctness, perf, both? How?

It's correctness because we can now emit a CheckArray on Undecided, which AI means will try to figure out what types flow out of. But since Undecided was unhandled, AI will assume bottom is the only possible value and we will crash at runtime.

>> JSTests/stress/undecided-arrays-should-not-need-original-array-for-length.js:11
>> +const nonUndecidedFrequency = 1000
> 
> What is this for?  Is it needed?

That's left over from when I was trying to make this test work. I'll delete.
Comment 5 Keith Miller 2020-05-14 12:52:32 PDT
Created attachment 399393 [details]
Patch
Comment 6 Saam Barati 2020-05-14 14:23:38 PDT
Comment on attachment 399393 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        bottom is the only possible value and insert a breakpoint, which

nit: AI doesn't insert a breakpoint :-)

FTL/DFG compilers, based on what AI says, will

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:243
> +        // As long as we have a JSArray getting its length shouldn't require any sane chainness.

JSArray => JSArray,
Comment 7 Keith Miller 2020-05-14 16:57:38 PDT
Comment on attachment 399393 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:12
>> +        bottom is the only possible value and insert a breakpoint, which
> 
> nit: AI doesn't insert a breakpoint :-)
> 
> FTL/DFG compilers, based on what AI says, will

That's a hell of a nit lol.

Clarified.

>> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:243
>> +        // As long as we have a JSArray getting its length shouldn't require any sane chainness.
> 
> JSArray => JSArray,

Done.
Comment 8 Keith Miller 2020-05-14 17:08:28 PDT
> >> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:243
> >> +        // As long as we have a JSArray getting its length shouldn't require any sane chainness.
> > 
> > JSArray => JSArray,
> 
> Done.

Actually, I don't think there should be a comma here. I think it's just a normal preposition.
Comment 9 Keith Miller 2020-05-14 17:18:05 PDT
*** Bug 211301 has been marked as a duplicate of this bug. ***
Comment 10 Keith Miller 2020-05-14 17:20:31 PDT
Committed r261725: <https://trac.webkit.org/changeset/261725>
Comment 11 Radar WebKit Bug Importer 2020-05-14 17:21:16 PDT
<rdar://problem/63250437>
Comment 12 Minh Tran 2020-05-15 00:23:30 PDT
*** Bug 211301 has been marked as a duplicate of this bug. ***