Bug 212519 - [JSC] for-in should allocate new temporary register for base
Summary: [JSC] for-in should allocate new temporary register for base
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-29 04:28 PDT by Yusuke Suzuki
Modified: 2020-05-30 20:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.89 KB, patch)
2020-05-29 04:32 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (21.16 KB, patch)
2020-05-29 14:19 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing (23.00 KB, patch)
2020-05-29 16:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (23.12 KB, patch)
2020-05-30 18:45 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (23.10 KB, patch)
2020-05-30 18:47 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].