Bug 185065 - [JSC] Add SameValue DFG node
Summary: [JSC] Add SameValue DFG node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari 11
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-26 20:04 PDT by goktug
Modified: 2020-04-17 09:01 PDT (History)
8 users (show)

See Also:


Attachments
Patch (42.37 KB, patch)
2018-04-28 16:35 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (47.37 KB, patch)
2018-04-28 18:52 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (47.37 KB, patch)
2018-04-28 18:54 PDT, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description goktug 2018-04-26 20:04:20 PDT
It seems like Object.is has very poor performance; even polyfill performs much better than the built-in method (2x). Also for comparison "===" performs ~10x.

See the following benchmarks:
https://jsperf.com/string-object-is-performance
https://jsperf.com/object-is-vs-polyfill

You can see similar issue fixed for V8 here:
https://bugs.chromium.org/p/v8/issues/detail?id=7007
Comment 1 Radar WebKit Bug Importer 2018-04-26 20:37:39 PDT
<rdar://problem/39781064>
Comment 2 Yusuke Suzuki 2018-04-28 16:35:38 PDT
Created attachment 339081 [details]
Patch

WIP: 64bit works.
Comment 3 Yusuke Suzuki 2018-04-28 18:52:44 PDT
Created attachment 339084 [details]
Patch
Comment 4 Yusuke Suzuki 2018-04-28 18:54:12 PDT
Created attachment 339085 [details]
Patch
Comment 5 Yusuke Suzuki 2018-05-01 20:30:17 PDT
Ping?
Comment 6 Saam Barati 2018-05-01 21:53:35 PDT
Comment on attachment 339085 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2643
> +        if (argumentCountIncludingThis != 3)

Why not < instead of !=?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6383
> +        m_jit.compareDouble(CCallHelpers::DoubleNotEqualOrUnordered, arg1FPR, arg1FPR, tempGPR);

Since this code branches internally, it feels like we should only do it if the doubles aren’t bitwise equal
Comment 7 Yusuke Suzuki 2018-05-01 22:49:44 PDT
Comment on attachment 339085 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2643
>> +        if (argumentCountIncludingThis != 3)
> 
> Why not < instead of !=?

Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6383
>> +        m_jit.compareDouble(CCallHelpers::DoubleNotEqualOrUnordered, arg1FPR, arg1FPR, tempGPR);
> 
> Since this code branches internally, it feels like we should only do it if the doubles aren’t bitwise equal

OK, fixed.
Comment 8 Yusuke Suzuki 2018-05-01 22:51:39 PDT
Committed r231224: <https://trac.webkit.org/changeset/231224>
Comment 9 Ryan Haddad 2018-05-02 09:16:58 PDT
The test added with this change is failing on the 32-bit JSC bot:

stress/object-is.js.default: Exception: Error: bad value: true
stress/object-is.js.default: shouldBe@object-is.js:3:24
stress/object-is.js.default: global code@object-is.js:67:13
stress/object-is.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/object-is.js.default

https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1808
Comment 10 Yusuke Suzuki 2018-05-02 17:42:17 PDT
Committed r231284: <https://trac.webkit.org/changeset/231284>