Bug 32367

Summary: Add support for short Ropes (up to 3 entries) inline within JSString.
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: JavaScriptCoreAssignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
The patch oliver: review+

Description Gavin Barraclough 2009-12-10 01:27:52 PST
(rather than externally allocating an object to hold the rope).
Switch jsAdd of (JSString* + JSString*) to now make use of Ropes.
Comment 1 Gavin Barraclough 2009-12-10 01:29:28 PST
Created attachment 44601 [details]
The patch
Comment 2 Oliver Hunt 2009-12-10 01:41:44 PST
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?
Comment 3 Gavin Barraclough 2009-12-10 02:07:28 PST
Note the "ASSERT(count >= 3);" – there must be at least three items (this is the minimum for op_strcat).
Comment 4 Oliver Hunt 2009-12-10 02:33:52 PST
Comment on attachment 44601 [details]
The patch

r=me
Comment 5 Mark Rowe (bdash) 2009-12-10 08:27:22 PST
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.
Comment 6 Gavin Barraclough 2009-12-10 14:13:30 PST
Transmitting file data .......
Committed revision 51964.