Bug 202656 - [JSC] GetterSetter should be JSCell, not JSObject
Summary: [JSC] GetterSetter should be JSCell, not JSObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-07 15:38 PDT by Yusuke Suzuki
Modified: 2019-10-14 12:54 PDT (History)
7 users (show)

See Also:

Attachments
WIP (18.15 KB, patch)
2019-10-07 15:42 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (21.04 KB, patch)
2019-10-08 13:54 PDT, Yusuke Suzuki 		saam: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-10-07 15:38:27 PDT 
We already have a good strength-reduction rule to keep JSCell GetterSetter fast. We do not need to make it JSObject.
Comment 1 Yusuke Suzuki 2019-10-07 15:39:02 PDT 
<rdar://problem/55939117>
Comment 2 Yusuke Suzuki 2019-10-07 15:42:16 PDT 
Created attachment 380367 [details]
WIP
Comment 3 Yusuke Suzuki 2019-10-08 13:54:20 PDT 
Created attachment 380461 [details]
Patch
Comment 4 Saam Barati 2019-10-08 14:12:53 PDT 
Comment on attachment 380461 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        2. RHS of that comparison is always folded into constant in DFG.

do we do the right thing even when RHS isn't folded?

> Source/JavaScriptCore/ChangeLog:17
> +        So we already support non-JSString/JSBigInt cell comparison in JSC JS code. We should use it instead of making GetterSetter JSObject.

this support was added after we made GetterSetter a JSObject?
Comment 5 Saam Barati 2019-10-08 14:13:10 PDT 
(In reply to Saam Barati from comment #4)
> Comment on attachment 380461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380461&action=review
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        2. RHS of that comparison is always folded into constant in DFG.
> 
> do we do the right thing even when RHS isn't folded?
> 
> > Source/JavaScriptCore/ChangeLog:17
> > +        So we already support non-JSString/JSBigInt cell comparison in JSC JS code. We should use it instead of making GetterSetter JSObject.
> 
> this support was added after we made GetterSetter a JSObject?

Sorry, I did not mean to clear Tadeu's r+
Comment 6 Saam Barati 2019-10-08 14:25:46 PDT 
Comment on attachment 380461 [details]
Patch

r=me, r=Tadeu
Comment 7 Yusuke Suzuki 2019-10-08 17:18:09 PDT 
Comment on attachment 380461 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:13
>>> +        2. RHS of that comparison is always folded into constant in DFG.
>> 
>> do we do the right thing even when RHS isn't folded?
> 
> Sorry, I did not mean to clear Tadeu's r+

Yes, existing code is doing the right thing.

>> Source/JavaScriptCore/ChangeLog:17
>> +        So we already support non-JSString/JSBigInt cell comparison in JSC JS code. We should use it instead of making GetterSetter JSObject.
> 
> this support was added after we made GetterSetter a JSObject?

Yes, it is added for Map#forEach.
Comment 8 Yusuke Suzuki 2019-10-08 17:25:30 PDT 
Committed r250878: <https://trac.webkit.org/changeset/250878>
Comment 9 Yusuke Suzuki 2019-10-09 14:24:01 PDT 
Committed r250932: <https://trac.webkit.org/changeset/250932>
Comment 10 Yusuke Suzuki 2019-10-14 12:54:34 PDT 
Committed r251088: <https://trac.webkit.org/changeset/251088>