[webkit-reviews] review denied: [Bug 58420] String operator+ reallocates unnecessary when concatting > 2 strings : [Attachment 89409] Patch v5

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 58420: String operator+ reallocates unnecessary when concatting > 2 strings
https://bugs.webkit.org/show_bug.cgi?id=58420

Attachment 89409: Patch v5
https://bugs.webkit.org/attachment.cgi?id=89409&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=89409&action=review

In general this approach looks really good, although I am going to say r- for
now, because I suggested many changes.

There is also a small risk that we could have made the two-string operator+
append a tiny bit slower if compilers are not smart enough to optimize out the
temporary struct. Can you do some performance testing to make sure this is an
improvement or at least not a regression?

> Source/JavaScriptCore/wtf/text/AtomicStringConcatenate.h:37
> +    StringTypeAdapter<AtomicString>(const AtomicString& string)
> +	   : m_buffer(string.string())
> +    {
> +    }

How about making this hold a StringTypeAdapter<String> and just forward to it?
That would let you avoid replicating the implementations of length() and
writeTo()

> Source/JavaScriptCore/wtf/text/StringBuilder.h:148
> +#include "StringOperators.h" // NOLINT

What's NOLINT about?

> Source/JavaScriptCore/wtf/text/WTFString.h:366
> +template<typename T1, typename T2>
> +StringAppend2<T1, T2> operator+(const T1& a, const T2& b)
> +{
> +    return StringAppend2<T1, T2>(a, b);
> +}

I think this is a little risky, because it could apply to any two non-POD
types. That has two issues:

1) It could give a really mysterious error when it fails, probably something
about not being able to instantiate StringAdapter.
2) If you have a class type that represents a numeric value, it might make
sense to add those things numerically, but also be able to append them to
strings. With this very generic template function, if you forget to implement
an operator+ specifically for that type, you'll accidentally get string appends
instead.

What I would suggest is to instead define template functions where either the
first or second argument has to be a String or AtomicString (for four total).

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:713
> +	       namespaceQName = String("xmlns:" +
toString(namespaces[i].prefix));

Maybe instead of this,	you should give StringAppend[N] and StringBuilder
implicit conversions to AtomicString. It seems String already had one so
presumably it can do no harm.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1421
> +		   String string = existingStyle->cssText() +
styleChange.cssStyle();
> +		   setNodeAttribute(styleContainer, styleAttr, string);

This seems like the same AtomicString issue - could avoid this by adding an
AtomicString implicit conversion.

> Source/WebCore/editing/MarkupAccumulator.cpp:209
> -    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : "xmlns";
> +    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix :
String("xmlns");

I wonder if it would be more efficient to have String or AtomicString constants
(as appropriate) defined for strings like "xmlns". Every time through this
code, if the prefix is empty, it converts the "xmlns" to String, creating a
buffer, and then to AtomicString, throwing away the buffer, which is wasteful.
Maybe there already is one? If you did that though, you'd also want the
implicit conversion from StringAppend2 to AtomicString.

> Source/WebCore/html/HTMLAnchorElement.cpp:307
> -    return fragmentIdentifier.isEmpty() ? "" : "#" + fragmentIdentifier;
> +    return fragmentIdentifier.isEmpty() ? String("") : "#" +
fragmentIdentifier;

Maybe we should add an emptyString global, or String::empty(), just like we
have emptyAtom.

> Source/WebCore/html/HTMLAnchorElement.cpp:444
> -    return query.isEmpty() ? "" : "?" + query;
> +    return query.isEmpty() ? String("") : "?" + query;

ditto previous comment about empty string.

> Source/WebCore/html/ImageInputType.cpp:61
> +    encoding.appendData(name.isEmpty() ? String("x") : (name + ".x"),
m_clickLocation.x());
> +    encoding.appendData(name.isEmpty() ? String("y") : (name + ".y"),
m_clickLocation.y());

Maybe it would be better to have defined String constants for "x" and "y".
Probably not a super hot code path though, even when hit (unlike xmlns which
could be very common in XML documents).

> Source/WebCore/page/Location.cpp:121
> -    return url.query().isEmpty() ? "" : "?" + url.query();
> +    return url.query().isEmpty() ? String("") : "?" + url.query();

Again, see previous remarks about empty string.

> Source/WebCore/page/Location.cpp:137
> +    return fragmentIdentifier.isEmpty() ? String("") : "#" +
fragmentIdentifier;

Again, see previous remarks about empty string.

> Source/WebCore/page/NavigatorBase.cpp:92
> -    DEFINE_STATIC_LOCAL(String, platformName, (uname(&osname) >= 0 ?
String(osname.sysname) + String(" ") + String(osname.machine) : ""));
> +    DEFINE_STATIC_LOCAL(String, platformName, (uname(&osname) >= 0 ?
String(osname.sysname) + String(" ") + String(osname.machine) : String("")));

Again, see previous remarks about empty string. Also, some of the String()
explicit calls on the other side of the ?: are probably unnecessary. I think
you only need the first one.

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:260
> -    dataObject->setFileExtension(extension.isEmpty() ? "" : "." +
extension);
> +    dataObject->setFileExtension(extension.isEmpty() ? String("") : "." +
extension);

Another empty string case.

> Source/WebCore/workers/WorkerLocation.cpp:69
> -    return m_url.query().isEmpty() ? "" : "?" + m_url.query();
> +    return m_url.query().isEmpty() ? String("") : "?" + m_url.query();

Empty string yada yada

> Source/WebCore/workers/WorkerLocation.cpp:74
> -    return m_url.fragmentIdentifier().isEmpty() ? "" : "#" +
m_url.fragmentIdentifier();
> +    return m_url.fragmentIdentifier().isEmpty() ? String("") : "#" +
m_url.fragmentIdentifier();

Empty string again.

> Source/WebKit/mac/Misc/WebNSURLExtras.mm:58
> -static uint32_t IDNScriptWhiteList[(USCRIPT_CODE_LIMIT + 31) / 32];
> +static uint32_t IDNScriptWhiteList[(unsigned(USCRIPT_CODE_LIMIT) + 31) /
32];

This is probably the consequence of having the super-generic operator+, as I
warned. I think it would be better to be a little less generic than to have
typecasts like this.