Attachment 369580 Details for Bug 197765 – Patch

[patch] Patch

bug-197765-20190510123151.patch (text/plain), 7.03 KB, created by Yusuke Suzuki on 2019-05-10 12:31:51 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-05-10 12:31:51 PDT

Size: 7.03 KB

patch

obsolete

>Subversion Revision: 245178
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 5c788c203aae6770b05eb33131b82b93f30bea63..1afe84f3201ef25dd08c48d326d78d51a3892b33 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,44 @@
>+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 NOBODY (OOPS!).
>+
>+        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-09  Keith Miller  <keith_miller@apple.com>
> 
>         Fix crashes related to pointer authentication for primitive gigacage
>diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>index 2ea4eb36cbe0497ff98c692b2ec9a39ecd391677..ee53d61d7f7546781a9046760f2a5a9ef38b6ed5 100644
>--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>@@ -2197,24 +2197,18 @@ JSCell* JIT_OPERATION operationStringSubstr(ExecState* exec, JSCell* cell, int32
> {
>     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);
> }
> 
> JSCell* JIT_OPERATION operationStringSlice(ExecState* exec, JSCell* cell, int32_t start, int32_t end)
> {
>     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);
> }
> 
> JSString* JIT_OPERATION operationToLowerCase(ExecState* exec, JSString* string, uint32_t failingIndex)
>diff --git a/Source/JavaScriptCore/runtime/StringPrototype.cpp b/Source/JavaScriptCore/runtime/StringPrototype.cpp
>index 348bf0daf5d0d8d10329cfb90fd048c218a2b3e9..f4db8fd4848f11b3b5558063eb36bf63a11cd9b0 100644
>--- a/Source/JavaScriptCore/runtime/StringPrototype.cpp
>+++ b/Source/JavaScriptCore/runtime/StringPrototype.cpp
>@@ -1143,21 +1143,21 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncSlice(ExecState* exec)
>     JSValue thisValue = exec->thisValue();
>     if (!checkObjectCoercible(thisValue))
>         return throwVMTypeError(exec, scope);
>-    String s = thisValue.toWTFString(exec);
>+    JSString* string = thisValue.toString(exec);
>     RETURN_IF_EXCEPTION(scope, encodedJSValue());
> 
>     JSValue a0 = exec->argument(0);
>     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);
>+    double end = a1.isUndefined() ? length : a1.toInteger(exec);
>     RETURN_IF_EXCEPTION(scope, encodedJSValue());
>-    return JSValue::encode(stringSlice(vm, WTFMove(s), start, end));
>+    RELEASE_AND_RETURN(scope, JSValue::encode(stringSlice(exec, vm, string, length, start, end)));
> }
> 
> // Return true in case of early return (resultLength got to limitLength).
>diff --git a/Source/JavaScriptCore/runtime/StringPrototypeInlines.h b/Source/JavaScriptCore/runtime/StringPrototypeInlines.h
>index 75ce8d1ac829282abee69637946270afed254496..c51d8901c4c0ab68caec751f0f7c56acfae48bae 100644
>--- a/Source/JavaScriptCore/runtime/StringPrototypeInlines.h
>+++ b/Source/JavaScriptCore/runtime/StringPrototypeInlines.h
>@@ -30,9 +30,8 @@
> namespace JSC {
> 
> 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;
>     if (to > from && to > 0 && from < length) {
>@@ -40,7 +39,7 @@ ALWAYS_INLINE JSString* stringSlice(VM& vm, String&& string, NumberType start, N
>             from = 0;
>         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);
> }

Flags:

msaboff: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197765: 369541 | 369564 | 369580