Attachment 361586 Details for Bug 194465 – Patch for landing

[patch] Patch for landing

bug-194465-20190208191910.patch (text/plain), 10.70 KB, created by Keith Miller on 2019-02-08 19:19:11 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2019-02-08 19:19:11 PST

Size: 10.70 KB

patch

obsolete

>Subversion Revision: 241104
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 74bab6fa5c5ed54946a8b008305d26c175c8316f..406e72155aead1fb0d72ddb9f679d9d13e02825a 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,28 @@
>+2019-02-08  Keith Miller  <keith_miller@apple.com>
>+
>+        We should only make rope strings when concatenating strings long enough.
>+        https://bugs.webkit.org/show_bug.cgi?id=194465
>+
>+        Reviewed by Saam Barati.
>+
>+        This patch stops us from allocating a rope string if the resulting
>+        rope would be smaller than the size of the JSRopeString object we
>+        would need to allocate.
>+
>+        This patch also adds paths so that we don't unnecessarily allocate
>+        JSString cells for primitives we are going to concatenate with a
>+        string anyway.
>+
>+        * dfg/DFGOperations.cpp:
>+        * runtime/CommonSlowPaths.cpp:
>+        (JSC::SLOW_PATH_DECL):
>+        * runtime/JSString.h:
>+        * runtime/Operations.cpp:
>+        (JSC::jsAddSlowCase):
>+        * runtime/Operations.h:
>+        (JSC::jsString):
>+        (JSC::jsAdd):
>+
> 2019-02-06  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] PrivateName to PublicName hash table is wasteful
>diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>index 22750b7997af9c9a6b0eea3a5acfced1458c7f2e..0fdf3dbb327611c9bebe02d8abcfd565c07a079d 100644
>--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>@@ -503,12 +503,7 @@ EncodedJSValue JIT_OPERATION operationValueAddNotNumber(ExecState* exec, Encoded
>     JSValue op1 = JSValue::decode(encodedOp1);
>     JSValue op2 = JSValue::decode(encodedOp2);
>     
>-    ASSERT(!op1.isNumber() || !op2.isNumber());
>-    
>-    if (op1.isString() && !op2.isObject())
>-        return JSValue::encode(jsString(exec, asString(op1), op2.toString(exec)));
>-
>-    return JSValue::encode(jsAddSlowCase(exec, op1, op2));
>+    return JSValue::encode(jsAddNonNumber(exec, op1, op2));
> }
> 
> EncodedJSValue JIT_OPERATION operationValueDiv(ExecState* exec, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2)
>diff --git a/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp b/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
>index bbbadab67fdf6fcad4cac0664cdc8e60bf5e4b2e..5871c930bbc8e06636110ea4c6769b202340751d 100644
>--- a/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
>+++ b/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
>@@ -517,19 +517,11 @@ SLOW_PATH_DECL(slow_path_add)
>     auto bytecode = pc->as<OpAdd>();
>     JSValue v1 = GET_C(bytecode.m_lhs).jsValue();
>     JSValue v2 = GET_C(bytecode.m_rhs).jsValue();
>-    JSValue result;
> 
>     ArithProfile& arithProfile = *exec->codeBlock()->arithProfileForPC(pc);
>     arithProfile.observeLHSAndRHS(v1, v2);
> 
>-    if (v1.isString() && !v2.isObject()) {
>-        JSString* v2String = v2.toString(exec);
>-        if (LIKELY(!throwScope.exception()))
>-            result = jsString(exec, asString(v1), v2String);
>-    } else if (v1.isNumber() && v2.isNumber())
>-        result = jsNumber(v1.asNumber() + v2.asNumber());
>-    else
>-        result = jsAddSlowCase(exec, v1, v2);
>+    JSValue result = jsAdd(exec, v1, v2);
> 
>     RETURN_WITH_PROFILING(result, {
>         updateArithProfileForBinaryArithOp(exec, pc, result, v1, v2);
>diff --git a/Source/JavaScriptCore/runtime/JSString.h b/Source/JavaScriptCore/runtime/JSString.h
>index 8b4d2091364236480ee62748b2706f1d3239b11d..112283f80bcfc138c3ac9b12d21342850cc55a77 100644
>--- a/Source/JavaScriptCore/runtime/JSString.h
>+++ b/Source/JavaScriptCore/runtime/JSString.h
>@@ -486,6 +486,8 @@ private:
> 
> 
>     friend JSString* jsString(ExecState*, JSString*, JSString*);
>+    friend JSString* jsString(ExecState*, const String&, JSString*);
>+    friend JSString* jsString(ExecState*, JSString*, const String&);
>     friend JSString* jsString(ExecState*, JSString*, JSString*, JSString*);
>     friend JSString* jsString(ExecState*, const String&, const String&, const String&);
> };
>diff --git a/Source/JavaScriptCore/runtime/Operations.cpp b/Source/JavaScriptCore/runtime/Operations.cpp
>index 836f953c3a87c57d5c622fbf4c70509050751beb..a5310b346d89945bea295a1623d70a5707fcb93a 100644
>--- a/Source/JavaScriptCore/runtime/Operations.cpp
>+++ b/Source/JavaScriptCore/runtime/Operations.cpp
>@@ -52,13 +52,23 @@ NEVER_INLINE JSValue jsAddSlowCase(CallFrame* callFrame, JSValue v1, JSValue v2)
>     RETURN_IF_EXCEPTION(scope, { });
> 
>     if (p1.isString()) {
>-        JSString* p2String = p2.toString(callFrame);
>+        if (p2.isCell()) {
>+            JSString* p2String = p2.toString(callFrame);
>+            RETURN_IF_EXCEPTION(scope, { });
>+            RELEASE_AND_RETURN(scope, jsString(callFrame, asString(p1), p2String));
>+        }
>+        String p2String = p2.toWTFString(callFrame);
>         RETURN_IF_EXCEPTION(scope, { });
>         RELEASE_AND_RETURN(scope, jsString(callFrame, asString(p1), p2String));
>     }
> 
>     if (p2.isString()) {
>-        JSString* p1String = p1.toString(callFrame);
>+        if (p2.isCell()) {
>+            JSString* p1String = p1.toString(callFrame);
>+            RETURN_IF_EXCEPTION(scope, { });
>+            RELEASE_AND_RETURN(scope, jsString(callFrame, p1String, asString(p2)));
>+        }
>+        String p1String = p1.toWTFString(callFrame);
>         RETURN_IF_EXCEPTION(scope, { });
>         RELEASE_AND_RETURN(scope, jsString(callFrame, p1String, asString(p2)));
>     }
>diff --git a/Source/JavaScriptCore/runtime/Operations.h b/Source/JavaScriptCore/runtime/Operations.h
>index 0ea98291bb68873bb3bab10461861e9158a87bd4..a34eea244564cfe26b9fbc1cdfd1fbcf6707f765 100644
>--- a/Source/JavaScriptCore/runtime/Operations.h
>+++ b/Source/JavaScriptCore/runtime/Operations.h
>@@ -37,6 +37,50 @@ JSValue jsTypeStringForValue(VM&, JSGlobalObject*, JSValue);
> bool jsIsObjectTypeOrNull(CallFrame*, JSValue);
> size_t normalizePrototypeChain(CallFrame*, JSCell*, bool& sawPolyProto);
> 
>+// This is a lower bound on the extra memory we expect to write if we were to allocate a rope instead of copying.
>+// Note, it doesn't differentiate between 8 and 16-bit strings because 16-bit strings are relatively rare.
>+constexpr unsigned minRopeStringLength = sizeof(JSRopeString);
>+
>+ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, JSString* s2)
>+{
>+    VM& vm = exec->vm();
>+    auto scope = DECLARE_THROW_SCOPE(vm);
>+
>+    unsigned length1 = u1.length();
>+    if (!length1)
>+        return s2;
>+    unsigned length2 = s2->length();
>+    if (!length2)
>+        return jsString(&vm, u1);
>+
>+    if (length1 + length2 >= minRopeStringLength)
>+        return jsString(exec, jsString(&vm, u1), s2);
>+
>+    const String& u2 = s2->value(exec);
>+    RETURN_IF_EXCEPTION(scope, { });
>+    return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull());
>+}
>+
>+ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, const String& u2)
>+{
>+    VM& vm = exec->vm();
>+    auto scope = DECLARE_THROW_SCOPE(vm);
>+
>+    unsigned length1 = s1->length();
>+    if (!length1)
>+        return jsString(&vm, u2);
>+    unsigned length2 = u2.length();
>+    if (!length2)
>+        return s1;
>+
>+    if (length1 + length2 >= minRopeStringLength)
>+        return jsString(exec, s1, jsString(&vm, u2));
>+
>+    const String& u1 = s1->value(exec);
>+    RETURN_IF_EXCEPTION(scope, { });
>+    return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull());
>+}
>+
> ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2)
> {
>     VM& vm = exec->vm();
>@@ -54,7 +98,14 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2)
>         return nullptr;
>     }
> 
>-    return JSRopeString::create(vm, s1, s2);
>+    if (length1 + length2 >= minRopeStringLength)
>+        return JSRopeString::create(vm, s1, s2);
>+
>+    const String& u1 = s1->value(exec);
>+    RETURN_IF_EXCEPTION(scope, { });
>+    const String& u2 = s2->value(exec);
>+    RETURN_IF_EXCEPTION(scope, { });
>+    return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull());
> }
> 
> ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2, JSString* s3)
>@@ -79,7 +130,17 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2, JS
>         throwOutOfMemoryError(exec, scope);
>         return nullptr;
>     }
>-    return JSRopeString::create(vm, s1, s2, s3);
>+
>+    if (length1 + length2 + length3 >= minRopeStringLength)
>+        return JSRopeString::create(vm, s1, s2, s3);
>+
>+    const String& u1 = s1->value(exec);
>+    RETURN_IF_EXCEPTION(scope, { });
>+    const String& u2 = s2->value(exec);
>+    RETURN_IF_EXCEPTION(scope, { });
>+    const String& u3 = s3->value(exec);
>+    RETURN_IF_EXCEPTION(scope, { });
>+    return JSString::create(vm, makeString(u1, u2, u3).releaseImpl().releaseNonNull());
> }
> 
> ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String& u2, const String& u3)
>@@ -109,7 +170,10 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String
>         return nullptr;
>     }
> 
>-    return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
>+    if (length1 + length2 + length3 >= minRopeStringLength)
>+        return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
>+
>+    return JSString::create(*vm, makeString(u1, u2, u3).releaseImpl().releaseNonNull());
> }
> 
> ALWAYS_INLINE JSValue jsStringFromRegisterArray(ExecState* exec, Register* strings, unsigned count)
>@@ -325,16 +389,31 @@ ALWAYS_INLINE bool jsLessEq(CallFrame* callFrame, JSValue v1, JSValue v2)
> //    13962   Add case: 5 3
> //    4000    Add case: 3 5
> 
>+
>+ALWAYS_INLINE JSValue jsAddNonNumber(CallFrame* callFrame, JSValue v1, JSValue v2)
>+{
>+    VM& vm = callFrame->vm();
>+    auto scope = DECLARE_THROW_SCOPE(vm);
>+    ASSERT(!v1.isNumber() || !v2.isNumber());
>+
>+    if (v1.isString() && !v2.isObject()) {
>+        if (v2.isString())
>+            RELEASE_AND_RETURN(scope, jsString(callFrame, asString(v1), asString(v2)));
>+        String s2 = v2.toWTFString(callFrame);
>+        RETURN_IF_EXCEPTION(scope, { });
>+        RELEASE_AND_RETURN(scope, jsString(callFrame, asString(v1), s2));
>+    }
>+
>+    // All other cases are pretty uncommon
>+    RELEASE_AND_RETURN(scope, jsAddSlowCase(callFrame, v1, v2));
>+}
>+
> ALWAYS_INLINE JSValue jsAdd(CallFrame* callFrame, JSValue v1, JSValue v2)
> {
>     if (v1.isNumber() && v2.isNumber())
>         return jsNumber(v1.asNumber() + v2.asNumber());
>         
>-    if (v1.isString() && !v2.isObject())
>-        return jsString(callFrame, asString(v1), v2.toString(callFrame));
>-
>-    // All other cases are pretty uncommon
>-    return jsAddSlowCase(callFrame, v1, v2);
>+    return jsAddNonNumber(callFrame, v1, v2);
> }
> 
> ALWAYS_INLINE JSValue jsSub(ExecState* exec, JSValue v1, JSValue v2)

Actions: View | Formatted Diff | Diff

Attachments on bug 194465: 361564 | 361568 | 361573 | 361578 | 361581 | 361584 | 361585 | 361586 | 361890 | 361891