We should only make rope strings when concatenating strings long enough.
Created attachment 361564 [details] Patch
Comment on attachment 361564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361564&action=review > Source/JavaScriptCore/runtime/Operations.h:41 > +// Note, it doesn't differentiate between 8 and 16-bit strings because 16-bit strings are relatively rare.` I'll remove the ` in this comment before landing.
Comment on attachment 361564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361564&action=review > Source/JavaScriptCore/dfg/DFGOperations.cpp:509 > + return JSValue::encode(jsString(exec, asString(op1), op2.toWTFString(exec))); Don't we want an exception check here? > Source/JavaScriptCore/runtime/JSString.h:493 > + please revert > Source/JavaScriptCore/runtime/Operations.h:41 > +// Note, it doesn't differentiate between 8 and 16-bit strings because 16-bit strings are relatively rare.` Remove trailing ` > Source/JavaScriptCore/runtime/Operations.h:42 > +constexpr unsigned minRopeStringLength = sizeof(JSRopeString); nit: let's call this "minimumRopeStringLength" > Source/JavaScriptCore/runtime/Operations.h:77 > + unsigned length1 = s1->length(); > + if (!length1) > + return jsString(&vm, u2); > + unsigned length2 = u2.length(); > + if (!length2) > + return s1; > + > + if (length1 + length2 >= minRopeStringLength) > + return jsString(exec, s1, jsString(&vm, u2)); Can you make a helper called something like shouldMakeRope since the code above is super similar. > Source/JavaScriptCore/runtime/Operations.h:81 > + return JSString::create(vm, makeRef(*makeString(u1, u2).impl())); I think you want releaseImpl() here. > Source/JavaScriptCore/runtime/Operations.h:108 > + return JSString::create(vm, makeRef(*makeString(u1, u2).impl())); ditto > Source/JavaScriptCore/runtime/Operations.h:143 > + return JSString::create(vm, makeRef(*makeString(u1, u2, u3).impl())); ditto > Source/JavaScriptCore/runtime/Operations.h:176 > + return JSString::create(*vm, makeRef(*makeString(u1, u2, u3).impl())); ditto
Created attachment 361568 [details] Patch for landing
Comment on attachment 361564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361564&action=review >> Source/JavaScriptCore/dfg/DFGOperations.cpp:509 >> + return JSValue::encode(jsString(exec, asString(op1), op2.toWTFString(exec))); > > Don't we want an exception check here? I'll just make a jsAddNonNumber from the second half of jsAdd and have this operation call that method that way we have less duplicated code. >> Source/JavaScriptCore/runtime/JSString.h:493 >> + > > please revert done. >> Source/JavaScriptCore/runtime/Operations.h:77 >> + return jsString(exec, s1, jsString(&vm, u2)); > > Can you make a helper called something like shouldMakeRope since the code above is super similar. This is hard because of lhs vs rhs issues. >> Source/JavaScriptCore/runtime/Operations.h:81 >> + return JSString::create(vm, makeRef(*makeString(u1, u2).impl())); > > I think you want releaseImpl() here. done. I think I need an adoptRef then.
Comment on attachment 361568 [details] Patch for landing Rejecting attachment 361568 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: LE_APPLE_PAY -DENABLE_APPLE_PAY_SESSION_V3 -DENABLE_APPLICATION_MANIFEST -DENABLE_ATTACHMENT_ELEMENT -DENABLE_AVF_CAPTIONS -DENABLE_CACHE_PARTITIONING -DENABLE_CHANNEL_MESSAGING -DENABLE_CONTENT_FILTERING -DENABLE_CSS_BOX_DECORATION_BREAK -DENABLE_CSS_COMPOSITING -DENABLE_CSS_PAINTING_API -DENABLE_CSS_SCROLL_SNAP -DENABLE_CSS_SELECTORS_LEVEL4 -DENABLE_CSS_TRAILING_WORD -DENABLE_CSS_TYPED_OM -DENABLE_CURSOR_VISIBILITY -DENABLE_DARK_MODE_CSS -DENABLE_DASHBOARD_SUPPORT -DENABLE_DATACUE_VALUE -DENABLE_DATALIST_ELEMENT -DENABLE_EXPERIMENTAL_FEATURES -DENABLE_FILTERS_LEVEL_2 -DENABLE_FTL_JIT -DENABLE_FULLSCREEN_API -DENABLE_GAMEPAD -DENABLE_GEOLOCATION -DENABLE_ICONDATABASE -DENABLE_INDEXED_DATABASE -DENABLE_INDEXED_DATABASE_IN_WORKERS -DENABLE_INPUT_TYPE_COLOR -DENABLE_INTERSECTION_OBSERVER -DENABLE_INTL -DENABLE_KEYBOARD_CODE_ATTRIBUTE -DENABLE_KEYBOARD_KEY_ATTRIBUTE -DENABLE_LAYOUT_FORMATTING_CONTEXT -DENABLE_LEGACY_CSS_VENDOR_PREFIXES -DENABLE_LEGACY_CUSTOM_PROTOCOL_MANAGER -DENABLE_LEGACY_ENCRYPTED_MEDIA -DENABLE_MATHML -DENABLE_MEDIA_CONTROLS_SCRIPT -DENABLE_MEDIA_SOURCE -DENABLE_MEDIA_STREAM -DENABLE_METER_ELEMENT -DENABLE_MOUSE_CURSOR_SCALE -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESOURCE_LOAD_STATISTICS -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_WEB_CRYPTO -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_USERSELECT_ALL -DENABLE_VARIATION_FONTS -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_RTC -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WEBMETAL -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.13 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -Wcomma -Wblock-capture-autoreleasing -Wstrict-prototypes -Wrange-loop-analysis -Wno-semicolon-before-method-body -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore -I. -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders --system-header-prefix=unicode/ -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/JavaScriptCorePrefix-erbfwpwlfefulucyeshvvmtfpeno/JavaScriptCorePrefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource21.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource21.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource21.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource21.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource6.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource6.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: https://webkit-queues.webkit.org/results/11085694
Created attachment 361573 [details] Patch for landing
Comment on attachment 361573 [details] Patch for landing cq- while I investigate failing tests.
Comment on attachment 361573 [details] Patch for landing Attachment 361573 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11086255 Number of test failures exceeded the failure limit.
Created attachment 361578 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 361573 [details] Patch for landing Attachment 361573 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11086398 Number of test failures exceeded the failure limit.
Created attachment 361581 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 361573 [details] Patch for landing Attachment 361573 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11086394 Number of test failures exceeded the failure limit.
Created attachment 361584 [details] Archive of layout-test-results from ews113 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 361573 [details] Patch for landing Attachment 361573 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11086317 Number of test failures exceeded the failure limit.
Created attachment 361585 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 361586 [details] Patch for landing
Ugh, these test failures are what I get for following Saam's advice! I fixed it though.
Comment on attachment 361586 [details] Patch for landing Clearing flags on attachment: 361586 Committed r241230: <https://trac.webkit.org/changeset/241230>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47938100>
Comment on attachment 361586 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=361586&action=review > Source/JavaScriptCore/runtime/Operations.h:42 > +constexpr unsigned minRopeStringLength = sizeof(JSRopeString); retrospectively, it'd be nice to make this a JSC option.
Re-opened since this is blocked by bug 194488
I'm looking into this too. I think the issue is that we also apply this rule to JSRopeString with JSString*s. In this case, previously, all we need to create a thing is JSRopeString. JSStrings are already here. At that case, length threshold should be the following until the rope is resolved. sizeof(JSRopeString) v.s. sizeof(JSString) + sizeof(StringImpl + length) Since sizeof(JSRopeString) - sizeof(JSString) is 16, and sizeof(StringImpl) is 24, we always waste a memory. I'm changing this part to see the effect
Created attachment 361890 [details] Patch
Created attachment 361891 [details] Patch
Comment on attachment 361891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361891&action=review > Source/JavaScriptCore/ChangeLog:30 > + This patch also avoids concatenation for ropes conservatively. Many ropes are > + temporary cells. So we do not resolve eagerly if one of operands is already a > + rope. I wonder if this is needed if we add this logic to the DFG/FTL fast path. Did you check that out of curiosity?
Comment on attachment 361891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361891&action=review >> Source/JavaScriptCore/ChangeLog:30 >> + rope. > > I wonder if this is needed if we add this logic to the DFG/FTL fast path. Did you check that out of curiosity? Good point! I did not try this thing yet, but I have mixed thought about introducing this to DFG/FTL. This could offer further great chance to reduce memory footprint. This is really nice! But, on the other hand, MakeRope is typically super fast. So, if MakeRope & ToString (it is sometimes transformed into constant in AI) offer faster performance, I think leaving it as is for DFG/FTL would be a possible implementation. If the code gets DFG/FTL, it means this code is hot or super hot. At that time, we would get different tradeoff between performance penalty and memory consumption. Anyway, I think another measurement is required for introducing this into DFG/FTL :P
Comment on attachment 361891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361891&action=review >>> Source/JavaScriptCore/ChangeLog:30 >>> + rope. >> >> I wonder if this is needed if we add this logic to the DFG/FTL fast path. Did you check that out of curiosity? > > Good point! I did not try this thing yet, but I have mixed thought about introducing this to DFG/FTL. This could offer further great chance to reduce memory footprint. This is really nice! > But, on the other hand, MakeRope is typically super fast. So, if MakeRope & ToString (it is sometimes transformed into constant in AI) offer faster performance, I think leaving it as is for DFG/FTL would be a possible implementation. > If the code gets DFG/FTL, it means this code is hot or super hot. At that time, we would get different tradeoff between performance penalty and memory consumption. > Anyway, I think another measurement is required for introducing this into DFG/FTL :P But looking through the DFG/FTL string concatenation related code, DFG/FTL has StrCat node, which implementation seems inefficient. Since DFG/FTL knows the type information, we have a chance to perform strength reduction, converting StrCat to something like StrCatNonString(String, Int32) etc., and call the function which attempts to perform this concatenation. It sounds like a low-hanging fruit, while I'm not sure whether StrCat node is common.
Comment on attachment 361891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361891&action=review >>>> Source/JavaScriptCore/ChangeLog:30 >>>> + rope. >>> >>> I wonder if this is needed if we add this logic to the DFG/FTL fast path. Did you check that out of curiosity? >> >> Good point! I did not try this thing yet, but I have mixed thought about introducing this to DFG/FTL. This could offer further great chance to reduce memory footprint. This is really nice! >> But, on the other hand, MakeRope is typically super fast. So, if MakeRope & ToString (it is sometimes transformed into constant in AI) offer faster performance, I think leaving it as is for DFG/FTL would be a possible implementation. >> If the code gets DFG/FTL, it means this code is hot or super hot. At that time, we would get different tradeoff between performance penalty and memory consumption. >> Anyway, I think another measurement is required for introducing this into DFG/FTL :P > > But looking through the DFG/FTL string concatenation related code, DFG/FTL has StrCat node, which implementation seems inefficient. > Since DFG/FTL knows the type information, we have a chance to perform strength reduction, converting StrCat to something like StrCatNonString(String, Int32) etc., and call the function which attempts to perform this concatenation. > It sounds like a low-hanging fruit, while I'm not sure whether StrCat node is common. Ooops, I misunderstood your comment. You're mentioning to this `isRope()` check, correct? The reason why we have this `isRope()` check is that we do not want to resolve the rope of the operands eagerly, which increases the memory footprint if the operand is a transient rope (the rope which is just used for the other ropes). So, we conservatively perform this `isRope()` check now. One thing we can do is that traversing the rope of operands, but not resolving the operands themselves. Create JSString and underlying buffer by traversing operand ropes. But do not resolve them. The down side is that this approach traverses ropes eagerly. I think the current `isRope()` check is a bit conservative but it is useful for typical use cases like, "literal" + number. And the current form is good for the first step :) If we find that eager traversing is beneficial, we should consider it. I think the above thing is still valid in DFG/FTL.
Comment on attachment 361891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361891&action=review r=me with fixes. > Source/JavaScriptCore/runtime/Operations.cpp:66 > + if (p2.isCell()) { Typo: this should be: if (p1.isCell()). > Source/JavaScriptCore/runtime/Operations.h:53 > + // JSRopeString + JSString v.s. JSString > + if (s2->isRope() || (length1 + length2) >= sizeof(JSRopeString)) You're comparing 2 case: LHS: make new String vs RHS: make rope. Cost of LHS: sizeof(JSString) (for new string) + sizeof(StringImpl) + length1 + length2. Cost of RHS: sizeof(JSString) (for u1) + sizeof(JSRopeString). Hence, it is beneficial to make the new string only if sizeof(StringImpl) + length1 + length2 < sizeof(JSRopeString). Please add this break down to the comment since without this break down, it is not necessarily clear why you do the comparison this way. > Source/JavaScriptCore/runtime/Operations.h:54 > + return jsString(exec, jsString(&vm, u1), s2); Just use JSRopeString::create() here since we already know we want to create a rope here. > Source/JavaScriptCore/runtime/Operations.h:58 > + const String& u2 = s2->value(exec); > + UNUSED_PARAM(u2); > + RETURN_IF_EXCEPTION(scope, { }); You already know that s2 is not a rope. So, you should never see an exception here: Instead, you can do: scope.assertNoException(); You can also get rid of the UNUSED_PARAM(u2) because you actually use it below. > Source/JavaScriptCore/runtime/Operations.h:59 > + return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); Let's use tryMakeString() instead of makeString(), and throw an OOME if it fails? > Source/JavaScriptCore/runtime/Operations.h:75 > + if (s1->isRope() || (length1 + length2) >= sizeof(JSRopeString)) You're comparing 2 case: LHS: make new String vs RHS: make rope. Cost of LHS: sizeof(JSString) (for new string) + sizeof(StringImpl) + length1 + length2. Cost of RHS: sizeof(JSString) (for u2) + sizeof(JSRopeString). Hence, it is beneficial to make the new string only if sizeof(StringImpl) + length1 + length2 < sizeof(JSRopeString). Ditto: please add breakdown to comment. > Source/JavaScriptCore/runtime/Operations.h:76 > + return jsString(exec, s1, jsString(&vm, u2)); Ditto: just use JSRopeString::create() here. > Source/JavaScriptCore/runtime/Operations.h:80 > + const String& u1 = s1->value(exec); > + UNUSED_PARAM(u1); > + RETURN_IF_EXCEPTION(scope, { }); Don't need UNUSED_PARAM(u1) because you do use u1 below. Because s1 cannot be a rope, replace RETURN_IF_EXCEPTION with scope.assertNoException(); > Source/JavaScriptCore/runtime/Operations.h:81 > + return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); Let's use tryMakeString() instead of makeString(), and throw an OOME if it fails? > Source/JavaScriptCore/runtime/Operations.h:148 > + // JSRopeString + JSString * 2 v.s. JSString > + if (length1 + length2 >= (sizeof(JSRopeString) + sizeof(JSString))) You're comparing 2 case: LHS: make new String vs RHS: make rope. Cost of LHS: sizeof(JSString) (for new string) + sizeof(StringImpl) + length1 + length2. Cost of RHS: sizeof(JSString) (for u1) + sizeof(JSString) (for u2) + sizeof(JSRopeString). Hence, it is beneficial to make the new string only if sizeof(StringImpl) + length1 + length2 < sizeof(JSString) + sizeof(JSRopeString). Ditto: please add breakdown to comment. > Source/JavaScriptCore/runtime/Operations.h:151 > + return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); Let's use tryMakeString() instead of makeString(), and throw an OOME if it fails? > Source/JavaScriptCore/runtime/Operations.h:182 > + // JSRopeString + JSString * 3 v.s. JSString > + if (length1 + length2 + length3 >= (sizeof(JSRopeString) + sizeof(JSString) * 2)) You're comparing 2 case: LHS: make new String vs RHS: make rope. Cost of LHS: sizeof(JSString) (for new string) + sizeof(StringImpl) + length1 + length2 + length3 Cost of RHS: sizeof(JSString) (for u1) + sizeof(JSString) (for u2) + sizeof(JSString) (for u3) + sizeof(JSRopeString). Hence, it is beneficial to make the new string only if sizeof(StringImpl) + length1 + length2 + length3 < sizeof(JSString) + sizeof(JSString) + sizeof(JSRopeString). Ditto: please add breakdown to comment. > Source/JavaScriptCore/runtime/Operations.h:185 > + return JSString::create(*vm, makeString(u1, u2, u3).releaseImpl().releaseNonNull()); Let's use tryMakeString() instead of makeString(), and throw an OOME if it fails?
Comment on attachment 361891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361891&action=review Thanks! Since the rule becomes a bit more conservative, I'll measure the performance again to see the effect. >> Source/JavaScriptCore/runtime/Operations.cpp:66 >> + if (p2.isCell()) { > > Typo: this should be: if (p1.isCell()). Ooops! Right. Fixed. >> Source/JavaScriptCore/runtime/Operations.h:53 >> + if (s2->isRope() || (length1 + length2) >= sizeof(JSRopeString)) > > You're comparing 2 case: LHS: make new String vs RHS: make rope. > Cost of LHS: sizeof(JSString) (for new string) + sizeof(StringImpl) + length1 + length2. > Cost of RHS: sizeof(JSString) (for u1) + sizeof(JSRopeString). > Hence, it is beneficial to make the new string only if sizeof(StringImpl) + length1 + length2 < sizeof(JSRopeString). > > Please add this break down to the comment since without this break down, it is not necessarily clear why you do the comparison this way. Right. I'll fix it and add comments about it. For `sizeof(StringImpl)`, I think we should use StringImpl::allocationSize instead because we emit some characters to the padding of StringImpl. >> Source/JavaScriptCore/runtime/Operations.h:54 >> + return jsString(exec, jsString(&vm, u1), s2); > > Just use JSRopeString::create() here since we already know we want to create a rope here. Nice, fixed. >> Source/JavaScriptCore/runtime/Operations.h:58 >> + RETURN_IF_EXCEPTION(scope, { }); > > You already know that s2 is not a rope. So, you should never see an exception here: Instead, you can do: > scope.assertNoException(); > > You can also get rid of the UNUSED_PARAM(u2) because you actually use it below. Fixed. >> Source/JavaScriptCore/runtime/Operations.h:59 >> + return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); > > Let's use tryMakeString() instead of makeString(), and throw an OOME if it fails? Changed, nice. >> Source/JavaScriptCore/runtime/Operations.h:75 >> + if (s1->isRope() || (length1 + length2) >= sizeof(JSRopeString)) > > You're comparing 2 case: LHS: make new String vs RHS: make rope. > Cost of LHS: sizeof(JSString) (for new string) + sizeof(StringImpl) + length1 + length2. > Cost of RHS: sizeof(JSString) (for u2) + sizeof(JSRopeString). > Hence, it is beneficial to make the new string only if sizeof(StringImpl) + length1 + length2 < sizeof(JSRopeString). > > Ditto: please add breakdown to comment. Fixed. >> Source/JavaScriptCore/runtime/Operations.h:76 >> + return jsString(exec, s1, jsString(&vm, u2)); > > Ditto: just use JSRopeString::create() here. Fixed. >> Source/JavaScriptCore/runtime/Operations.h:80 >> + RETURN_IF_EXCEPTION(scope, { }); > > Don't need UNUSED_PARAM(u1) because you do use u1 below. > Because s1 cannot be a rope, replace RETURN_IF_EXCEPTION with scope.assertNoException(); Fixed. >> Source/JavaScriptCore/runtime/Operations.h:81 >> + return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); > > Let's use tryMakeString() instead of makeString(), and throw an OOME if it fails? Fixed. >> Source/JavaScriptCore/runtime/Operations.h:148 >> + if (length1 + length2 >= (sizeof(JSRopeString) + sizeof(JSString))) > > You're comparing 2 case: LHS: make new String vs RHS: make rope. > Cost of LHS: sizeof(JSString) (for new string) + sizeof(StringImpl) + length1 + length2. > Cost of RHS: sizeof(JSString) (for u1) + sizeof(JSString) (for u2) + sizeof(JSRopeString). > Hence, it is beneficial to make the new string only if sizeof(StringImpl) + length1 + length2 < sizeof(JSString) + sizeof(JSRopeString). > > Ditto: please add breakdown to comment. Fixed. >> Source/JavaScriptCore/runtime/Operations.h:151 >> + return JSString::create(vm, makeString(u1, u2).releaseImpl().releaseNonNull()); > > Let's use tryMakeString() instead of makeString(), and throw an OOME if it fails? Fixed. >> Source/JavaScriptCore/runtime/Operations.h:182 >> + if (length1 + length2 + length3 >= (sizeof(JSRopeString) + sizeof(JSString) * 2)) > > You're comparing 2 case: LHS: make new String vs RHS: make rope. > Cost of LHS: sizeof(JSString) (for new string) + sizeof(StringImpl) + length1 + length2 + length3 > Cost of RHS: sizeof(JSString) (for u1) + sizeof(JSString) (for u2) + sizeof(JSString) (for u3) + sizeof(JSRopeString). > Hence, it is beneficial to make the new string only if sizeof(StringImpl) + length1 + length2 + length3 < sizeof(JSString) + sizeof(JSString) + sizeof(JSRopeString). > > Ditto: please add breakdown to comment. Fixed. >> Source/JavaScriptCore/runtime/Operations.h:185 >> + return JSString::create(*vm, makeString(u1, u2, u3).releaseImpl().releaseNonNull()); > > Let's use tryMakeString() instead of makeString(), and throw an OOME if it fails? Fixed.
Committed r241493: <https://trac.webkit.org/changeset/241493>