[webkit-reviews] review granted: [Bug 215308] [JSC] Speculate children first in DFG NewArray : [Attachment 406249] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 215308: [JSC] Speculate children first in DFG NewArray
https://bugs.webkit.org/show_bug.cgi?id=215308

Attachment 406249: Patch

https://bugs.webkit.org/attachment.cgi?id=406249&action=review




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 406249
  --> https://bugs.webkit.org/attachment.cgi?id=406249
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.