(rather than externally allocating an object to hold the rope). Switch jsAdd of (JSString* + JSString*) to now make use of Ropes.
Created attachment 44601 [details] The patch
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?
Note the "ASSERT(count >= 3);" – there must be at least three items (this is the minimum for op_strcat).
Comment on attachment 44601 [details] The patch r=me
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.
Transmitting file data ....... Committed revision 51964.