Bug 15837 – AddNode causes excessive mallocs due to add() using toString(exec)

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

[Eric Seidel (no email)]

AddNode causes excessive mallocs due to add() using toString(exec)

add(exec, value1, value2) uses toString(exec) to create a UString from a StringImp.  This is quite unfortunate as it causes creation of unnecessary UStrings which are the immediately discarded.  It would be more efficient to static cast to a StringImp* and call value() directly.  I'm not 100% sure that's safe however. (I'm not sure if the design is supposed to allow other subclasses of JSValue to return true for isString()... I don't think it should.)

[Eric Seidel (no email)]

Hum... a simple static_cast won't work, since only one of the values needs to be a string for add to work with strings.  Might require some thinking to do this w/o slowing things down w/ more branches.

[Eric Seidel (no email)]

Created attachment 17059 [details]
one possible fix (seems to be a wash, sadly)

[Eric Seidel (no email)]

Maciej points out that this patch only works by chance... sizeof(JSType) is only accidentally correct.

[Eric Seidel (no email)]

Maciej made a second suggestion of trying something like this:

JSType t1 = v1->type();
JSType t2 = v2->type();

if (t1 == NumberType && t2 == NumberType)
    return v1->toNumber(exec) + v2->toNumber();
else if (t1 == StringType && t2 == StringType)
   return static_cast<StringImp*>(v1)->value() + static_cast<StringImp*>(v2)->value();

return addSlowCase(v1, v2);

[Maciej Stachowiak]

Actually, my suggestion is to still have the type mask (&& means extra branches means suckage), I would still generate the combo type code and maybe pass it to the slow case function, but check the two hot paths first as you have it.

[Eric Seidel (no email)]

Created attachment 17062 [details]
another fix (0.8% speed speedup according to SunSpider)

[Eric Seidel (no email)]

Hum... I hadn't seen maciej's comment before I wrote this patch... I guess there is probably still room for improvement. :)  I'm not gonna look a gift 0.8% in the mouth though.

[Darin Adler]

Comment on attachment 17062 [details]
another fix (0.8% speed speedup according to SunSpider)

+static inline JSValue* throwOutOfMemoryError(ExecState* exec)

I don't think this should be inline.

     if (p1->isString() || p2->isString()) {
         UString value = p1->toString(exec) + p2->toString(exec);

Can't those use value() too? I know this is the slow case, but it would be nice to code it tighter.

+        if (value.isNull())
+            return throwOutOfMemoryError(exec);
+        else
             return jsString(value);

A nice opportunity to get rid of an else after return.

+    if (t1 == NumberType && t2 == NumberType)
+        return jsNumber(v1->toNumber(exec) + v2->toNumber(exec));
+    else if (t1 == StringType && t2 == StringType) {

No else after return.

+    } else if (t1 == StringType) { // common js idium "" + object

Misspelling -- it's idiom.

review+, assuming you'll make throwOutOfMemoryError not be inline.

[Eric Seidel (no email)]

(In reply to comment #8)
> (From update of attachment 17062 [details] [edit])
> +static inline JSValue* throwOutOfMemoryError(ExecState* exec)
> 
> I don't think this should be inline.

Fixed.  (Made the speedup 1.1% instead of 0.8%)

>      if (p1->isString() || p2->isString()) {
>          UString value = p1->toString(exec) + p2->toString(exec);
> 
> Can't those use value() too? I know this is the slow case, but it would be nice
> to code it tighter.

Well, I'd have to break it out into individual cases.  But yes, the same idiom could be repeated.

> +        if (value.isNull())
> +            return throwOutOfMemoryError(exec);
> +        else
>              return jsString(value);
> 
> A nice opportunity to get rid of an else after return.

Done.

> +    if (t1 == NumberType && t2 == NumberType)
> +        return jsNumber(v1->toNumber(exec) + v2->toNumber(exec));
> +    else if (t1 == StringType && t2 == StringType) {
> 
> No else after return.

I tried that... (and removing the one after that next if, which also always returns) and SunSpider claimed that my speedup went from 1.1% to 0.5% :(  Maybe it was just variance, but I ran the test several times.  I did not look at the generated assembly.

> +    } else if (t1 == StringType) { // common js idium "" + object
> 
> Misspelling -- it's idiom.

Yeah... eventually Xcode might have comment spell checking. :)

> review+, assuming you'll make throwOutOfMemoryError not be inline.

Thanks.

[Eric Seidel (no email)]

Landed in r27482.  I filed 15860 to cover any further speedups.