Bug 215308 – [JSC] Speculate children first in DFG NewArray

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Speculate children first in DFG NewArray

[Yusuke Suzuki]

Created attachment 406249 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/64749263>

[Mark Lam]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r265405: <https://trac.webkit.org/changeset/265405>

[Yusuke Suzuki]

Opened for extension. https://bugs.webkit.org/show_bug.cgi?id=215310