Bug 60700 – Replace direct StringConcatenate usage, by using operator+ (again)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Now that operator+ is fast for multiple string concats, deploy its usage everywhere. Previously I switched several places to using makeString() directly, revert that.

[Nikolas Zimmermann]

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 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 on attachment 93290 [details]
Patch

Attachment 93290 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8662011

[Darin Adler]

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 on attachment 93290 [details]
Patch

Attachment 93290 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8685928

[Nikolas Zimmermann]

(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]

Created attachment 93411 [details]
Patch v2

[WebKit Review Bot]

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]

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]

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 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]

(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]

Landed in r86542 and r86543 (forgot one file).

[Joseph Pecoraro]

This introduced a subtle issue, filed at:
<http://webkit.org/b/69011> FTPDirectoryDocument Shows Garbled String for Last Modified Date