Attachment 368498 Details for Bug 197362 – Patch

[patch] Patch

bug-197362-20190429151524.patch (text/plain), 7.48 KB, created by Yusuke Suzuki on 2019-04-29 15:15:24 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-04-29 15:15:24 PDT

Size: 7.48 KB

patch

obsolete

>Subversion Revision: 244754
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 38ccae2b96ef1b467fced432d5b2ea0043e7adae..3561647fbf460a3a54e90fde4f626c7c58aac33f 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,22 @@
>+2019-04-29  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        normalizeMapKey should normalize NaN to one PureNaN bit pattern to make MapHash same
>+        https://bugs.webkit.org/show_bug.cgi?id=197362
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Our Map/Set's hash algorithm relies on the bit pattern of JSValue. So our Map/Set has
>+        normalization of the key, which normalizes Int32 / Double etc. But we did not normalize
>+        pure NaNs into one canonicalized pure NaN. So we end up having multiple different pure NaNs
>+        in one Map/Set. This patch normalizes NaN into one jsNaN(), which uses PNaN for the representation.
>+
>+        * dfg/DFGSpeculativeJIT.cpp:
>+        (JSC::DFG::SpeculativeJIT::compileNormalizeMapKey):
>+        * ftl/FTLLowerDFGToB3.cpp:
>+        (JSC::FTL::DFG::LowerDFGToB3::compileNormalizeMapKey):
>+        * runtime/HashMapImpl.h:
>+        (JSC::normalizeMapKey):
>+
> 2019-04-29  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         JITStubRoutineSet wastes 180KB of HashTable capacity on can.com
>diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>index 7111acf3eab52c005204916b90ba24302600b484..3c0390725e83572eba1b6297603f0c30f583d917 100644
>--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>@@ -11714,6 +11714,7 @@ void SpeculativeJIT::compileNormalizeMapKey(Node* node)
>     FPRReg tempFPR = temp.fpr();
> 
>     CCallHelpers::JumpList passThroughCases;
>+    CCallHelpers::JumpList doneCases;
> 
>     passThroughCases.append(m_jit.branchIfNotNumber(keyRegs, scratchGPR));
>     passThroughCases.append(m_jit.branchIfInt32(keyRegs));
>@@ -11723,19 +11724,22 @@ void SpeculativeJIT::compileNormalizeMapKey(Node* node)
> #else
>     unboxDouble(keyRegs.tagGPR(), keyRegs.payloadGPR(), doubleValueFPR, tempFPR);
> #endif
>-    passThroughCases.append(m_jit.branchIfNaN(doubleValueFPR));
>+    auto notNaN = m_jit.branchIfNotNaN(doubleValueFPR);
>+    m_jit.moveTrustedValue(jsNaN(), resultRegs);
>+    doneCases.append(m_jit.jump());
> 
>+    notNaN.link(&m_jit);
>     m_jit.truncateDoubleToInt32(doubleValueFPR, scratchGPR);
>     m_jit.convertInt32ToDouble(scratchGPR, tempFPR);
>     passThroughCases.append(m_jit.branchDouble(JITCompiler::DoubleNotEqual, doubleValueFPR, tempFPR));
> 
>     m_jit.boxInt32(scratchGPR, resultRegs);
>-    auto done = m_jit.jump();
>+    doneCases.append(m_jit.jump());
> 
>     passThroughCases.link(&m_jit);
>     m_jit.moveValueRegs(keyRegs, resultRegs);
> 
>-    done.link(&m_jit);
>+    doneCases.link(&m_jit);
>     jsValueResult(resultRegs, node);
> }
> 
>diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>index 62a6d334c3111bdb049660800c21bc905bdc90bb..9c9903de38142031649f09fe90a4ccfd31b597be 100644
>--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>@@ -9822,6 +9822,7 @@ class LowerDFGToB3 {
> 
>         m_out.appendTo(notInt32NumberCase, notNaNCase);
>         LValue doubleValue = unboxDouble(key);
>+        ValueFromBlock normalizedNaNResult = m_out.anchor(m_out.constInt64(JSValue::encode(jsNaN())));
>         m_out.branch(m_out.doubleNotEqualOrUnordered(doubleValue, doubleValue), unsure(continuation), unsure(notNaNCase));
> 
>         m_out.appendTo(notNaNCase, convertibleCase);
>@@ -9830,11 +9831,11 @@ class LowerDFGToB3 {
>         m_out.branch(m_out.doubleNotEqualOrUnordered(doubleValue, integerValueConvertedToDouble), unsure(continuation), unsure(convertibleCase));
> 
>         m_out.appendTo(convertibleCase, continuation);
>-        ValueFromBlock slowResult = m_out.anchor(boxInt32(integerValue));
>+        ValueFromBlock boxedIntResult = m_out.anchor(boxInt32(integerValue));
>         m_out.jump(continuation);
> 
>         m_out.appendTo(continuation, lastNext);
>-        setJSValue(m_out.phi(Int64, fastResult, slowResult));
>+        setJSValue(m_out.phi(Int64, fastResult, normalizedNaNResult, boxedIntResult));
>     }
> 
>     void compileGetMapBucket()
>diff --git a/Source/JavaScriptCore/runtime/HashMapImpl.h b/Source/JavaScriptCore/runtime/HashMapImpl.h
>index 4e87c892f91dd62f6d634ee312302d2b32e4d64e..8dea68f1e90c94a5422ab25e295b47a746a28f52 100644
>--- a/Source/JavaScriptCore/runtime/HashMapImpl.h
>+++ b/Source/JavaScriptCore/runtime/HashMapImpl.h
>@@ -26,6 +26,7 @@
> #pragma once
> 
> #include "ExceptionHelpers.h"
>+#include "JSCJSValueInlines.h"
> #include "JSObject.h"
> 
> namespace JSC {
>@@ -245,7 +246,7 @@ ALWAYS_INLINE JSValue normalizeMapKey(JSValue key)
> 
>     double d = key.asDouble();
>     if (std::isnan(d))
>-        return key;
>+        return jsNaN();
> 
>     int i = static_cast<int>(d);
>     if (i == d) {
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 316b88eff9ac4d77207826aebd8b9ab4a2d4c322..32baeabd5f9b34a4210f84a28cea982200115264 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,24 @@
>+2019-04-29  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        normalizeMapKey should normalize NaN to one PureNaN bit pattern to make MapHash same
>+        https://bugs.webkit.org/show_bug.cgi?id=197362
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/hash-table-with-normalized-nan.js: Added.
>+        (shouldBe):
>+        (div):
>+        (NaN1):
>+        (NaN2):
>+        (NaN3):
>+        (NaN4):
>+        (NaN1NoInline):
>+        (NaN2NoInline):
>+        (NaN3NoInline):
>+        (NaN4NoInline):
>+        (test2):
>+        (test4):
>+
> 2019-04-26  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r244708.
>diff --git a/JSTests/stress/hash-table-with-normalized-nan.js b/JSTests/stress/hash-table-with-normalized-nan.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..77698ea3e92fd694b6fb841d2b6cf65bf7205461
>--- /dev/null
>+++ b/JSTests/stress/hash-table-with-normalized-nan.js
>@@ -0,0 +1,81 @@
>+function shouldBe(actual, expected) {
>+    if (actual !== expected)
>+        throw new Error('bad value: ' + actual);
>+}
>+noInline(shouldBe);
>+
>+function div(a, b) {
>+    return a / b;
>+}
>+noInline(div);
>+
>+function NaN1() {
>+    return div(0, 0);
>+}
>+function NaN2() {
>+    return NaN;
>+}
>+function NaN3() {
>+    return Infinity/Infinity;
>+}
>+function NaN4() {
>+    return NaN + NaN;
>+}
>+
>+function NaN1NoInline() {
>+    return div(0, 0);
>+}
>+noInline(NaN1NoInline);
>+function NaN2NoInline() {
>+    return NaN;
>+}
>+noInline(NaN2NoInline);
>+function NaN3NoInline() {
>+    return Infinity/Infinity;
>+}
>+noInline(NaN3NoInline);
>+function NaN4NoInline() {
>+    return NaN + NaN;
>+}
>+noInline(NaN4NoInline);
>+
>+function test1()
>+{
>+    var set = new Set();
>+    set.add(NaN1());
>+    set.add(NaN2());
>+    set.add(NaN3());
>+    set.add(NaN4());
>+    return set.size;
>+}
>+noInline(test1);
>+
>+function test2()
>+{
>+    return new Set([NaN1(), NaN2(), NaN3(), NaN4()]).size;
>+}
>+noInline(test2);
>+
>+function test3()
>+{
>+    var set = new Set();
>+    set.add(NaN1NoInline());
>+    set.add(NaN2NoInline());
>+    set.add(NaN3NoInline());
>+    set.add(NaN4NoInline());
>+    return set.size;
>+}
>+noInline(test3);
>+
>+function test4()
>+{
>+    return new Set([NaN1NoInline(), NaN2NoInline(), NaN3NoInline(), NaN4NoInline()]).size;
>+}
>+noInline(test4);
>+
>+for (var i = 0; i < 1e5; ++i) {
>+    shouldBe(test1(), 1);
>+    shouldBe(test2(), 1);
>+    shouldBe(test3(), 1);
>+    shouldBe(test4(), 1);
>+}

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197362: 368446 | 368498