Bug 194465 – We should only make rope strings when concatenating strings long enough.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

We should only make rope strings when concatenating strings long enough.

[Keith Miller]

Created attachment 361564 [details]
Patch

[Keith Miller]

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.

[Saam Barati]

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

[Keith Miller]

Created attachment 361568 [details]
Patch for landing

[Keith Miller]

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.

[WebKit Commit Bot]

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

[Keith Miller]

Created attachment 361573 [details]
Patch for landing

[Keith Miller]

Comment on attachment 361573 [details]
Patch for landing

cq- while I investigate failing tests.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

Created attachment 361586 [details]
Patch for landing

[Keith Miller]

Ugh, these test failures are what I get for following Saam's advice! I fixed it though.

[WebKit Commit Bot]

Comment on attachment 361586 [details]
Patch for landing

Clearing flags on attachment: 361586

Committed r241230: <https://trac.webkit.org/changeset/241230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47938100>

[Saam Barati]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 194488

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 361890 [details]
Patch

[Yusuke Suzuki]

Created attachment 361891 [details]
Patch

[Keith Miller]

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?

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Mark Lam]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r241493: <https://trac.webkit.org/changeset/241493>