| Summary: | Undecided Arrays shouldn't need to be OriginalArray to covert to GetArrayLength | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||
| Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ews-watchlist, mark.lam, msaboff, myoki.crystal, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Keith Miller
2020-05-14 12:20:37 PDT
Created attachment 399390 [details]
Patch
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 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 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. Created attachment 399393 [details]
Patch
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 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. > >> 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.
*** Bug 211301 has been marked as a duplicate of this bug. *** Committed r261725: <https://trac.webkit.org/changeset/261725> *** Bug 211301 has been marked as a duplicate of this bug. *** |