| Summary: | [JSC] Speculate children first in DFG NewArray | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Yusuke Suzuki
2020-08-07 23:04:05 PDT
Created attachment 406249 [details]
Patch
Comment on attachment 406249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406249&action=review r=me > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8456 > + // Because we first speculate on all of the children here, we never exit after creating I suggest /we never/we can never/. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8457 > + // uninitialized contiguous JSArray, which ensures that we never produce half-baked JSArray. I suggest /we never produce half-baked/we will never produce a half-baked/. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12237 > + // After the allocation, we must not exit until we fill butterfly completely. I wonder if we should add a VM::disallowOSRExit flag that we can set here if ASSERT_ENABLED and assert that the flag is not set in the OSR Exit ramp. We'll also need to clear the flag below. Maybe we can do that in another patch later. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6755 > + // Because we first speculate on all of the children here, we never exit after creating > + // uninitialized contiguous JSArray, which ensures that we never produce half-baked JSArray. Ditto: same suggestions as in above DFG comment. media/remote-control-command-seek.html is recently failing in multiple EWS attempts with different patches. https://ews-build.webkit.org/#/builders/32/builds/15348 https://ews-build.webkit.org/#/builders/32/builds/15361 Comment on attachment 406249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406249&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8456 >> + // Because we first speculate on all of the children here, we never exit after creating > > I suggest /we never/we can never/. Fixed. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8457 >> + // uninitialized contiguous JSArray, which ensures that we never produce half-baked JSArray. > > I suggest /we never produce half-baked/we will never produce a half-baked/. Fixed. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12237 >> + // After the allocation, we must not exit until we fill butterfly completely. > > I wonder if we should add a VM::disallowOSRExit flag that we can set here if ASSERT_ENABLED and assert that the flag is not set in the OSR Exit ramp. We'll also need to clear the flag below. Maybe we can do that in another patch later. Sounds good. I'll file a bug for that. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6755 >> + // uninitialized contiguous JSArray, which ensures that we never produce half-baked JSArray. > > Ditto: same suggestions as in above DFG comment. Ditto. Committed r265405: <https://trac.webkit.org/changeset/265405> Opened for extension. https://bugs.webkit.org/show_bug.cgi?id=215310 |