Bug 95416 – Replace uses of WTF::String::operator+= with StringBuilder

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Replace uses of WTF::String::operator+= with StringBuilder

[Adam Barth]

Created attachment 161384 [details]
Patch

[Benjamin Poulain]

Comment on attachment 161384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161384&action=review

Looks good. Just one comment:

> Source/WebCore/platform/network/ResourceRequestBase.cpp:377
> -        result.iterator->second += "," + value;
> +        result.iterator->second.append("," + value);

This change will cause 2 concatenation:
1) the string operators will create a new String for ("," + value), then String::append() will create a new string for the result.

If you do (result.iterator->second = result.iterator->second + ',' + value), you only get one "operation".

[Adam Barth]

Created attachment 161422 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 161422 [details]
Patch for landing

Clearing flags on attachment: 161422

Committed r127112: <http://trac.webkit.org/changeset/127112>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 161422 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=161422&action=review

> Source/WebCore/ChangeLog:12
> +        StringBuilder. Eventually, I'd like to remove operator+= so that more
> +        code doesn't fall into this trap.

A great idea.

[Darin Adler]

Comment on attachment 161422 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=161422&action=review

> Source/WebCore/html/HTMLAnchorElement.cpp:148
> +    url.append(String::number(x));

StringBuilder should offer a way to do this operation without creating and destroying a StringImpl.

> Source/WebCore/html/HTMLAnchorElement.cpp:514
> -    String url = stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr));
> +    StringBuilder url;
> +    url.append(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)));
>      appendServerMapMousePosition(url, event);
> -    KURL kurl = document()->completeURL(url);
> +    KURL kurl = document()->completeURL(url.toString());

The by-far-most-common case is to not append a mouse position. I wonder if StringBuilder adds unwanted overhead here.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:377
> -        result.iterator->second += "," + value;
> +        result.iterator->second = result.iterator->second + ',' + value;

This case is interesting, because in theory we could change the String class so that this idiom generated efficient code. I don’t think the new form reads better than the old.

[Adam Barth]

Comment on attachment 161422 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=161422&action=review

>> Source/WebCore/html/HTMLAnchorElement.cpp:148
>> +    url.append(String::number(x));
> 
> StringBuilder should offer a way to do this operation without creating and destroying a StringImpl.

Yes!

>> Source/WebCore/html/HTMLAnchorElement.cpp:514
>> +    KURL kurl = document()->completeURL(url.toString());
> 
> The by-far-most-common case is to not append a mouse position. I wonder if StringBuilder adds unwanted overhead here.

Take a look at line 60 and onwards of StringBuilder::append:

http://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/StringBuilder.h#L53

My understanding is that in this case, we'll just retain the string and then return it, so the overhead looks akin to storing the string in a local variable.  I might not be completely understanding how StringBuilder works however.

>> Source/WebCore/platform/network/ResourceRequestBase.cpp:377
>> +        result.iterator->second = result.iterator->second + ',' + value;
> 
> This case is interesting, because in theory we could change the String class so that this idiom generated efficient code. I don’t think the new form reads better than the old.

Yeah, the new code is kind of ugly.  I wasn't sure exactly what to do in this case, which is why I originally changed it to call append().  I can revert this part of the patch if you like.

[Benjamin Poulain]

> >> Source/WebCore/platform/network/ResourceRequestBase.cpp:377
> >> +        result.iterator->second = result.iterator->second + ',' + value;
> > 
> > This case is interesting, because in theory we could change the String class so that this idiom generated efficient code. I don’t think the new form reads better than the old.
> 
> Yeah, the new code is kind of ugly.  I wasn't sure exactly what to do in this case, which is why I originally changed it to call append().  I can revert this part of the patch if you like.

I also considered this when checking the patch.

We could have operator+=() taking a StringAppend. But I think removing String::operator+=() is a good goal. Removing the slow operations will force future patches to consider string operations more carefully.