Title: [241533] releases/WebKitGTK/webkit-2.24/Source
Revision
241533
Author
carlo...@webkit.org
Date
2019-02-14 03:28:14 -0800 (Thu, 14 Feb 2019)

Log Message

Merge r241493 - We should only make rope strings when concatenating strings long enough.
https://bugs.webkit.org/show_bug.cgi?id=194465

Reviewed by Mark Lam.

Source/_javascript_Core:

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.

The important change from the previous one is that we do not apply
the above rule to JSRopeStrings generated by JSStrings. If we convert
it to JSString, comparison of memory consumption becomes the following,
because JSRopeString does not have StringImpl until it is resolved.

    sizeof(JSRopeString) v.s. sizeof(JSString) + sizeof(StringImpl) + content

Since sizeof(JSString) + sizeof(StringImpl) is larger than sizeof(JSRopeString),
resolving eagerly increases memory footprint. The point is that we need to
account newly created JSString and JSRopeString from the operands. This is the
reason why this patch adds different thresholds for each jsString functions.

This patch also avoids concatenation for ropes conservatively. Many ropes are
temporary cells. So we do not resolve eagerly if one of operands is already a
rope.

In CLI execution, this change is performance neutral in JetStream2 (run 6 times, 1 for warming up and average in latter 5.).

    Before: 159.3778
    After:  160.72340000000003

* dfg/DFGOperations.cpp:
* runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL):
* runtime/JSString.h:
(JSC::JSString::isRope const):
* runtime/Operations.cpp:
(JSC::jsAddSlowCase):
* runtime/Operations.h:
(JSC::jsString):
(JSC::jsAddNonNumber):
(JSC::jsAdd):

Source/WTF:

* wtf/text/StringImpl.h:
(WTF::StringImpl::headerSize):

Modified Paths

Diff

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog (241532 => 241533)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog	2019-02-14 11:28:08 UTC (rev 241532)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog	2019-02-14 11:28:14 UTC (rev 241533)
@@ -1,3 +1,51 @@
+2019-02-13  Keith Miller  <keith_mil...@apple.com> and Yusuke Suzuki  <ysuz...@apple.com>
+
+        We should only make rope strings when concatenating strings long enough.
+        https://bugs.webkit.org/show_bug.cgi?id=194465
+
+        Reviewed by Mark Lam.
+
+        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.
+
+        The important change from the previous one is that we do not apply
+        the above rule to JSRopeStrings generated by JSStrings. If we convert
+        it to JSString, comparison of memory consumption becomes the following,
+        because JSRopeString does not have StringImpl until it is resolved.
+
+            sizeof(JSRopeString) v.s. sizeof(JSString) + sizeof(StringImpl) + content
+
+        Since sizeof(JSString) + sizeof(StringImpl) is larger than sizeof(JSRopeString),
+        resolving eagerly increases memory footprint. The point is that we need to
+        account newly created JSString and JSRopeString from the operands. This is the
+        reason why this patch adds different thresholds for each jsString functions.
+
+        This patch also avoids concatenation for ropes conservatively. Many ropes are
+        temporary cells. So we do not resolve eagerly if one of operands is already a
+        rope.
+
+        In CLI execution, this change is performance neutral in JetStream2 (run 6 times, 1 for warming up and average in latter 5.).
+
+            Before: 159.3778
+            After:  160.72340000000003
+
+        * dfg/DFGOperations.cpp:
+        * runtime/CommonSlowPaths.cpp:
+        (JSC::SLOW_PATH_DECL):
+        * runtime/JSString.h:
+        (JSC::JSString::isRope const):
+        * runtime/Operations.cpp:
+        (JSC::jsAddSlowCase):
+        * runtime/Operations.h:
+        (JSC::jsString):
+        (JSC::jsAddNonNumber):
+        (JSC::jsAdd):
+
 2019-02-13  Saam Barati  <sbar...@apple.com>
 
         AirIRGenerator::addSwitch switch patchpoint needs to model clobbering the scratch register

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGOperations.cpp (241532 => 241533)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGOperations.cpp	2019-02-14 11:28:08 UTC (rev 241532)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/dfg/DFGOperations.cpp	2019-02-14 11:28:14 UTC (rev 241533)
@@ -503,12 +503,7 @@
     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)

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/CommonSlowPaths.cpp (241532 => 241533)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/CommonSlowPaths.cpp	2019-02-14 11:28:08 UTC (rev 241532)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/CommonSlowPaths.cpp	2019-02-14 11:28:14 UTC (rev 241533)
@@ -517,19 +517,11 @@
     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);

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSString.h (241532 => 241533)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSString.h	2019-02-14 11:28:08 UTC (rev 241532)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/JSString.h	2019-02-14 11:28:14 UTC (rev 241533)
@@ -196,11 +196,12 @@
         Is8Bit = 1u
     };
 
