Bug 194465 - We should only make rope strings when concatenating strings long enough.
Summary: We should only make rope strings when concatenating strings long enough.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on: 194488
Blocks: 193606
  Show dependency treegraph
 
Reported: 2019-02-08 16:48 PST by Keith Miller
Modified: 2019-02-14 13:39 PST (History)
8 users (show)

See Also:


Attachments
Patch (10.06 KB, patch)
2019-02-08 16:53 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (10.67 KB, patch)
2019-02-08 17:21 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (10.60 KB, patch)
2019-02-08 17:45 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (434.36 KB, application/zip)
2019-02-08 18:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (742.35 KB, application/zip)
2019-02-08 19:10 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (1.32 MB, application/zip)
2019-02-08 19:15 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (6.34 MB, application/zip)
2019-02-08 19:18 PST, Build Bot
no flags Details
Patch for landing (10.70 KB, patch)
2019-02-08 19:19 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (13.04 KB, patch)
2019-02-12 21:23 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.22 KB, patch)
2019-02-12 21:28 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2019-02-08 16:48:39 PST
We should only make rope strings when concatenating strings long enough.
Comment 1 Keith Miller 2019-02-08 16:53:25 PST
Created attachment 361564 [details]
Patch
Comment 2 Keith Miller 2019-02-08 16:59:11 PST
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 3 Saam Barati 2019-02-08 17:01:03 PST
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
Comment 4 Keith Miller 2019-02-08 17:21:33 PST
Created attachment 361568 [details]
Patch for landing
Comment 5 Keith Miller 2019-02-08 17:28:24 PST
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 6 WebKit Commit Bot 2019-02-08 17:45:11 PST
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
Comment 7 Keith Miller 2019-02-08 17:45:46 PST
Created attachment 361573 [details]
Patch for landing
Comment 8 Keith Miller 2019-02-08 18:19:50 PST
Comment on attachment 361573 [details]
Patch for landing

cq- while I investigate failing tests.
Comment 9 Build Bot 2019-02-08 18:44:34 PST
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.
Comment 10 Build Bot 2019-02-08 18:44:35 PST
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 11 Build Bot 2019-02-08 19:10:10 PST
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.
Comment 12 Build Bot 2019-02-08 19:10:12 PST
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 13 Build Bot 2019-02-08 19:15:21 PST
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.
Comment 14 Build Bot 2019-02-08 19:15:23 PST
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 15 Build Bot 2019-02-08 19:18:14 PST
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.
Comment 16 Build Bot 2019-02-08 19:18:16 PST
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
Comment 17 Keith Miller 2019-02-08 19:19:11 PST
Created attachment 361586 [details]
Patch for landing
Comment 18 Keith Miller 2019-02-08 19:21:50 PST
Ugh, these test failures are what I get for following Saam's advice! I fixed it though.
Comment 19 WebKit Commit Bot 2019-02-08 19:56:39 PST
Comment on attachment 361586 [details]
Patch for landing

Clearing flags on attachment: 361586

Committed r241230: <https://trac.webkit.org/changeset/241230>
Comment 20 WebKit Commit Bot 2019-02-08 19:56:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2019-02-08 19:57:33 PST
<rdar://problem/47938100>
Comment 22 Saam Barati 2019-02-10 19:50:59 PST
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.
Comment 23 WebKit Commit Bot 2019-02-10 20:07:09 PST
Re-opened since this is blocked by bug 194488
Comment 24 Yusuke Suzuki 2019-02-12 15:08:39 PST
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
Comment 25 Yusuke Suzuki 2019-02-12 21:23:48 PST
Created attachment 361890 [details]
Patch
Comment 26 Yusuke Suzuki 2019-02-12 21:28:19 PST
Created attachment 361891 [details]
Patch
Comment 27 Keith Miller 2019-02-12 22:33:41 PST
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 28 Yusuke Suzuki 2019-02-12 23:26:31 PST
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 29 Yusuke Suzuki 2019-02-12 23:32:14 PST
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 30 Yusuke Suzuki 2019-02-13 00:41:19 PST
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 31 Mark Lam 2019-02-13 16:24:32 PST
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 32 Yusuke Suzuki 2019-02-13 17:44:47 PST
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.
Comment 33 Yusuke Suzuki 2019-02-13 19:01:31 PST
Committed r241493: <https://trac.webkit.org/changeset/241493>