[JSC] Use the generic path in jsSpliceSubstringsWithSeparator(s) for short two-range results

https://bugs.webkit.org/show_bug.cgi?id=319023

Reviewed by Yusuke Suzuki.

For a global RegExp replace with a single match (rangeCount == 2, separatorCount == 1),
we create two substring StringImpls and concatenate them with jsString(). But when the
result is short, jsString() flattens it via tryMakeString() anyway, so this costs three
allocations and a double copy — strictly slower than the generic single-allocation path
below. This hits the common thousands-separator idiom
`String(n).replace(/\B(?=(\d{3})+(?!\d))/g, ",")` for 4-6 digit numbers.

Take the special case only when jsString() would actually make a rope. The threshold is
extracted into shouldMakeRope() in JSString.h and shared with the jsString() overloads.

                                                   Baseline                  Patched

string-replace-generic                         19.7240+-0.6320     ^     14.7327+-0.3753        ^ definitely 1.3388x faster
string-replace-regexp-global-thousands-separator
                                              153.5817+-1.6965     ^    120.7386+-1.0667        ^ definitely 1.2720x faster
string-replace-regexp-global-single-match
                                              152.1385+-0.4575     ^    102.3380+-0.7269        ^ definitely 1.4866x faster

Tests: JSTests/microbenchmarks/string-replace-regexp-global-single-match.js
       JSTests/microbenchmarks/string-replace-regexp-global-thousands-separator.js

* JSTests/microbenchmarks/string-replace-regexp-global-single-match.js: Added.
(test):
* JSTests/microbenchmarks/string-replace-regexp-global-thousands-separator.js: Added.
(test):
* Source/JavaScriptCore/runtime/JSString.h:
(JSC::shouldMakeRope):
* Source/JavaScriptCore/runtime/OperationsInlines.h:
(JSC::jsString):
* Source/JavaScriptCore/runtime/StringPrototypeInlines.h:
(JSC::jsSpliceSubstringsWithSeparators):
(JSC::jsSpliceSubstringsWithSeparator):

Canonical link: https://commits.webkit.org/317282@main

Author: sosukesuzuki

JSTests/microbenchmarks/string-replace-regexp-global-single-match.js

@@ -0,0 +1,26 @@
+function test(str, re)
+{
+    return str.replace(re, ",");
+}
+noInline(test);
+
+// 4-6 digit numbers: the thousands-separator idiom produces exactly one match.
+const strs = [];
+for (let k = 0; k < 16; k++) {
+    const digits = 4 + (k % 3);
+    let s = String(1 + (k % 9));
+    for (let d = 1; d < digits; d++)
+        s += String((k + d * 7) % 10);
+    strs.push(s);
+}
+
+const re = /\B(?=(\d{3})+(?!\d))/g;
+
+let result;
+for (let i = 0; i < 2000000; ++i)
+    result = test(strs[i & 15], re);
+
+if (test("1234", re) !== "1,234")
+    throw new Error("bad result: " + test("1234", re));
+if (test("123456", re) !== "123,456")
+    throw new Error("bad result: " + test("123456", re));

JSTests/microbenchmarks/string-replace-regexp-global-thousands-separator.js

@@ -0,0 +1,19 @@
+function test(str, re)
+{
+    return str.replace(re, ",");
+}
+noInline(test);
+
+// Mixed 4-8 digit numbers, as produced by the common numberWithCommas idiom.
+const strs = [];
+for (let k = 0; k < 16; k++)
+    strs.push(String(Math.round(1000 * Math.pow(1.9, k))));
+
+const re = /\B(?=(\d{3})+(?!\d))/g;
+
+let result;
+for (let i = 0; i < 2000000; ++i)
+    result = test(strs[i & 15], re);
+
+if (test("1234567", re) !== "1,234,567")
+    throw new Error("bad result: " + test("1234567", re));

Source/JavaScriptCore/runtime/JSString.h

@@ -946,6 +946,13 @@ inline JSString* JSString::getIndex(JSGlobalObject* globalObject, unsigned i)
     return jsSingleCharacterString(vm, view[i]);
 }
 
+// (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + totalLength
+// (2) Cost of making JSRopeString: sizeof(JSRopeString) + newFiberCount * sizeof(JSString) (for fibers not already wrapped in a JSString)
+ALWAYS_INLINE bool shouldMakeRope(size_t totalLength, unsigned newFiberCount)
+{
+    return StringImpl::headerSize<Latin1Character>() + totalLength >= sizeof(JSRopeString) + (newFiberCount - 1) * sizeof(JSString);
+}
+
 inline JSString* jsString(VM& vm, const String& s)
 {
     int size = s.length();

Source/JavaScriptCore/runtime/OperationsInlines.h

@@ -89,12 +89,10 @@ ALWAYS_INLINE JSString* jsString(JSGlobalObject* globalObject, const String& u1,
         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<Latin1Character>() + length1 + length2) >= sizeof(JSRopeString))
