Attachment 361890 Details for Bug 194465 – Patch

[patch] Patch

bug-194465-20190212212347.patch (text/plain), 13.04 KB, created by Yusuke Suzuki on 2019-02-12 21:23:48 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-02-12 21:23:48 PST

Size: 13.04 KB

patch

obsolete

>Subversion Revision: 241311
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 755147932328300b3f9b3b3ea45080d5a1396254..a6052dabef666d1e9a4825650245b86f88bd0e1e 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,52 @@
>+2019-02-12  Keith Miller  <keith_miller@apple.com> and Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        We should only make rope strings when concatenating strings long enough.
>+        https://bugs.webkit.org/show_bug.cgi?id=194465
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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.
>+
>+        In CLI execution, this change is performance neutral in JetStream2 (average in 6 runs on MBP).
>+
>+            Before: 160.76083333333332
>+            After:  160.68233333333333
>+
>+        And we can get 1.7% improvement in RAMification (average in 6 runs on MBP).
>+
>+            Before: 35183395.333333336
>+            After:  34590661.166666664
>+
>+        * 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-12  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize
>diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>index 984fc0b961e500954d6df0c4a9d3a677749eaaff..6a578266eb4a128327e53cdaacb7d861577c49ef 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 00afd41432204d0042ba7a95390641d17bddcc92..948a97096d9c0346bbf35291a9005ad61cbda9b6 100644
>--- a/Source/JavaScriptCore/runtime/JSString.h
>+++ b/Source/JavaScriptCore/runtime/JSString.h
>@@ -196,11 +196,12 @@ class JSString : public JSCell {
>         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 @@ class JSRopeString final : public JSString {
> 
> 
>     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&);
> };
> 
>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..7e6aeef795f3cc997d4605104014e174c5c68187 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);
> 
>+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);
>+
>+    // JSRopeString + JSString v.s. JSString
>+    if (s2->isRope() || (length1 + length2) >= sizeof(JSRopeString))
>+        return jsString(exec, jsString(&vm, u1), s2);
>+
>+    const String& u2 = s2->value(exec);
>+    UNUSED_PARAM(u2);
>+    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;
>+
>+    // JSRopeString + JSString v.s. JSString
>+    if (s1->isRope() || (length1 + length2) >= sizeof(JSRopeString))
>+        return jsString(exec, s1, jsString(&vm, u2));
>+
>+    const String& u1 = s1->value(exec);
>+    UNUSED_PARAM(u1);
>+    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();
>@@ -79,9 +123,34 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2, JS
>         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;
>+    }
>+
>+    // JSRopeString + JSString * 2 v.s. JSString
>+    if (length1 + length2 >= (sizeof(JSRopeString) + sizeof(JSString)))
>+        return JSRopeString::create(vm, jsString(&vm, u1), jsString(&vm, u2));
>+
>+    return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull());
>+}
>+
> ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String& u2, const String& u3)
> {
>     VM* vm = &exec->vm();
>@@ -95,13 +164,13 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String
>     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 +178,11 @@ 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));
>+    // JSRopeString + JSString * 3 v.s. JSString
>+    if (length1 + length2 + length3 >= (sizeof(JSRopeString) + sizeof(JSString) * 2))
>+        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 +398,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 (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)

Actions: View | Formatted Diff | Diff

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