RESOLVED FIXED 60700
Replace direct StringConcatenate usage, by using operator+ (again)
https://bugs.webkit.org/show_bug.cgi?id=60700
Summary Replace direct StringConcatenate usage, by using operator+ (again)
Nikolas Zimmermann
Reported 2011-05-12 08:00:35 PDT
Now that operator+ is fast for multiple string concats, deploy its usage everywhere. Previously I switched several places to using makeString() directly, revert that.
Attachments
Patch (72.80 KB, patch)
2011-05-12 08:41 PDT, Nikolas Zimmermann
darin: review-
webkit.review.bot: commit-queue-
Patch v2 (66.97 KB, patch)
2011-05-13 00:13 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-01 (1.24 MB, application/zip)
2011-05-13 00:38 PDT, WebKit Review Bot
no flags
Patch v3 (67.01 KB, patch)
2011-05-14 03:54 PDT, Nikolas Zimmermann
krit: review+
krit: commit-queue-
Nikolas Zimmermann
Comment 1 2011-05-12 08:41:44 PDT
Created attachment 93290 [details] Patch With that patch, there's only one direct user of StringConcatenate.h remaining in WebCore: history/PageCache.cpp:#define PCLOG(...) pageCacheLog(pageCacheLogPrefix(indentLevel), makeString(__VA_ARGS__)) Everything else uses the new String append operator.
WebKit Review Bot
Comment 2 2011-05-12 08:50:30 PDT
Comment on attachment 93290 [details] Patch Attachment 93290 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8690659
Early Warning System Bot
Comment 3 2011-05-12 08:51:44 PDT
Darin Adler
Comment 4 2011-05-12 09:39:44 PDT
Comment on attachment 93290 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93290&action=review Generally looks good. I suggest landing a patch that covers the simpler obviously-correct cases first, and then we can slowly consider the ones that require a little more thought. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:120 > - builder.append(makeString('-', toASCIILower(c))); > + builder.append("-" + toASCIILower(c)); Two problems here. First, this code won’t work. It will take the address of the "-" string, and then move forward by a number of characters equal the the lowercase character code. Pointer arithmetic. Second of all, if it did work, we’d be trading an optimized code path to append two character to a general purpose code path that has to call strlen just to get the length of a string literal. > Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:139 > - builder.append(makeString('-', toASCIILower(c))); > + builder.append("-" + toASCIILower(c)); Same problem as above. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:105 > - String message = makeString("Profile \"webkit-profile://", CPUProfileType, '/', encodeWithURLEscapeSequences(title), '#', String::number(profile->uid()), "\" finished."); > + String message = String("Profile \"webkit-profile://") + CPUProfileType + "/" + encodeWithURLEscapeSequences(title) + '#' + String::number(profile->uid()) + "\" finished."; The new code wastefully converts the first literal into a String just so we can concatenate. This is less efficient than the old code. Changing the '/' into a "/", adds an unneeded call to strlen into the mix. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:113 > - String message = makeString("Profile \"webkit-profile://", CPUProfileType, '/', encodeWithURLEscapeSequences(title), "#0\" started."); > + String message = String("Profile \"webkit-profile://") + CPUProfileType + "/" + encodeWithURLEscapeSequences(title) + "#0\" started."; Same problem of allocating an extra string. We don’t want to convert things to String just so we can concatenate them and make the final result into a string. We make sure we preserve a way of doing it that does not require extra memory allocation. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:496 > + String message; > + if (m_document->url().isNull()) > + message = "Unsafe attempt to load URL " + url.string() + '.'; > + else > + message = "Unsafe attempt to load URL " + url.string() + " from frame with URL " + m_document->url().string() + ". Domains, protocols and ports must match.\n"; The ? : avoids creating an empty object and then using the assignment operator, which is less efficient than just initializing. Was it necessary to change from ? : to if?
Gyuyoung Kim
Comment 5 2011-05-12 18:00:05 PDT
Nikolas Zimmermann
Comment 6 2011-05-13 00:07:36 PDT
(In reply to comment #4) > (From update of attachment 93290 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93290&action=review > > Generally looks good. I suggest landing a patch that covers the simpler obviously-correct cases first, and then we can slowly consider the ones that require a little more thought. You are right. > > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:120 > First, this code won’t work. It will take the address of the "-" string, and then move forward by a number of characters equal the the lowercase character code. Pointer arithmetic. Ay ay ay, great catch. > > Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:139 I'll leave out both files from the next patch. > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:105 > The new code wastefully converts the first literal into a String just so we can concatenate. This is less efficient than the old code. Right, I left it out for now. > > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:113 Ditto. > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:496 > > + String message; > > + if (m_document->url().isNull()) > > + message = "Unsafe attempt to load URL " + url.string() + '.'; > > + else > > + message = "Unsafe attempt to load URL " + url.string() + " from frame with URL " + m_document->url().string() + ". Domains, protocols and ports must match.\n"; > > The ? : avoids creating an empty object and then using the assignment operator, which is less efficient than just initializing. Was it necessary to change from ? : to if? If I want to use the ? operator, I have to assure both sides are Strings, so I have to use String("Unsafe...") + url.string()... Compared to that, I think the new solution is better, no?
Nikolas Zimmermann
Comment 7 2011-05-13 00:13:15 PDT
Created attachment 93411 [details] Patch v2
WebKit Review Bot
Comment 8 2011-05-13 00:38:41 PDT
Comment on attachment 93411 [details] Patch v2 Attachment 93411 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8690936 New failing tests: svg/dom/SVGTransformList-basics.xhtml
WebKit Review Bot
Comment 9 2011-05-13 00:38:46 PDT
Created attachment 93414 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Nikolas Zimmermann
Comment 10 2011-05-14 03:54:23 PDT
Created attachment 93552 [details] Patch v3 Fix typo in patch v2, leading to cr-linux tests problem: + case SVG_TRANSFORM_SCALE: { - DEFINE_STATIC_LOCAL(String, scaleString, ("matrix(")); + DEFINE_STATIC_LOCAL(String, scaleString, ("scale("));
Dirk Schulze
Comment 11 2011-05-14 11:07:30 PDT
Comment on attachment 93552 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=93552&action=review r=me with some notes. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:83 > + return sourceID + ":" + String::number(scriptBreakpoint.lineNumber); not ':'? > Source/WebCore/page/DOMWindow.cpp:1702 > + " from frame with URL " + activeWindowURL.string() + ". Domains, protocols and ports must match.\n"; don't we use indention with 4 spaces here? I like your style more, but we should be consistent. > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:56 > + String key = "F" + String::number(i); 'F' instead of "F"? > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:164 > + String key = "F" + String::number(i); Ditto.
Nikolas Zimmermann
Comment 12 2011-05-15 14:23:16 PDT
(In reply to comment #11) > (From update of attachment 93552 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93552&action=review > > r=me with some notes. > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:83 > > + return sourceID + ":" + String::number(scriptBreakpoint.lineNumber); > > not ':'? > > > Source/WebCore/page/DOMWindow.cpp:1702 > > + " from frame with URL " + activeWindowURL.string() + ". Domains, protocols and ports must match.\n"; > > don't we use indention with 4 spaces here? I like your style more, but we should be consistent. Fixed. > > > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:56 > > + String key = "F" + String::number(i); > > 'F' instead of "F"? > > > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:164 > > + String key = "F" + String::number(i); > > Ditto. That doesn't build see efl build output from patch v1.
Nikolas Zimmermann
Comment 13 2011-05-16 01:00:37 PDT
Landed in r86542 and r86543 (forgot one file).
Joseph Pecoraro
Comment 14 2011-09-28 11:43:19 PDT
This introduced a subtle issue, filed at: <http://webkit.org/b/69011> FTPDirectoryDocument Shows Garbled String for Last Modified Date
Note You need to log in before you can comment on or make changes to this bug.