[webkit-reviews] review granted: [Bug 196886] DFG should be able to constant fold Object.create() with a constant prototype operand : [Attachment 367361] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted  review:
Bug 196886: DFG should be able to constant fold Object.create() with a constant
prototype operand
https://bugs.webkit.org/show_bug.cgi?id=196886

Attachment 367361: Patch

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




--- Comment #2 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 367361
  --> https://bugs.webkit.org/attachment.cgi?id=367361
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367361&action=review

Looks nice. r=me with nits.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2595
> +	       else {
> +		   RELEASE_ASSERT(base.isObject());

Since this child1 is `UntypedUse`, any value can come here. Let's do `else if
(base.isObject())` instead.
And initialize `structure` with nullptr since we now have a path that does not
assign anything to the variable structure :)

I think we can write a test, like,

function test()
{
    for (var i = 0; i < 1e6; ++i) {
	try { Object.create(undefined); } catch { }
    }
}

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2599
> +	       if (!!structure) {

Personally I like `if (structure)`.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:759
> +			   RELEASE_ASSERT(base.isObject());

Ditto.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:763
> +		       if (!!structure) {

Ditto.

> Source/JavaScriptCore/runtime/StructureCache.cpp:63
> +    holdLock(m_lock);

This immediately discards the LockHolder after this statement finishes, and
unlocked.
Use `auto locker = holdLock(m_lock);` instead.

> Source/JavaScriptCore/runtime/StructureCache.cpp:74
> +    holdLock(m_lock);

Ditto.

> Source/JavaScriptCore/runtime/StructureCache.h:61
>      StructureMap m_structures;

This is WeakGCMap. So it is held by Heap, and collector checks this without
taking this lock.
I think it is safe since we don't run GC end phase while compiler thread is
running.
Another thing I would like to ensure is that this holds Weak<> handles.
I think this does not return dead cells when the compiler threads successfully
read it concurrently.
Can you ensure they are safe?