Bug 162111 – [JSC] Do not need to use defineProperty to define methods for object literals

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

[Yusuke Suzuki]

[JSC] Do not need to use defineProperty to define methods for object literals

[Yusuke Suzuki]

Created attachment 289159 [details]
Patch

[Saam Barati]

Comment on attachment 289159 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:593
> +    if (node.isClassProperty() && node.needsSuperBinding()) {

Why && and not ||? When is isClassProperty true but needsSuperBinding false?

[Yusuke Suzuki]

Comment on attachment 289159 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:593
>> +    if (node.isClassProperty() && node.needsSuperBinding()) {
> 
> Why && and not ||? When is isClassProperty true but needsSuperBinding false?

|| includes non-class cases we would like to avoid in this patch (isClassProperty = false, needsSuperBinding = true).
BTW, currently, if isClassProperty = true, needsSuperBinding is always true.
It may be changed (or not since we need to use defineProperty to perform conflict check) in the future when introducing class fields (static prop = ...;), but, anyway, I changed this condition to `if (node.isClassProperty())` and insert `ASSERT(node.needsSuperBinding())` for now.

[Yusuke Suzuki]

Created attachment 289192 [details]
Patch

[Saam Barati]

Comment on attachment 289192 [details]
Patch

r=me

[Yusuke Suzuki]

Comment on attachment 289192 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 289192 [details]
Patch

Clearing flags on attachment: 289192

Committed r206082: <http://trac.webkit.org/changeset/206082>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

According to arewefastyet, it causes some perf regression on kraken-parse-financial.
I'll investigate it.

[Yusuke Suzuki]

Hmmmmmmmmmmmmmmmmmmmmm, I cannot reproduce this regression on my Linux machine.
And I think this patch is not related to this regression (maybe, this regression? can be gone later, I hope), since this code path is not used in kraken-parse-financial.

[Yusuke Suzuki]

The regression is gone.