Bug 211914 – Undecided Arrays shouldn't need to be OriginalArray to covert to GetArrayLength

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Undecided Arrays shouldn't need to be OriginalArray to covert to GetArrayLength

[Keith Miller]

Created attachment 399390 [details]
Patch

[Saam Barati]

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?

[Mark Lam]

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?

[Keith Miller]

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.

[Keith Miller]

Created attachment 399393 [details]
Patch

[Saam Barati]

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,

[Keith Miller]

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.

[Keith Miller]

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

[Keith Miller]

*** Bug 211301 has been marked as a duplicate of this bug. ***

[Keith Miller]

Committed r261725: <https://trac.webkit.org/changeset/261725>

[Radar WebKit Bug Importer]

<rdar://problem/63250437>

[Minh Tran]

*** Bug 211301 has been marked as a duplicate of this bug. ***