Bug 162111 - [JSC] Do not need to use defineProperty to define methods for object literals
Summary: [JSC] Do not need to use defineProperty to define methods for object literals
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:
Depends on:
Blocks: 162108
  Show dependency treegraph
 
Reported: 2016-09-16 20:18 PDT by Yusuke Suzuki
Modified: 2016-09-19 14:54 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.71 KB, patch)
2016-09-16 21:10 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (7.67 KB, patch)
2016-09-17 22:39 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 2016-09-16 20:18:13 PDT
[JSC] Do not need to use defineProperty to define methods for object literals
Comment 1 Yusuke Suzuki 2016-09-16 21:10:29 PDT
Created attachment 289159 [details]
Patch
Comment 2 Saam Barati 2016-09-17 16:50:37 PDT
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?
Comment 3 Yusuke Suzuki 2016-09-17 22:36:44 PDT
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.
Comment 4 Yusuke Suzuki 2016-09-17 22:39:27 PDT
Created attachment 289192 [details]
Patch
Comment 5 Saam Barati 2016-09-18 10:20:32 PDT
Comment on attachment 289192 [details]
Patch

r=me
Comment 6 Yusuke Suzuki 2016-09-18 10:38:55 PDT
Comment on attachment 289192 [details]
Patch

Thanks!
Comment 7 WebKit Commit Bot 2016-09-18 10:42:23 PDT
Comment on attachment 289192 [details]
Patch

Clearing flags on attachment: 289192

Committed r206082: <http://trac.webkit.org/changeset/206082>
Comment 8 WebKit Commit Bot 2016-09-18 10:42:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Yusuke Suzuki 2016-09-18 23:24:19 PDT
According to arewefastyet, it causes some perf regression on kraken-parse-financial.
I'll investigate it.
Comment 10 Yusuke Suzuki 2016-09-18 23:43:26 PDT
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.
Comment 11 Yusuke Suzuki 2016-09-19 14:54:29 PDT
The regression is gone.