- Revision
- 245194
- Author
- [email protected]
- Date
- 2019-05-10 13:49:35 -0700 (Fri, 10 May 2019)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (245193 => 245194)
--- trunk/Source/_javascript_Core/ChangeLog 2019-05-10 20:42:09 UTC (rev 245193)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-05-10 20:49:35 UTC (rev 245194)
@@ -1,3 +1,44 @@
+2019-05-10 Yusuke Suzuki <[email protected]>
+
+ [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 <[email protected]>
testb3 failing with crash in JSC::B3::BasicBlock::appendNonTerminal
Modified: trunk/Source/_javascript_Core/dfg/DFGOperations.cpp (245193 => 245194)
--- trunk/Source/_javascript_Core/dfg/DFGOperations.cpp 2019-05-10 20:42:09 UTC (rev 245193)
+++ trunk/Source/_javascript_Core/dfg/DFGOperations.cpp 2019-05-10 20:49:35 UTC (rev 245194)
@@ -2197,11 +2197,8 @@
{
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)
@@ -2208,13 +2205,10 @@
{
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)
Modified: trunk/Source/_javascript_Core/runtime/StringPrototype.cpp (245193 => 245194)
--- trunk/Source/_javascript_Core/runtime/StringPrototype.cpp 2019-05-10 20:42:09 UTC (rev 245193)
+++ trunk/Source/_javascript_Core/runtime/StringPrototype.cpp 2019-05-10 20:49:35 UTC (rev 245194)
@@ -1143,21 +1143,21 @@
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).
Modified: trunk/Source/_javascript_Core/runtime/StringPrototypeInlines.h (245193 => 245194)
--- trunk/Source/_javascript_Core/runtime/StringPrototypeInlines.h 2019-05-10 20:42:09 UTC (rev 245193)
+++ trunk/Source/_javascript_Core/runtime/StringPrototypeInlines.h 2019-05-10 20:49:35 UTC (rev 245194)
@@ -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 @@
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);
}
