Bug 32367 – Add support for short Ropes (up to 3 entries) inline within JSString.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

(rather than externally allocating an object to hold the rope).
Switch jsAdd of (JSString* + JSString*) to now make use of Ropes.

[Gavin Barraclough]

Created attachment 44601 [details]
The patch

[Oliver Hunt]

Comment on attachment 44601 [details]
The patch


> +    ALWAYS_INLINE JSValue jsString(ExecState* exec, Register* strings, unsigned count)
> +    {
> +        JSGlobalData* globalData = &exec->globalData();
> +        ASSERT(count >= 3);
> +
> +        unsigned ropeLength = 0;
> +        for (unsigned i = 0; i < count; ++i) {
> +            JSValue v = strings[i].jsValue();
> +            if (LIKELY(v.isString())) {
> +                ropeLength += asString(v)->ropeLength();
> +            } else
> +                ++ropeLength;
> +        }
> +
> +        if (ropeLength == 3)
> +            return new (globalData) JSString(exec, strings[0].jsValue(), strings[1].jsValue(), strings[2].jsValue());

This seems wrong
if i have a two arguments, and one says its ropeLength is 2, then ropeLength will be 3, and we will take this code path which assumes each rope portion is passed on the register file

I think maybe
a = "foo"
a += "bar"
c = "foo" + a
Will trigger this code path?

[Gavin Barraclough]

Note the "ASSERT(count >= 3);" – there must be at least three items (this is the minimum for op_strcat).

[Oliver Hunt]

Comment on attachment 44601 [details]
The patch

r=me

[Mark Rowe (bdash)]

Comment on attachment 44601 [details]
The patch

> Index: JavaScriptCore/runtime/Operations.h
> ===================================================================
> --- JavaScriptCore/runtime/Operations.h	(revision 51933)
> +++ JavaScriptCore/runtime/Operations.h	(working copy)
> @@ -35,6 +35,60 @@ namespace JSC {
>      bool jsIsObjectType(JSValue);
>      bool jsIsFunctionType(JSValue);
>  
> +    ALWAYS_INLINE JSValue jsString(ExecState* exec, JSString* s1, JSString* s2)
> +    {
> +        unsigned ropeLength = s1->ropeLength() + s2->ropeLength();
> +        JSGlobalData* globalData = &exec->globalData();
> +
> +        if (ropeLength <= JSString::s_maxInternalRopeLength)
> +            return new (globalData) JSString(globalData, ropeLength, s1, s2);
> +        else {

There’s no need for the else immediately after a return.

> +            unsigned index = 0;
> +            RefPtr<JSString::Rope> rope = JSString::Rope::createOrNull(ropeLength);
> +            if (UNLIKELY(!rope))
> +                return throwOutOfMemoryError(exec);
> +            rope->append(index, s1);
> +            rope->append(index, s2);
> +            ASSERT(index == ropeLength);
> +            return new (globalData) JSString(globalData, rope.release());
> +        }
> +    }
> +
> +    ALWAYS_INLINE JSValue jsString(ExecState* exec, Register* strings, unsigned count)
> +    {
> +        JSGlobalData* globalData = &exec->globalData();
> +        ASSERT(count >= 3);

It would be clearer to assert about the argument immediately, and to move the global data assignment down to immediately above where it is used.

> +
> +        unsigned ropeLength = 0;
> +        for (unsigned i = 0; i < count; ++i) {
> +            JSValue v = strings[i].jsValue();
> +            if (LIKELY(v.isString())) {
> +                ropeLength += asString(v)->ropeLength();
> +            } else
> +                ++ropeLength;

Braces around a one-line if body?

> +        }
> +
> +        if (ropeLength == 3)
> +            return new (globalData) JSString(exec, strings[0].jsValue(), strings[1].jsValue(), strings[2].jsValue());
> +        else {

There’s no need for an else following a return, which will make the following code easier to read.


> +            return
> +                jsString(callFrame, asString(v1), asString(v2));

Unexpected line break.

[Gavin Barraclough]

Transmitting file data .......
Committed revision 51964.