+    // We do not account the cost of copying u1's characters into a rope since u1 may be a shared
+    // StringImpl, and it may not introduce additional cost. We conservatively consider the cost of
+    // u1's JSString cell.
+    if (s2->isRope() || shouldMakeRope(static_cast<size_t>(length1) + length2, /* newFiberCount */ 1))
         return JSRopeString::create(vm, jsString(vm, u1), s2);
 
     ASSERT(!s2->isRope());
@@ -125,9 +123,7 @@ ALWAYS_INLINE JSString* jsString(JSGlobalObject* globalObject, JSString* s1, con
         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<Latin1Character>() + length1 + length2) >= sizeof(JSRopeString))
+    if (s1->isRope() || shouldMakeRope(static_cast<size_t>(length1) + length2, /* newFiberCount */ 1))
         return JSRopeString::create(vm, s1, jsString(vm, u2));
 
     ASSERT(!s1->isRope());
@@ -204,9 +200,7 @@ ALWAYS_INLINE JSString* jsString(JSGlobalObject* globalObject, const String& u1,
         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<Latin1Character>() + length1 + length2) >= (sizeof(JSRopeString) + sizeof(JSString)))
+    if (shouldMakeRope(static_cast<size_t>(length1) + length2, /* newFiberCount */ 2))
         return JSRopeString::create(vm, jsString(vm, u1), jsString(vm, u2));
 
     String newString = tryMakeString(u1, u2);
@@ -244,9 +238,7 @@ ALWAYS_INLINE JSString* jsString(JSGlobalObject* globalObject, const String& u1,
         return nullptr;
     }
 
-    // (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<Latin1Character>() + length1 + length2 + length3) >= (sizeof(JSRopeString) + sizeof(JSString) * 2))
+    if (shouldMakeRope(static_cast<size_t>(length1) + length2 + length3, /* newFiberCount */ 3))
         return JSRopeString::create(vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
 
     String newString = tryMakeString(u1, u2, u3);

Source/JavaScriptCore/runtime/StringPrototype.cpp

@@ -380,8 +380,10 @@ JSString* replaceUsingRegExpSearch(VM& vm, JSGlobalObject* globalObject, JSStrin
             break;
         }
 
-        if (global)
-            RELEASE_AND_RETURN(scope, replaceAllWithStringUsingRegExpSearch(vm, globalObject, string, source, regExp, replacementString));
+        if (global) {
+            JSString* replacementVal = replaceValue.isString() ? asString(replaceValue) : nullptr;
+            RELEASE_AND_RETURN(scope, replaceAllWithStringUsingRegExpSearch(vm, globalObject, string, source, regExp, replacementVal, replacementString));
+        }
         RELEASE_AND_RETURN(scope, replaceOneWithStringUsingRegExpSearch(vm, globalObject, string, source, regExp, replacementString));
     }
 

Source/JavaScriptCore/runtime/StringPrototypeInlines.h

@@ -171,9 +171,19 @@ ALWAYS_INLINE JSString* jsSpliceSubstringsWithSeparators(JSGlobalObject* globalO
     }
 
     if (rangeCount == 2 && separatorCount == 1) {
-        String leftPart(StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[0].begin(), substringRanges[0].distance()));
-        String rightPart(StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[1].begin(), substringRanges[1].distance()));
-        RELEASE_AND_RETURN(scope, jsString(globalObject, leftPart, separators[0], rightPart));
+        // Only profitable when jsString() below would create a rope. For shorter results, jsString()
+        // flattens via tryMakeString anyway, so the substrings and the extra copy are pure overhead
+        // compared to the generic single-allocation path below.
+        CheckedInt32 length = substringRanges[0].distance();
+        length += substringRanges[1].distance();
+        length += separators[0].length();
+        if (!length.hasOverflowed()) [[likely]] {
+            if (shouldMakeRope(length.value(), /* newFiberCount */ 3)) {
+                String leftPart(StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[0].begin(), substringRanges[0].distance()));
+                String rightPart(StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[1].begin(), substringRanges[1].distance()));
+                RELEASE_AND_RETURN(scope, jsString(globalObject, leftPart, separators[0], rightPart));
+            }
+        }
     }
 
     CheckedInt32 totalLength = 0;
@@ -244,7 +254,7 @@ ALWAYS_INLINE JSString* jsSpliceSubstringsWithSeparators(JSGlobalObject* globalO
     RELEASE_AND_RETURN(scope, jsString(vm, impl.releaseNonNull()));
 }
 
