[webkit-reviews] review granted: [Bug 194465] We should only make rope strings when concatenating strings long enough. : [Attachment 361564] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 194465: We should only make rope strings when concatenating strings long
enough.
https://bugs.webkit.org/show_bug.cgi?id=194465

Attachment 361564: Patch

https://bugs.webkit.org/attachment.cgi?id=361564&action=review




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 361564
  --> https://bugs.webkit.org/attachment.cgi?id=361564
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