Bug 95416 - Replace uses of WTF::String::operator+= with StringBuilder
Summary: Replace uses of WTF::String::operator+= with StringBuilder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-29 18:32 PDT by Adam Barth
Modified: 2012-08-30 10:16 PDT (History)
3 users (show)

See Also:


Attachments
Patch (13.26 KB, patch)
2012-08-29 18:34 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (13.28 KB, patch)
2012-08-30 01:01 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-08-29 18:32:58 PDT
Replace uses of WTF::String::operator+= with StringBuilder
Comment 1 Adam Barth 2012-08-29 18:34:26 PDT
Created attachment 161384 [details]
Patch
Comment 2 Benjamin Poulain 2012-08-29 19:09:39 PDT
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".
Comment 3 Adam Barth 2012-08-30 01:01:58 PDT
Created attachment 161422 [details]
Patch for landing
Comment 4 WebKit Review Bot 2012-08-30 01:19:40 PDT
Comment on attachment 161422 [details]
Patch for landing

Clearing flags on attachment: 161422

Committed r127112: <http://trac.webkit.org/changeset/127112>
Comment 5 WebKit Review Bot 2012-08-30 01:19:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2012-08-30 09:50:10 PDT
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.
Comment 7 Darin Adler 2012-08-30 09:53:00 PDT
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.
Comment 8 Adam Barth 2012-08-30 10:01:26 PDT
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.
Comment 9 Benjamin Poulain 2012-08-30 10:16:59 PDT
> >> 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.