Changeset 241230 in webkit

Timestamp:

Feb 8, 2019 7:56:32 PM (5 years ago)

Author:

keith_miller@apple.com

Message:

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):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGOperations.cpp (modified) (1 diff)
• runtime/CommonSlowPaths.cpp (modified) (1 diff)
• runtime/JSString.h (modified) (1 diff)
• runtime/Operations.cpp (modified) (2 diffs)
• runtime/Operations.h (modified) (6 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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-08  Saam barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGOperations.cpp

@@ -504 +504 @@
     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));
 }
 

trunk/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp

@@ -518 +518 @@
     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, {

trunk/Source/JavaScriptCore/runtime/JSString.h

@@ -487 +487 @@
 
     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&);

trunk/Source/JavaScriptCore/runtime/Operations.cpp

@@ -53 +53 @@
 
     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));
@@ -59 +64 @@
 
     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)));

trunk/Source/JavaScriptCore/runtime/Operations.h

@@ -38 +38 @@
 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)
 {
@@ -55 +99 @@
     }
 
-    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());
 }
 
@@ -80 +131 @@
         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());
 }
 
@@ -110 +171 @@
     }
 
-    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());
 }
 
@@ -326 +390 @@
 //    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)
 {
@@ -331 +414 @@
         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);
 }