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

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki 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 361891: Patch

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




--- Comment #31 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 361891
  --> https://bugs.webkit.org/attachment.cgi?id=361891
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?