Bug 32367 - Add support for short Ropes (up to 3 entries) inline within JSString.
Summary: Add support for short Ropes (up to 3 entries) inline within JSString.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-10 01:27 PST by Gavin Barraclough
Modified: 2009-12-10 14:13 PST (History)
0 users

See Also:


Attachments
The patch (25.57 KB, patch)
2009-12-10 01:29 PST, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.