-ALWAYS_INLINE JSString* jsSpliceSubstringsWithSeparator(JSGlobalObject* globalObject, JSString* sourceVal, const String& source, const Range<int32_t>* substringRanges, int rangeCount, const String& separator, int separatorCount)
+ALWAYS_INLINE JSString* jsSpliceSubstringsWithSeparator(JSGlobalObject* globalObject, JSString* sourceVal, const String& source, const Range<int32_t>* substringRanges, int rangeCount, JSString* separatorVal, const String& separator, int separatorCount)
 {
     VM& vm = getVM(globalObject);
     auto scope = DECLARE_THROW_SCOPE(vm);
@@ -260,9 +270,25 @@ ALWAYS_INLINE JSString* jsSpliceSubstringsWithSeparator(JSGlobalObject* globalOb
     }
 
     if (rangeCount == 2 && separatorCount == 1) {
-        String leftPart(StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[0].begin(), substringRanges[0].distance()));
-        String rightPart(StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[1].begin(), substringRanges[1].distance()));
-        RELEASE_AND_RETURN(scope, jsString(globalObject, leftPart, separator, rightPart));
+        // Only profitable when jsString() below would create a rope. For shorter results, jsString()
+        // flattens via tryMakeString anyway, so the substrings and the extra copy are pure overhead
+        // compared to the generic single-allocation path below.
+        CheckedInt32 length = substringRanges[0].distance();
+        length += substringRanges[1].distance();
+        length += separator.length();
+        if (!length.hasOverflowed()) [[likely]] {
+            if (separatorVal) {
+                if (shouldMakeRope(length.value(), /* newFiberCount */ 2)) {
+                    JSString* leftPart = jsString(vm, StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[0].begin(), substringRanges[0].distance()));
+                    JSString* rightPart = jsString(vm, StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[1].begin(), substringRanges[1].distance()));
+                    RELEASE_AND_RETURN(scope, jsString(globalObject, leftPart, separatorVal, rightPart));
+                }
+            } else if (shouldMakeRope(length.value(), /* newFiberCount */ 3)) {
+                String leftPart(StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[0].begin(), substringRanges[0].distance()));
+                String rightPart(StringImpl::createSubstringSharingImpl(*source.impl(), substringRanges[1].begin(), substringRanges[1].distance()));
+                RELEASE_AND_RETURN(scope, jsString(globalObject, leftPart, separator, rightPart));
+            }
+        }
     }
 
     CheckedInt32 totalLength = 0;
@@ -1199,7 +1225,7 @@ static ALWAYS_INLINE JSString* tryTrimSpaces(VM& vm, JSGlobalObject* globalObjec
     return nullptr;
 }
 
-ALWAYS_INLINE JSString* replaceAllWithStringUsingRegExpSearchNoBackreferences(VM& vm, JSGlobalObject* globalObject, JSString* string, const String& source, RegExp* regExp, const String& replacementString)
+ALWAYS_INLINE JSString* replaceAllWithStringUsingRegExpSearchNoBackreferences(VM& vm, JSGlobalObject* globalObject, JSString* string, const String& source, RegExp* regExp, JSString* replacementVal, const String& replacementString)
 {
     auto scope = DECLARE_THROW_SCOPE(vm);
 
@@ -1245,7 +1271,7 @@ ALWAYS_INLINE JSString* replaceAllWithStringUsingRegExpSearchNoBackreferences(VM
         if (!sourceRanges.tryConstructAndAppend(lastIndex, sourceLen)) [[unlikely]]
             OUT_OF_MEMORY(globalObject, scope);
     }
-    RELEASE_AND_RETURN(scope, jsSpliceSubstringsWithSeparator(globalObject, string, source, sourceRanges.span().data(), sourceRanges.size(), replacementString, replacementCount));
+    RELEASE_AND_RETURN(scope, jsSpliceSubstringsWithSeparator(globalObject, string, source, sourceRanges.span().data(), sourceRanges.size(), replacementVal, replacementString, replacementCount));
 }
 
 struct StringReplaceTemplatePart {
@@ -1406,13 +1432,13 @@ ALWAYS_INLINE void appendReplacementUsingTemplate(StringBuilder& result, std::sp
     }
 }
 
-ALWAYS_INLINE JSString* replaceAllWithStringUsingRegExpSearch(VM& vm, JSGlobalObject* globalObject, JSString* string, const String& source, RegExp* regExp, const String& replacementString)
+ALWAYS_INLINE JSString* replaceAllWithStringUsingRegExpSearch(VM& vm, JSGlobalObject* globalObject, JSString* string, const String& source, RegExp* regExp, JSString* replacementVal, const String& replacementString)
 {
     auto scope = DECLARE_THROW_SCOPE(vm);
 
     size_t dollarPos = replacementString.find('$');
     if (dollarPos == notFound)
-        RELEASE_AND_RETURN(scope, replaceAllWithStringUsingRegExpSearchNoBackreferences(vm, globalObject, string, source, regExp, replacementString));
+        RELEASE_AND_RETURN(scope, replaceAllWithStringUsingRegExpSearchNoBackreferences(vm, globalObject, string, source, regExp, replacementVal, replacementString));
 
     StringReplaceTemplateParts templateParts;