WebKit Bugzilla
Attachment 361890 Details for
Bug 194465
: We should only make rope strings when concatenating strings long enough.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-194465-20190212212347.patch (text/plain), 13.04 KB, created by
Yusuke Suzuki
on 2019-02-12 21:23:48 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Yusuke Suzuki
Created:
2019-02-12 21:23:48 PST
Size:
13.04 KB
patch
obsolete
>Subversion Revision: 241311 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index 755147932328300b3f9b3b3ea45080d5a1396254..a6052dabef666d1e9a4825650245b86f88bd0e1e 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,52 @@ >+2019-02-12 Keith Miller <keith_miller@apple.com> and Yusuke Suzuki <ysuzuki@apple.com> >+ >+ We should only make rope strings when concatenating strings long enough. >+ https://bugs.webkit.org/show_bug.cgi?id=194465 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ This patch stops us from allocating a rope string if the resulting >+ rope would be smaller than the size of the JSRopeString object we >+ would need to allocate. >+ >+ This patch also adds paths so that we don't unnecessarily allocate >+ JSString cells for primitives we are going to concatenate with a >+ string anyway. >+ >+ The important change from the previous one is that we do not apply >+ the above rule to JSRopeStrings generated by JSStrings. If we convert >+ it to JSString, comparison of memory consumption becomes the following, >+ because JSRopeString does not have StringImpl until it is resolved. >+ >+ sizeof(JSRopeString) v.s. sizeof(JSString) + sizeof(StringImpl) + content >+ >+ Since sizeof(JSString) + sizeof(StringImpl) is larger than sizeof(JSRopeString), >+ resolving eagerly increases memory footprint. The point is that we need to >+ account newly created JSString and JSRopeString from the operands. This is the >+ reason why this patch adds different thresholds for each jsString functions. >+ >+ In CLI execution, this change is performance neutral in JetStream2 (average in 6 runs on MBP). >+ >+ Before: 160.76083333333332 >+ After: 160.68233333333333 >+ >+ And we can get 1.7% improvement in RAMification (average in 6 runs on MBP). >+ >+ Before: 35183395.333333336 >+ After: 34590661.166666664 >+ >+ * dfg/DFGOperations.cpp: >+ * runtime/CommonSlowPaths.cpp: >+ (JSC::SLOW_PATH_DECL): >+ * runtime/JSString.h: >+ (JSC::JSString::isRope const): >+ * runtime/Operations.cpp: >+ (JSC::jsAddSlowCase): >+ * runtime/Operations.h: >+ (JSC::jsString): >+ (JSC::jsAddNonNumber): >+ (JSC::jsAdd): >+ > 2019-02-12 Michael Catanzaro <mcatanzaro@igalia.com> > > [WPE][GTK] Unsafe g_unsetenv() use in WebProcessPool::platformInitialize >diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp >index 984fc0b961e500954d6df0c4a9d3a677749eaaff..6a578266eb4a128327e53cdaacb7d861577c49ef 100644 >--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp >+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp >@@ -503,12 +503,7 @@ EncodedJSValue JIT_OPERATION operationValueAddNotNumber(ExecState* exec, Encoded > JSValue op1 = JSValue::decode(encodedOp1); > JSValue op2 = JSValue::decode(encodedOp2); > >- ASSERT(!op1.isNumber() || !op2.isNumber()); >- >- if (op1.isString() && !op2.isObject()) >- return JSValue::encode(jsString(exec, asString(op1), op2.toString(exec))); >- >- return JSValue::encode(jsAddSlowCase(exec, op1, op2)); >+ return JSValue::encode(jsAddNonNumber(exec, op1, op2)); > } > > EncodedJSValue JIT_OPERATION operationValueDiv(ExecState* exec, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2) >diff --git a/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp b/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp >index bbbadab67fdf6fcad4cac0664cdc8e60bf5e4b2e..5871c930bbc8e06636110ea4c6769b202340751d 100644 >--- a/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp >+++ b/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp >@@ -517,19 +517,11 @@ SLOW_PATH_DECL(slow_path_add) > auto bytecode = pc->as<OpAdd>(); > JSValue v1 = GET_C(bytecode.m_lhs).jsValue(); > JSValue v2 = GET_C(bytecode.m_rhs).jsValue(); >- JSValue result; > > ArithProfile& arithProfile = *exec->codeBlock()->arithProfileForPC(pc); > arithProfile.observeLHSAndRHS(v1, v2); > >- if (v1.isString() && !v2.isObject()) { >- JSString* v2String = v2.toString(exec); >- if (LIKELY(!throwScope.exception())) >- result = jsString(exec, asString(v1), v2String); >- } else if (v1.isNumber() && v2.isNumber()) >- result = jsNumber(v1.asNumber() + v2.asNumber()); >- else >- result = jsAddSlowCase(exec, v1, v2); >+ JSValue result = jsAdd(exec, v1, v2); > > RETURN_WITH_PROFILING(result, { > updateArithProfileForBinaryArithOp(exec, pc, result, v1, v2); >diff --git a/Source/JavaScriptCore/runtime/JSString.h b/Source/JavaScriptCore/runtime/JSString.h >index 00afd41432204d0042ba7a95390641d17bddcc92..948a97096d9c0346bbf35291a9005ad61cbda9b6 100644 >--- a/Source/JavaScriptCore/runtime/JSString.h >+++ b/Source/JavaScriptCore/runtime/JSString.h >@@ -196,11 +196,12 @@ class JSString : public JSCell { > Is8Bit = 1u > }; > >+ bool isRope() const { return m_value.isNull(); } >+ > protected: > friend class JSValue; > > JS_EXPORT_PRIVATE bool equalSlowCase(ExecState*, JSString* other) const; >- bool isRope() const { return m_value.isNull(); } > bool isSubstring() const; > bool is8Bit() const { return m_flags & Is8Bit; } > void setIs8Bit(bool flag) const >@@ -486,7 +487,10 @@ class JSRopeString final : public JSString { > > > friend JSString* jsString(ExecState*, JSString*, JSString*); >+ friend JSString* jsString(ExecState*, const String&, JSString*); >+ friend JSString* jsString(ExecState*, JSString*, const String&); > friend JSString* jsString(ExecState*, JSString*, JSString*, JSString*); >+ friend JSString* jsString(ExecState*, const String&, const String&); > friend JSString* jsString(ExecState*, const String&, const String&, const String&); > }; > >diff --git a/Source/JavaScriptCore/runtime/Operations.cpp b/Source/JavaScriptCore/runtime/Operations.cpp >index 836f953c3a87c57d5c622fbf4c70509050751beb..a5310b346d89945bea295a1623d70a5707fcb93a 100644 >--- a/Source/JavaScriptCore/runtime/Operations.cpp >+++ b/Source/JavaScriptCore/runtime/Operations.cpp >@@ -52,13 +52,23 @@ NEVER_INLINE JSValue jsAddSlowCase(CallFrame* callFrame, JSValue v1, JSValue v2) > RETURN_IF_EXCEPTION(scope, { }); > > if (p1.isString()) { >- JSString* p2String = p2.toString(callFrame); >+ if (p2.isCell()) { >+ JSString* p2String = p2.toString(callFrame); >+ RETURN_IF_EXCEPTION(scope, { }); >+ RELEASE_AND_RETURN(scope, jsString(callFrame, asString(p1), p2String)); >+ } >+ String p2String = p2.toWTFString(callFrame); > RETURN_IF_EXCEPTION(scope, { }); > RELEASE_AND_RETURN(scope, jsString(callFrame, asString(p1), p2String)); > } > > if (p2.isString()) { >- JSString* p1String = p1.toString(callFrame); >+ if (p2.isCell()) { >+ JSString* p1String = p1.toString(callFrame); >+ RETURN_IF_EXCEPTION(scope, { }); >+ RELEASE_AND_RETURN(scope, jsString(callFrame, p1String, asString(p2))); >+ } >+ String p1String = p1.toWTFString(callFrame); > RETURN_IF_EXCEPTION(scope, { }); > RELEASE_AND_RETURN(scope, jsString(callFrame, p1String, asString(p2))); > } >diff --git a/Source/JavaScriptCore/runtime/Operations.h b/Source/JavaScriptCore/runtime/Operations.h >index 0ea98291bb68873bb3bab10461861e9158a87bd4..7e6aeef795f3cc997d4605104014e174c5c68187 100644 >--- a/Source/JavaScriptCore/runtime/Operations.h >+++ b/Source/JavaScriptCore/runtime/Operations.h >@@ -37,6 +37,50 @@ JSValue jsTypeStringForValue(VM&, JSGlobalObject*, JSValue); > bool jsIsObjectTypeOrNull(CallFrame*, JSValue); > size_t normalizePrototypeChain(CallFrame*, JSCell*, bool& sawPolyProto); > >+ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, JSString* s2) >+{ >+ VM& vm = exec->vm(); >+ auto scope = DECLARE_THROW_SCOPE(vm); >+ >+ unsigned length1 = u1.length(); >+ if (!length1) >+ return s2; >+ unsigned length2 = s2->length(); >+ if (!length2) >+ return jsString(&vm, u1); >+ >+ // JSRopeString + JSString v.s. JSString >+ if (s2->isRope() || (length1 + length2) >= sizeof(JSRopeString)) >+ return jsString(exec, jsString(&vm, u1), s2); >+ >+ const String& u2 = s2->value(exec); >+ UNUSED_PARAM(u2); >+ RETURN_IF_EXCEPTION(scope, { }); >+ return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); >+} >+ >+ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, const String& u2) >+{ >+ VM& vm = exec->vm(); >+ auto scope = DECLARE_THROW_SCOPE(vm); >+ >+ unsigned length1 = s1->length(); >+ if (!length1) >+ return jsString(&vm, u2); >+ unsigned length2 = u2.length(); >+ if (!length2) >+ return s1; >+ >+ // JSRopeString + JSString v.s. JSString >+ if (s1->isRope() || (length1 + length2) >= sizeof(JSRopeString)) >+ return jsString(exec, s1, jsString(&vm, u2)); >+ >+ const String& u1 = s1->value(exec); >+ UNUSED_PARAM(u1); >+ RETURN_IF_EXCEPTION(scope, { }); >+ return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); >+} >+ > ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2) > { > VM& vm = exec->vm(); >@@ -79,9 +123,34 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2, JS > throwOutOfMemoryError(exec, scope); > return nullptr; > } >+ > return JSRopeString::create(vm, s1, s2, s3); > } > >+ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String& u2) >+{ >+ VM& vm = exec->vm(); >+ auto scope = DECLARE_THROW_SCOPE(vm); >+ >+ unsigned length1 = u1.length(); >+ if (!length1) >+ return jsString(&vm, u2); >+ unsigned length2 = u2.length(); >+ if (!length2) >+ return jsString(&vm, u1); >+ static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), ""); >+ if (sumOverflows<int32_t>(length1, length2)) { >+ throwOutOfMemoryError(exec, scope); >+ return nullptr; >+ } >+ >+ // JSRopeString + JSString * 2 v.s. JSString >+ if (length1 + length2 >= (sizeof(JSRopeString) + sizeof(JSString))) >+ return JSRopeString::create(vm, jsString(&vm, u1), jsString(&vm, u2)); >+ >+ return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); >+} >+ > ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String& u2, const String& u3) > { > VM* vm = &exec->vm(); >@@ -95,13 +164,13 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String > ASSERT(length3 <= JSString::MaxLength); > > if (!length1) >- RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u2), jsString(vm, u3))); >+ RELEASE_AND_RETURN(scope, jsString(exec, u2, u3)); > > if (!length2) >- RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u1), jsString(vm, u3))); >+ RELEASE_AND_RETURN(scope, jsString(exec, u1, u3)); > > if (!length3) >- RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u1), jsString(vm, u2))); >+ RELEASE_AND_RETURN(scope, jsString(exec, u1, u2)); > > static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), ""); > if (sumOverflows<int32_t>(length1, length2, length3)) { >@@ -109,7 +178,11 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String > return nullptr; > } > >- return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3)); >+ // JSRopeString + JSString * 3 v.s. JSString >+ if (length1 + length2 + length3 >= (sizeof(JSRopeString) + sizeof(JSString) * 2)) >+ return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3)); >+ >+ return JSString::create(*vm, makeString(u1, u2, u3).releaseImpl().releaseNonNull()); > } > > ALWAYS_INLINE JSValue jsStringFromRegisterArray(ExecState* exec, Register* strings, unsigned count) >@@ -325,16 +398,31 @@ ALWAYS_INLINE bool jsLessEq(CallFrame* callFrame, JSValue v1, JSValue v2) > // 13962 Add case: 5 3 > // 4000 Add case: 3 5 > >+ >+ALWAYS_INLINE JSValue jsAddNonNumber(CallFrame* callFrame, JSValue v1, JSValue v2) >+{ >+ VM& vm = callFrame->vm(); >+ auto scope = DECLARE_THROW_SCOPE(vm); >+ ASSERT(!v1.isNumber() || !v2.isNumber()); >+ >+ if (LIKELY(v1.isString() && !v2.isObject())) { >+ if (v2.isString()) >+ RELEASE_AND_RETURN(scope, jsString(callFrame, asString(v1), asString(v2))); >+ String s2 = v2.toWTFString(callFrame); >+ RETURN_IF_EXCEPTION(scope, { }); >+ RELEASE_AND_RETURN(scope, jsString(callFrame, asString(v1), s2)); >+ } >+ >+ // All other cases are pretty uncommon >+ RELEASE_AND_RETURN(scope, jsAddSlowCase(callFrame, v1, v2)); >+} >+ > ALWAYS_INLINE JSValue jsAdd(CallFrame* callFrame, JSValue v1, JSValue v2) > { > if (v1.isNumber() && v2.isNumber()) > return jsNumber(v1.asNumber() + v2.asNumber()); > >- if (v1.isString() && !v2.isObject()) >- return jsString(callFrame, asString(v1), v2.toString(callFrame)); >- >- // All other cases are pretty uncommon >- return jsAddSlowCase(callFrame, v1, v2); >+ return jsAddNonNumber(callFrame, v1, v2); > } > > ALWAYS_INLINE JSValue jsSub(ExecState* exec, JSValue v1, JSValue v2)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194465
:
361564
|
361568
|
361573
|
361578
|
361581
|
361584
|
361585
|
361586
|
361890
|
361891