+    bool isRope() const { return m_value.isNull(); }
+
 protected:
     friend class JSValue;
 
     JS_EXPORT_PRIVATE bool equalSlowCase(ExecState*, JSString* other) const;
-    bool isRope() const { return m_value.isNull(); }
     bool isSubstring() const;
     bool is8Bit() const { return m_flags & Is8Bit; }
     void setIs8Bit(bool flag) const
@@ -486,7 +487,10 @@
 
 
     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&);
     friend JSString* jsString(ExecState*, const String&, const String&, const String&);
 };
 

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/Operations.cpp (241532 => 241533)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/Operations.cpp	2019-02-14 11:28:08 UTC (rev 241532)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/Operations.cpp	2019-02-14 11:28:14 UTC (rev 241533)
@@ -52,13 +52,23 @@
     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 (p1.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)));
     }

Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/Operations.h (241532 => 241533)


--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/Operations.h	2019-02-14 11:28:08 UTC (rev 241532)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/runtime/Operations.h	2019-02-14 11:28:14 UTC (rev 241533)
@@ -37,6 +37,75 @@
 bool jsIsObjectTypeOrNull(CallFrame*, JSValue);
 size_t normalizePrototypeChain(CallFrame*, JSCell*, bool& sawPolyProto);
 
+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);
+    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
+    if (sumOverflows<int32_t>(length1, length2)) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
+
+    // (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + length1 + length2
+    // (2) Cost of making JSRopeString: sizeof(JSString) (for u1) + sizeof(JSRopeString)
+    // We do not account u1 cost in (2) since u1 may be shared StringImpl, and it may not introduce additional cost.
+    // We conservatively consider the cost of u1. Currently, we are not considering about is8Bit() case because 16-bit
+    // strings are relatively rare. But we can do that if we need to consider it.
+    if (s2->isRope() || (StringImpl::headerSize<LChar>() + length1 + length2) >= sizeof(JSRopeString))
+        return JSRopeString::create(vm, jsString(&vm, u1), s2);
+
+    ASSERT(!s2->isRope());
+    const String& u2 = s2->value(exec);
+    scope.assertNoException();
+    String newString = tryMakeString(u1, u2);
+    if (!newString) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
+    return JSString::create(vm, newString.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;
+    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
+    if (sumOverflows<int32_t>(length1, length2)) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
+
+    // (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + length1 + length2
+    // (2) Cost of making JSRopeString: sizeof(JSString) (for u2) + sizeof(JSRopeString)
+    if (s1->isRope() || (StringImpl::headerSize<LChar>() + length1 + length2) >= sizeof(JSRopeString))
+        return JSRopeString::create(vm, s1, jsString(&vm, u2));
+
+    ASSERT(!s1->isRope());
+    const String& u1 = s1->value(exec);
+    scope.assertNoException();
+    String newString = tryMakeString(u1, u2);
+    if (!newString) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
+    return JSString::create(vm, newString.releaseImpl().releaseNonNull());
+}
+
 ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2)
 {
     VM& vm = exec->vm();
@@ -79,9 +148,40 @@
         throwOutOfMemoryError(exec, scope);
         return nullptr;
     }
+
     return JSRopeString::create(vm, s1, s2, s3);
 }
 
+ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String& u2)
+{
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
+    unsigned length1 = u1.length();
+    if (!length1)
+        return jsString(&vm, u2);
+    unsigned length2 = u2.length();
+    if (!length2)
+        return jsString(&vm, u1);
+    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
+    if (sumOverflows<int32_t>(length1, length2)) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
+
+    // (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + length1 + length2
+    // (2) Cost of making JSRopeString: sizeof(JSString) (for u1) + sizeof(JSString) (for u2) + sizeof(JSRopeString)
+    if ((StringImpl::headerSize<LChar>() + length1 + length2) >= (sizeof(JSRopeString) + sizeof(JSString)))
+        return JSRopeString::create(vm, jsString(&vm, u1), jsString(&vm, u2));
+
+    String newString = tryMakeString(u1, u2);
+    if (!newString) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
+    return JSString::create(vm, newString.releaseImpl().releaseNonNull());
+}
+
 ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String& u2, const String& u3)
 {
     VM* vm = &exec->vm();
@@ -95,13 +195,13 @@
     ASSERT(length3 <= JSString::MaxLength);
 
     if (!length1)
-        RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u2), jsString(vm, u3)));
+        RELEASE_AND_RETURN(scope, jsString(exec, u2, u3));
 
     if (!length2)
-        RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u1), jsString(vm, u3)));
+        RELEASE_AND_RETURN(scope, jsString(exec, u1, u3));
 
     if (!length3)
-        RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u1), jsString(vm, u2)));
+        RELEASE_AND_RETURN(scope, jsString(exec, u1, u2));
 
     static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
     if (sumOverflows<int32_t>(length1, length2, length3)) {
@@ -109,7 +209,17 @@
         return nullptr;
     }
 
-    return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
+    // (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + length1 + length2 + length3
+    // (2) Cost of making JSRopeString: sizeof(JSString) (for u1) + sizeof(JSString) (for u2) + sizeof(JSString) (for u3) + sizeof(JSRopeString)
+    if ((StringImpl::headerSize<LChar>() + length1 + length2 + length3) >= (sizeof(JSRopeString) + sizeof(JSString) * 2))
+        return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
+
+    String newString = tryMakeString(u1, u2, u3);
+    if (!newString) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
+    return JSString::create(*vm, newString.releaseImpl().releaseNonNull());
 }
 
 ALWAYS_INLINE JSValue jsStringFromRegisterArray(ExecState* exec, Register* strings, unsigned count)
@@ -325,16 +435,31 @@
 //    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 (LIKELY(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)

Modified: releases/WebKitGTK/webkit-2.24/Source/WTF/ChangeLog (241532 => 241533)


--- releases/WebKitGTK/webkit-2.24/Source/WTF/ChangeLog	2019-02-14 11:28:08 UTC (rev 241532)
+++ releases/WebKitGTK/webkit-2.24/Source/WTF/ChangeLog	2019-02-14 11:28:14 UTC (rev 241533)
@@ -1,3 +1,13 @@
+2019-02-13  Yusuke Suzuki  <ysuz...@apple.com>
+
+        We should only make rope strings when concatenating strings long enough.
+        https://bugs.webkit.org/show_bug.cgi?id=194465
+
+        Reviewed by Mark Lam.
+
+        * wtf/text/StringImpl.h:
+        (WTF::StringImpl::headerSize):
+
 2019-02-11  Myles C. Maxfield  <mmaxfi...@apple.com>
 
         [Cocoa] Ask platform for generic font family mappings

Modified: releases/WebKitGTK/webkit-2.24/Source/WTF/wtf/text/StringImpl.h (241532 => 241533)


--- releases/WebKitGTK/webkit-2.24/Source/WTF/wtf/text/StringImpl.h	2019-02-14 11:28:08 UTC (rev 241532)
+++ releases/WebKitGTK/webkit-2.24/Source/WTF/wtf/text/StringImpl.h	2019-02-14 11:28:14 UTC (rev 241533)
@@ -485,6 +485,8 @@
 #endif
 
     BufferOwnership bufferOwnership() const { return static_cast<BufferOwnership>(m_hashAndFlags & s_hashMaskBufferOwnership); }
+
+    template<typename T> static size_t headerSize() { return tailOffset<T>(); }
     
 protected:
     ~StringImpl();
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to