Changeset 245194 in webkit

Timestamp:

May 10, 2019 1:49:35 PM (5 years ago)

Author:

ysuzuki@apple.com

Message:

[JSC] String substring operation should return ropes consistently
​https://bugs.webkit.org/show_bug.cgi?id=197765
<rdar://problem/37689944>

Reviewed by Michael Saboff.

Currently we have different policies per string substring operation function.

1. String#slice returns the resolved non-rope string
2. String#substring returns rope string
3. String#substr returns rope string in runtime function, non-rope string in DFG and FTL

Due to (3), we see large memory use in the tested web page[1]. Non rope substring have a problem.
First of all, that returned string seems not used immediately. It is possible that the resulted
string is used as a part of the other ropes (like, xxx.substring(...) + "Hello"). To avoid the
eager materialization of the string, we are using StringImpl::createSubstringSharingImpl for the
resulted non rope string. StringImpl::createSubstringSharingImpl is StringImpl's substring feature: the
substring is pointing the owner StringImpl. While this has memory saving benefit, it can retain owner
StringImpl so long, and it could keep very large owner StringImpl alive.

The problem we are attempting to solve with StringImpl::createSubstringSharingImpl can be solved by
the rope string simply. Rope string can share the underlying string. And good feature of the rope
string is that, when resolving rope string, the rope string always create a new StringImpl instead of
using StringImpl::createSubstringSharingImpl. So we allow the owner StringImpl to be destroyed. And this
resolving only happens when we actually want to use the content of the rope string. In addition, we recently
shrunk the sizeof(JSRopeString) from 48 to 32, so JSRopeString is cheap.

In this patch, we change (2) and (3) to (1), using rope String as a result of substring operations.

RAMification and JetStream2 are neutral. The web page[1] shows large memory footprint improvement from 776MB to 681MB.

[1]: ​https://beta.observablehq.com/@ldgardner/assignment-4-visualizations-and-multiple-views

• dfg/DFGOperations.cpp:
• runtime/StringPrototype.cpp:

(JSC::stringProtoFuncSlice):

• runtime/StringPrototypeInlines.h:

(JSC::stringSlice):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGOperations.cpp (modified) (2 diffs)
• runtime/StringPrototype.cpp (modified) (2 diffs)
• runtime/StringPrototypeInlines.h (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-05-10  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] String substring operation should return ropes consistently
+        https://bugs.webkit.org/show_bug.cgi?id=197765
+        <rdar://problem/37689944>
+
+        Reviewed by Michael Saboff.
+
+        Currently we have different policies per string substring operation function.
+
+            1. String#slice returns the resolved non-rope string
+            2. String#substring returns rope string
+            3. String#substr returns rope string in runtime function, non-rope string in DFG and FTL
+
+        Due to (3), we see large memory use in the tested web page[1]. Non rope substring have a problem.
+        First of all, that returned string seems not used immediately. It is possible that the resulted
+        string is used as a part of the other ropes (like, xxx.substring(...) + "Hello"). To avoid the
+        eager materialization of the string, we are using StringImpl::createSubstringSharingImpl for the
+        resulted non rope string. StringImpl::createSubstringSharingImpl is StringImpl's substring feature: the
+        substring is pointing the owner StringImpl. While this has memory saving benefit, it can retain owner
+        StringImpl so long, and it could keep very large owner StringImpl alive.
+
+        The problem we are attempting to solve with StringImpl::createSubstringSharingImpl can be solved by
+        the rope string simply. Rope string can share the underlying string. And good feature of the rope
+        string is that, when resolving rope string, the rope string always create a new StringImpl instead of
+        using StringImpl::createSubstringSharingImpl. So we allow the owner StringImpl to be destroyed. And this
+        resolving only happens when we actually want to use the content of the rope string. In addition, we recently
+        shrunk the sizeof(JSRopeString) from 48 to 32, so JSRopeString is cheap.
+
+        In this patch, we change (2) and (3) to (1), using rope String as a result of substring operations.
+
+        RAMification and JetStream2 are neutral. The web page[1] shows large memory footprint improvement from 776MB to 681MB.
+
+        [1]: https://beta.observablehq.com/@ldgardner/assignment-4-visualizations-and-multiple-views
+
+        * dfg/DFGOperations.cpp:
+        * runtime/StringPrototype.cpp:
+        (JSC::stringProtoFuncSlice):
+        * runtime/StringPrototypeInlines.h:
+        (JSC::stringSlice):
+
 2019-05-10  Robin Morisset  <rmorisset@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGOperations.cpp

@@ -2198 +2198 @@
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
-    auto scope = DECLARE_THROW_SCOPE(vm);
-
-    auto string = jsCast<JSString*>(cell)->value(exec);
-    RETURN_IF_EXCEPTION(scope, nullptr);
-    return jsSubstring(&vm, string, from, span);
+
+    return jsSubstring(vm, exec, jsCast<JSString*>(cell), from, span);
 }
 
@@ -2209 +2206 @@
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
-    auto scope = DECLARE_THROW_SCOPE(vm);
-
-    auto string = jsCast<JSString*>(cell)->value(exec);
-    RETURN_IF_EXCEPTION(scope, nullptr);
+
+    JSString* string = asString(cell);
     static_assert(static_cast<uint64_t>(JSString::MaxLength) <= static_cast<uint64_t>(std::numeric_limits<int32_t>::max()), "");
-
-    return stringSlice(vm, WTFMove(string), start, end);
+    return stringSlice(exec, vm, string, string->length(), start, end);
 }
 

trunk/Source/JavaScriptCore/runtime/StringPrototype.cpp

@@ -1144 +1144 @@
     if (!checkObjectCoercible(thisValue))
         return throwVMTypeError(exec, scope);
-    String s = thisValue.toWTFString(exec);
+    JSString* string = thisValue.toString(exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
@@ -1150 +1150 @@
     JSValue a1 = exec->argument(1);
 
-    int len = s.length();
-    RELEASE_ASSERT(len >= 0);
+    int length = string->length();
+    RELEASE_ASSERT(length >= 0);
 
     // The arg processing is very much like ArrayProtoFunc::Slice
     double start = a0.toInteger(exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    double end = a1.isUndefined() ? len : a1.toInteger(exec);
-    RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    return JSValue::encode(stringSlice(vm, WTFMove(s), start, end));
+    double end = a1.isUndefined() ? length : a1.toInteger(exec);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    RELEASE_AND_RETURN(scope, JSValue::encode(stringSlice(exec, vm, string, length, start, end)));
 }
 

trunk/Source/JavaScriptCore/runtime/StringPrototypeInlines.h

@@ -31 +31 @@
 
 template<typename NumberType>
-ALWAYS_INLINE JSString* stringSlice(VM& vm, String&& string, NumberType start, NumberType end)
+ALWAYS_INLINE JSString* stringSlice(ExecState* exec, VM& vm, JSString* string, int32_t length, NumberType start, NumberType end)
 {
-    int32_t length = string.length();
     NumberType from = start < 0 ? length + start : start;
     NumberType to = end < 0 ? length + end : end;
@@ -41 +40 @@
         if (to > length)
             to = length;
-        return jsSubstring(&vm, WTFMove(string), static_cast<unsigned>(from), static_cast<unsigned>(to) - static_cast<unsigned>(from));
+        return jsSubstring(vm, exec, string, static_cast<unsigned>(from), static_cast<unsigned>(to) - static_cast<unsigned>(from));
     }
     return jsEmptyString(&vm);