[webkit-reviews] review granted: [Bug 58420] String operator+ reallocates unnecessarily when concatting > 2 strings : [Attachment 93105] Patch v20

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 58420: String operator+ reallocates unnecessarily when concatting > 2
strings
https://bugs.webkit.org/show_bug.cgi?id=58420

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=93105&action=review

Congratulations on getting this to compile!

I’m going to say review+ but I think that some of the changes here are not good
and we need some additional refinement after landing this.

> Source/JavaScriptCore/wtf/text/AtomicString.h:205
>  
> +#include "StringConcatenate.h"
>  #endif // AtomicString_h

I’m glad you finally got this working, but the way all these headers include
other headers remains a mess. For example, there’s no difference between
including AtomicString.h and StringConcatenate.h now; including either always
includes both. I’m OK landing like this, but I think we need to work on
improving this over time.

> Source/JavaScriptCore/wtf/text/StringOperators.h:4
> + * Copyright (C) 1999 Lars Knoll (knoll at kde.org)
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All
rights reserved.
> + * Copyright (C) Research In Motion Limited 2011. All rights reserved.

Given what code was moved to this file, I think you moved too many operators.
I’m pretty sure none of this code was written by Lars.

> Source/JavaScriptCore/wtf/text/WTFString.cpp:961
> +StringAppend<const char*, String> operator+(const char* string1, const
String& string2)
> +{
> +    return StringAppend<const char*, String>(string1, string2);
> +}
> +
> +StringAppend<const char*, AtomicString> operator+(const char* string1, const
AtomicString& string2)
> +{
> +    return StringAppend<const char*, AtomicString>(string1, string2);
> +}

These should be inlines in the header. The StringAppend optimization works
better if the entire thing can be evaluated at compile time and ends compiling
down to a single function call. Having these functions non-inline will create
unwanted function calls.

> Source/JavaScriptCore/wtf/text/WTFString.cpp:967
> +const String& emptyString()
> +{
> +    DEFINE_STATIC_LOCAL(String, emptyString, (""));
> +    return emptyString;
> +}

I think this should return the same value as emptyAtom, not a distinct empty
string.

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:743
> -	   AtomicString attrQName = attrPrefix.isEmpty() ?
toAtomicString(attributes[i].localname) : AtomicString(attrPrefix + ":" +
toString(attributes[i].localname));
> +	   AtomicString attrQName = attrPrefix.isEmpty() ?
toAtomicString(attributes[i].localname) : attrPrefix + colonString +
toString(attributes[i].localname);

This is a step in the wrong direction. The string + operator should support
appending a single character or a char* without first converting it to a
String, so it should not have been necessary to make any code change here at
all.

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

This seems worse than before. Why take a string with "xmlns" and then at
runtime, append a second string with ":" in it, when the xmlns and the ":"
could be concatenated at compile time? We end up concatenating three strings
when we could just concatenate two. It's an improvement to use xmlnsAtom when
we are not concatenating, though.

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

Here it is not sensible to separately concatenate the "." and then the "x".
That's extra work at runtime for no reason. We should just concatenate ".x" as
before. It's fine to use xString for the other side of the ternary operator,
though.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:673
> +	   AtomicString prefixColonLocalName = prefix + colonString +
localName;

Same comment as above about the colon. With makeString we would have been able
to use a single character ':' and avoid having to even call strlen, much less
having to look at the length of a preallocated String object. Why can’t our
operator+ do the same?

> Source/WebCore/loader/CrossOriginAccessControl.cpp:113
> -	   errorDescription = (accessControlOriginString == "*") ? "Cannot use
wildcard in Access-Control-Allow-Origin when credentials flag is true."
> +	   errorDescription = (accessControlOriginString == "*") ?
String("Cannot use wildcard in Access-Control-Allow-Origin when credentials
flag is true.")
>	       : "Origin " + securityOrigin->toString() + " is not allowed by
Access-Control-Allow-Origin.";

Why exactly is this needed now if it wasn’t needed before? The relationship to
the operator+ change is not clear, nor is it explained by the change log. Maybe
the issue is that we have to have one side of the ? : be an explicit String to
make sure that the other side gets converted from StringAdapter to String? If
so, then I suggest getting these changes to explicitly make a String landed
separately before we add the StringAdapter, just to make the StringAdapter
change smaller.

> Source/WebCore/page/NavigatorBase.cpp:90
> +    if (WEBCORE_NAVIGATOR_PLATFORM != emptyString())
>	   return WEBCORE_NAVIGATOR_PLATFORM;

There is an isEmpty function that should be used instead of comparing with an
empty string.

> Source/WebKit/chromium/src/WebHTTPLoadInfo.cpp:108
> -	   result.first->second += String("\n") + value;
> +	   result.first->second += String("\n") + String(value);

This is unnecessarily inefficient. Why turn the "\n" into a String before
concatenating it?

> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:308
> -    result += "<" + element->nodeName().lower();
> +    result += String("<") + element->nodeName().lower();

Seems like a step in the wrong direction. Explicitly making a String out of the
"<" before concatenating.

> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:337
> -				   result += "./" + param->directoryName + "/";

> +				   result += String("./") +
param->directoryName + "/";

Seems like a step in the wrong direction. Explicitly making a String out of the
"./" before concatenating.

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:186
> -    EXPECT_EQ(testStringA, decodeString(v.data(), v.data() + v.size()));
> +    EXPECT_EQ(String(testStringA), decodeString(v.data(), v.data() +
v.size()));
>  
>      v = encodeString(String(testStringB));
> -    EXPECT_EQ(testStringB, decodeString(v.data(), v.data() + v.size()));
> +    EXPECT_EQ(String(testStringB), decodeString(v.data(), v.data() +
v.size()));

Why is this change needed? How is it related to the operator+ change?

> Source/WebKit/mac/WebView/WebFrame.mm:495
> -    return _private->coreFrame->documentTypeString() + markupString;
> +    return String(_private->coreFrame->documentTypeString() +
String(markupString));

I wish we could keep this simpler, but I guess I can live with it like this.