Attachment 369541 Details for Bug 197765 – Patch

[patch] Patch

bug-197765-20190509204233.patch (text/plain), 6.36 KB, created by Yusuke Suzuki on 2019-05-09 20:42:34 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-05-09 20:42:34 PDT

Size: 6.36 KB

patch

obsolete

>Subversion Revision: 245167
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 8895238a9c44b8da66659a94a20b3f32f4b54fdb..1fcae81eb6759da666a2864d4f72848b61fb500f 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2019-05-09  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] String substring operation should return ropes consistently
>+        https://bugs.webkit.org/show_bug.cgi?id=197765
>+
>+        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 https://beta.observablehq.com/@ldgardner/assignment-4-visualizations-and-multiple-views.
>+        The result of String#{substring,substr,slice} would not be resolved and used. So we should return rope string instead because
>+        now rope string is cheap (sizeof(JSRopeString) is 32).
>+
>+        In addition, rope String allows us to resolve string when we actually use it. Currently, we eagerly resolve the resulted string.
>+        To make memory efficient, we are using 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 long owner StringImpl live.
>+        When resolving rope String, we always create a new StringImpl. 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 this patch, we change (2) and (3) to (1), using rope String as a result of substring operations.
>+
>+        * dfg/DFGOperations.cpp:
>+        * runtime/StringPrototype.cpp:
>+        (JSC::stringProtoFuncSlice):
>+        * runtime/StringPrototypeInlines.h:
>+        (JSC::stringSlice):
>+
> 2019-05-09  Keith Miller  <keith_miller@apple.com>
> 
>         parseStatementListItem needs a stack overflow check
>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);
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 197765: 369541 | 369564 | 369580