Bug 212519

Summary: [JSC] for-in should allocate new temporary register for base
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
saam: review+
Patch for landing
none
Patch for landing
none
Patch for landing none

Description Yusuke Suzuki 2020-05-29 04:28:50 PDT 
[JSC] for-in should allocate new temporary register for base
Comment 1 Yusuke Suzuki 2020-05-29 04:32:54 PDT 
Created attachment 400568 [details]
Patch
Comment 2 Yusuke Suzuki 2020-05-29 04:33:43 PDT 
<rdar://problem/63722044>
Comment 3 Yusuke Suzuki 2020-05-29 14:19:54 PDT 
Created attachment 400615 [details]
Patch
Comment 4 Saam Barati 2020-05-29 14:42:57 PDT 
Comment on attachment 400615 [details]
Patch

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

Nice!

r=me

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1855
> +    auto canUseFastHasOwnProperty = [&] {

nice!

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1865
> +            return generator.variable(generator.propertyNames().thisIdentifier, ThisResolutionType::Local) == structureContext->baseVariable().value();

what happens for heap |this| inside arrow function?

> Source/JavaScriptCore/parser/ASTBuilder.h:1454
> +        && (dot->base()->isResolveNode() || dot->base()->isThisNode())
> +        && (!dot->base()->isResolveNode() || static_cast<ResolveNode*>(dot->base())->identifier() != "Reflect")) {

nit: you could make it:
((dot->base->isResolveNode() && ...->id() != "Reflect) || dot->base->isThisNode())
to avoid double vtable call
Comment 5 Yusuke Suzuki 2020-05-29 15:12:56 PDT 
Comment on attachment 400615 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1865
>> +            return generator.variable(generator.propertyNames().thisIdentifier, ThisResolutionType::Local) == structureContext->baseVariable().value();
> 
> what happens for heap |this| inside arrow function?

Arrow function loads |this| from scope to its local m_thisRegister. m_thisRegister can point to |this| in CallFrame or a variable register which is allocated for allow function.
So, after executing ensureThis(), m_thisRegister is always correct for |this|.

>> Source/JavaScriptCore/parser/ASTBuilder.h:1454
>> +        && (!dot->base()->isResolveNode() || static_cast<ResolveNode*>(dot->base())->identifier() != "Reflect")) {
> 
> nit: you could make it:
> ((dot->base->isResolveNode() && ...->id() != "Reflect) || dot->base->isThisNode())
> to avoid double vtable call

Sounds nice! Fixed.
Comment 6 Yusuke Suzuki 2020-05-29 16:48:19 PDT 
Created attachment 400634 [details]
Patch for landing
Comment 7 Yusuke Suzuki 2020-05-30 18:45:55 PDT 
Created attachment 400686 [details]
Patch for landing
Comment 8 Yusuke Suzuki 2020-05-30 18:47:57 PDT 
Created attachment 400687 [details]
Patch for landing
Comment 9 EWS 2020-05-30 20:20:44 PDT 
Committed r262354: <https://trac.webkit.org/changeset/262354>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400687 [details].