Replace uses of WTF::String::operator+= with StringBuilder
Created attachment 161384 [details] Patch
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".
Created attachment 161422 [details] Patch for landing
Comment on attachment 161422 [details] Patch for landing Clearing flags on attachment: 161422 Committed r127112: <http://trac.webkit.org/changeset/127112>
All reviewed patches have been landed. Closing bug.
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 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 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.
> >> 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.