String operator+ reallocates unnecessarily when concatting > 2 strings

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

String operator+ reallocates too often when concatting > 2 strings.

void String::append(const String& str)
{
    if (str.isEmpty())
       return;

    // FIXME: This is extremely inefficient. So much so that we might want to take this
    // out of String's API. We can make it better by optimizing the case where exactly
    // one String is pointing at this StringImpl, but even then it's going to require a
    // call to fastMalloc every single time.
    if (str.m_impl) {
        if (m_impl) {
            UChar* data;
            if (str.length() > numeric_limits<unsigned>::max() - m_impl->length())
                CRASH();
            RefPtr<StringImpl> newImpl = StringImpl::createUninitialized(m_impl->length() + str.length(), data);
            memcpy(data, m_impl->characters(), m_impl->length() * sizeof(UChar));
            memcpy(data + m_impl->length(), str.characters(), str.length() * sizeof(UChar));
            m_impl = newImpl.release();
        } else
            m_impl = str.m_impl;
    }
}

Currently String::append is used when invoking operator+:

String operator+(const String& a, const String& b)
{
    if (a.isEmpty())
        return b;
    if (b.isEmpty())
        return a;
    String c = a;
    c += b;
    return c;
}

In bug 57840, 57843 I worked on converting WebCore to use StringBuilder/StringConcatenate, where possible. Though it become apparent that the better approach is to fix operator+ so it makes use of makeString/StringBuilder internally, instead of having to change lots of call-sites currently using operator+.

The idea behind the StringAppend2 template trick, as seen in the patch, is from Maciej, and it's great!

[Nikolas Zimmermann]

Created attachment 89357 [details]
Patch

Uploading for discussion and EWS results. As symbols changed in JSC, I expect breakage on win.
Works just fine on mac. I wish I could measure performance, if and how much impact this is. Maybe apple folks can run PLT?

[Nikolas Zimmermann]

Created attachment 89358 [details]
Patch v2

Oops, I left in some local unrelated WebCore.xcodeproj changes.

[WebKit Review Bot]

Attachment 89358 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/text/StringBuilder.h:148:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

(In reply to comment #3)
> Attachment 89358 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> 
> Source/JavaScriptCore/wtf/text/StringBuilder.h:148:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 25 files
I'll add // NOLINT to suppress this warning. Uploading next version, after all EWS results are available.

[Early Warning System Bot]

Attachment 89358 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8397199

[WebKit Review Bot]

Attachment 89358 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8398215

[Build Bot]

Attachment 89358 [details] did not build on win:
Build output: http://queues.webkit.org/results/8399218

[Nikolas Zimmermann]

Created attachment 89371 [details]
Patch v3

Fix style and hopefully the linux/win builds. Gtk and EFL ews are unrepsonsive.

[Gustavo Noronha (kov)]

Attachment 89371 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8403222

[WebKit Review Bot]

Attachment 89371 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8404242

[WebKit Review Bot]

Attachment 89358 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8399258

[Build Bot]

Attachment 89371 [details] did not build on win:
Build output: http://queues.webkit.org/results/8402258

[Nikolas Zimmermann]

Created attachment 89377 [details]
Patch v4

More build fixes...

[WebKit Review Bot]

Attachment 89377 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8397279

[Nikolas Zimmermann]

Created attachment 89409 [details]
Patch v5

Trying to fix cr-linux build, cr-mac still didn't build.

[WebKit Review Bot]

Attachment 89377 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8399368

[Adam Roben (:aroben)]

Why not just use StringConcatenate.h directly?

[Nikolas Zimmermann]

(In reply to comment #17)
> Why not just use StringConcatenate.h directly?

See bugs https://bugs.webkit.org/show_bug.cgi?id=57840 and https://bugs.webkit.org/show_bug.cgi?id=57843, where I started to move WebCore/ towards using StringBuilder/StringConcatenate directly.
Maciej suggested to optimize our operator+ instead, to aviod having to change all call sites that just concat 2 strings, that was the initial motivation.

I wanted to make WebCore use StringBuilder/Concatenate directly, though Maciej convinved me this direction would be better.

[Maciej Stachowiak]

Comment on attachment 89409 [details]
Patch v5

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.

[Nikolas Zimmermann]

(In reply to comment #19)
> (From update of attachment 89409 [details])
> 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.
Sure!
 
> 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?
Hm, do you have suggestions on performance testing? Did you think about sharking? Or a micro benchmark? Not sure what's the best way to measure performance here.
 
> > 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()
Good idea, fixed.
 
> > Source/JavaScriptCore/wtf/text/StringBuilder.h:148
> > +#include "StringOperators.h" // NOLINT
> 
> What's NOLINT about?
It suppresses style bot messages, as this include order error is intentional.
 
> > 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.
Yeah, i've seen this as well, before making adjustments of the operator+ usage in several places.

> 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).
Great suggestion, fixed.

> 
> > 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.
Fixed.

> 
> > 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.
Fixed.

> 
> > 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.
We have XMLNSNames::xmlnsAttr as QualifiedName, we can access the "xmlns" string by using localName(), which gives an AtomicString, should be cheaper.
    AtomicString attr = prefix.isEmpty() ? xmlnsAttr.localName() : xmlnsAttr.localName() + ":" + prefix; 

> 
> > 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.
How about an emptyString() free function returning a const String&, that's declared using DEFINE_STATIC_LOCAL?


> > Source/WebCore/html/HTMLAnchorElement.cpp:444
> > -    return query.isEmpty() ? "" : "?" + query;
> > +    return query.isEmpty() ? String("") : "?" + query;
> 
> ditto previous comment about empty string.
Now uses emptyString() here.
 
> > 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).
Right, I left it this way.

> 
> > 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.
Fixed.

> > Source/WebCore/page/Location.cpp:137
> > +    return fragmentIdentifier.isEmpty() ? String("") : "#" + fragmentIdentifier;
> 
> Again, see previous remarks about empty string.
Fixed.
 
> > 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.
Okay, will check.
 
> > Source/WebCore/platform/chromium/ClipboardChromium.cpp:260
> > -    dataObject->setFileExtension(extension.isEmpty() ? "" : "." + extension);
> > +    dataObject->setFileExtension(extension.isEmpty() ? String("") : "." + extension);
> 
> Another empty string case.
> Empty string yada yada
> Empty string again.
Fixed.

> 
> > 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.
Definately, fixed.

Going to rebuild from scratch, and then upload a new patch. Thanks a lot for the review!

[Nikolas Zimmermann]

Created attachment 91328 [details]
Patch v6

Adressed Maciejs comments.
I've tried a simple microbenchmark, concatting two Strings, several thousands of times, and couldn't measure any difference between the new and the old method. Though that's not really accurate, but I think we won't see any performance regressions for the simple two string concat case, and only wins for n>2 concats.

Could we verify the theory that nothing changed using a post-commit PLT run from Apple?
P.S. I bet we'll need a Patch v7 as I need to adjust win symbol export table after I get the EWS results.

[WebKit Review Bot]

Attachment 91328 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8520059

[Early Warning System Bot]

Attachment 91328 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8520061

[Collabora GTK+ EWS bot]

Attachment 91328 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8516157

[Build Bot]

Attachment 91328 [details] did not build on win:
Build output: http://queues.webkit.org/results/8514317

[Nikolas Zimmermann]

Created attachment 91344 [details]
Patch v7

Forgot to include a change to rendering/RenderCounter.cpp, should fix Qt/Win build.
Adding another AtomicString.h include to StringOperators.h, which hopefully fixes cr-linux builds.

[WebKit Review Bot]

Attachment 91344 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8511473

[Early Warning System Bot]

Attachment 91344 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8518087

[Nikolas Zimmermann]

Created attachment 91358 [details]
Patch v8

More build fixes...

[Build Bot]

Attachment 91344 [details] did not build on win:
Build output: http://queues.webkit.org/results/8520087

[WebKit Review Bot]

Attachment 91358 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8518100

[Nikolas Zimmermann]

Created attachment 91364 [details]
Patch v9

More build fixes...

[WebKit Review Bot]

Attachment 91328 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8519080

[Early Warning System Bot]

Attachment 91358 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8514339

[WebKit Review Bot]

Attachment 91364 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8511503

[Gavin Barraclough]

Comment on attachment 91364 [details]
Patch v9

Hey Niko,

I like the patch! - this seems like a great implementation to keep the nice clean + operator while giving it a fast implementation under the hood.  I have one concern, and one suggestion.

My concern is the use of DEFINE_STATIC_LOCAL with strings.  This is fine for the empty string – this has a special impl that is thread safe, but all other strings are not, and adding static locals with make these methods unsafe to call from other threads.  If you believe this is okay for these methods, then I think we should at least add an "ASSERT(isMainThread());" to guard.

Looking at this, the approach of bailing out after 5 appends doesn't seem terrible, but I think I've seen a couple of hot string adds that append more than 4 times.  Another possibility would be to tweak your code to take a recursive approach - define the LHS of a StringAppend be able to be either a String, or another string append.  The code would effectively recurse summing the sizes of the StringAppends (though this would all be inlined, so it would just be summing the string adaptors sizes), then allocate a buffer, then recurse over the structure writing characters to a buffer.  I've hacked up a quick implementation outside of WebCore to toy with the idea, I'll upload a copy so you can see what you think of the idea.

I'll r- this patch for now, really with respect the the thread safety concern re use of DEFINE_STATIC_LOCAL, but really like this idea in principal.

cheers,
G.

[Gavin Barraclough]

Created attachment 91383 [details]
Example of recursive implementation suggestion.

[WebKit Review Bot]

Attachment 91364 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8517191

[WebKit Review Bot]

Attachment 91364 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8511610

[Nikolas Zimmermann]

Gavin, thanks for the review and the ToyRecursive concept - I think I'll switch to it before submitting another patch, as it's more elegant, and avoids the StringBuilder-fallback.

[Nikolas Zimmermann]

Created attachment 91821 [details]
Patch v10

A new approach, using Gavins recursive template idea, works like a charme, and looks more beautiful than the first approach.
Builds fine on my mac, from scratch, no regressions in the layout tests.

[Early Warning System Bot]

Attachment 91821 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8531342

[WebKit Review Bot]

Attachment 91821 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8526451

[Build Bot]

Attachment 91821 [details] did not build on win:
Build output: http://queues.webkit.org/results/8521908

[Nikolas Zimmermann]

Created attachment 91823 [details]
Patch v11

Oops, forgot to include the changes in Source/WebCore/ChangeLog.
Added an inline variant of the operator+ for two Strings, hopefully fixes Qt build, which sees QString operator+ clashes.

[Nikolas Zimmermann]

Created attachment 91824 [details]
Patch v12

Previous patch had a merge problem.

[Early Warning System Bot]

Attachment 91823 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8526460

[WebKit Review Bot]

Attachment 91823 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8530356

[Collabora GTK+ EWS bot]

Attachment 91821 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8531350

[Early Warning System Bot]

Attachment 91824 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8523745

[Build Bot]

Attachment 91824 [details] did not build on win:
Build output: http://queues.webkit.org/results/8523753

[WebKit Review Bot]

Attachment 91821 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8531358

[WebKit Review Bot]

Attachment 91823 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8528367

[WebKit Review Bot]

Attachment 91821 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8530371

[Nikolas Zimmermann]

Created attachment 91826 [details]
Patch v13

More build fixes...

[Early Warning System Bot]

Attachment 91826 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8529377

[Nikolas Zimmermann]

Does anyone have a suggestion how to fix the Qt build for "Patch v13". There are operator+ clashes with QString. I'm not sure why QString is included at all when building css/CSS*.cpp files, but it appears to be and cause clashes with the operator+ defined in StringOperators.h.

I hope ossy or any szeged guys could have a look?

[Csaba Osztrogonác]

Created attachment 91831 [details]
workaround for Patch v13 to make Qt build happy

I have a quick look at the Qt build, and attached a quick workaround.
I know it isn't a good solution for this problem, but it can show
what happened. It seems, that the problem is around operator+ when
the left side is char* and the right side is String. I think the
compiler can't determine which operator+ should be used here:
a) const QString operator+(const char*, const QString&)
b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)

I hope it can help you a little bit. But unfortunately I don't 
have more time on sunday evening to find a better solution.

[Build Bot]

Attachment 91826 [details] did not build on win:
Build output: http://queues.webkit.org/results/8530405

[Nikolas Zimmermann]

(In reply to comment #58)
> Created an attachment (id=91831) [details]
> workaround for Patch v13 to make Qt build happy
> 
> I have a quick look at the Qt build, and attached a quick workaround.
> I know it isn't a good solution for this problem, but it can show
> what happened. It seems, that the problem is around operator+ when
> the left side is char* and the right side is String. I think the
> compiler can't determine which operator+ should be used here:
> a) const QString operator+(const char*, const QString&)
> b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)
> 
> I hope it can help you a little bit. But unfortunately I don't 
> have more time on sunday evening to find a better solution.

Thanks a lot for your help! Is the QString include really intended when building WebCore (non Qt specific files)? If we'd remove that, we wouldn't need your changes, and I'd really like to avoid hacks like those.

I'm not yet sure why the win build remains broken, any ideas here? CC'ing Adam Roben.

[Adam Roben (:aroben)]

(In reply to comment #59)
> Attachment 91826 [details] did not build on win:
> Build output: http://queues.webkit.org/results/8530405

Here's the first error:

6>c:\cygwin\home\buildbot\webkitbuild\debug\include\private\javascriptcore\StringOperators.h(74) : error C2143: syntax error : missing ';' before '<'

I have no idea what's causing this! Hopefully I'll have some time later today to try building this patch myself.

[Nikolas Zimmermann]

(In reply to comment #61)
> (In reply to comment #59)
> > Attachment 91826 [details] [details] did not build on win:
> > Build output: http://queues.webkit.org/results/8530405
> 
> Here's the first error:
> 
> 6>c:\cygwin\home\buildbot\webkitbuild\debug\include\private\javascriptcore\StringOperators.h(74) : error C2143: syntax error : missing ';' before '<'
> 
> I have no idea what's causing this! Hopefully I'll have some time later today to try building this patch myself.

template<typename StringType1, typename StringType2>
class StringTypeAdapter<StringAppend<StringType1, StringType2> > {

I'm not sure where it exactly fails. StringOperators.h should include StringConcatenate.h which defines StringTypeAdapter. I hope you'll find it out using a local build.

[Zoltan Herczeg]

According to g++ -H, the qstring.h is included:

..... ../../../Source/JavaScriptCore/wtf/Vector.h
...... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/QDataStream
....... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qdatastream.h
........ /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qiodevice.h
......... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qobject.h
.......... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qstring.h

In short, Vector.h has this define:

#if PLATFORM(QT)
#include <QDataStream>
#endif

which pulls the qstring.h

[Nikolas Zimmermann]

(In reply to comment #63)
> According to g++ -H, the qstring.h is included:
> 
> ..... ../../../Source/JavaScriptCore/wtf/Vector.h
> ...... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/QDataStream
> ....... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qdatastream.h
> ........ /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qiodevice.h
> ......... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qobject.h
> .......... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qstring.h
> 
> In short, Vector.h has this define:
> 
> #if PLATFORM(QT)
> #include <QDataStream>
> #endif
> 
> which pulls the qstring.h

Possible workaround for the Qt port that doesn't involve changing WebCore files (as Ossys workaround does).
I've noticed that qstring.h has a conditon around operator+ (#ifndef QT_USE_FAST_OPERATOR_PLUS), so we might workaround this, by disabling the QString operator+ completly when building within WebCore.

How about defining QT_USE_FAST_OPERATOR_PLUS then including QDataStream?
Can someone with a Qt build try?

Thanks in advance!

[Darin Adler]

I’d like our operator+ to be compatible with the QString operator+. I don’t understand why they end up conflicting. All the discussion here has been about avoiding including the QString header; I don’t see any discussion or details about the actual conflict. As long as one of the two arguments to operator+ is a String or something else from WebCore I’d be surprised if it conflicted with the QString operator.

[Nikolas Zimmermann]

 (In reply to comment #65)
> I’d like our operator+ to be compatible with the QString operator+. I don’t understand why they end up conflicting. All the discussion here has been about avoiding including the QString header; I don’t see any discussion or details about the actual conflict. As long as one of the two arguments to operator+ is a String or something else from WebCore I’d be surprised if it conflicted with the QString operator.

Hey Darin,

I think comment 58 explains it well:

> I have a quick look at the Qt build, and attached a quick workaround.
> I know it isn't a good solution for this problem, but it can show
> what happened. It seems, that the problem is around operator+ when
> the left side is char* and the right side is String. I think the
> compiler can't determine which operator+ should be used here:
> a) const QString operator+(const char*, const QString&)
> b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)

I guess you missed that comment.

[Adam Roben (:aroben)]

(In reply to comment #58)
> Created an attachment (id=91831) [details]
> workaround for Patch v13 to make Qt build happy
> 
> I have a quick look at the Qt build, and attached a quick workaround.
> I know it isn't a good solution for this problem, but it can show
> what happened. It seems, that the problem is around operator+ when
> the left side is char* and the right side is String. I think the
> compiler can't determine which operator+ should be used here:
> a) const QString operator+(const char*, const QString&)
> b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)

Can't we fix this by adding this operator?

WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&);

[Darin Adler]

(In reply to comment #66)
> Hey Darin,
> 
> I think comment 58 explains it well:
> 
> > I have a quick look at the Qt build, and attached a quick workaround.
> > I know it isn't a good solution for this problem, but it can show
> > what happened. It seems, that the problem is around operator+ when
> > the left side is char* and the right side is String. I think the
> > compiler can't determine which operator+ should be used here:
> > a) const QString operator+(const char*, const QString&)
> > b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)
> 
> I guess you missed that comment.

I think the simplest solution to that is to explicitly overload operator+ for the case where the left side is const char* and the right side is const String&. While it’s inelegant to have to have that extra function, it would do no harm.

On a separate note, I also think we want that case optimized so that the char* doesn’t get converted to a string before we create the concatenated string. Is that optimized or not?

[Nikolas Zimmermann]

(In reply to comment #67)
> (In reply to comment #58)
> > Created an attachment (id=91831) [details] [details]
> > workaround for Patch v13 to make Qt build happy
> > 
> > I have a quick look at the Qt build, and attached a quick workaround.
> > I know it isn't a good solution for this problem, but it can show
> > what happened. It seems, that the problem is around operator+ when
> > the left side is char* and the right side is String. I think the
> > compiler can't determine which operator+ should be used here:
> > a) const QString operator+(const char*, const QString&)
> > b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)
> 
> Can't we fix this by adding this operator?
> 
> WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&);

Hm, I had this in earlier patches, not sure why I removed it, will test...

(In reply to comment #68)
> (In reply to comment #66)
> > Hey Darin,
> > 
> > I think comment 58 explains it well:
> > 
> > > I have a quick look at the Qt build, and attached a quick workaround.
> > > I know it isn't a good solution for this problem, but it can show
> > > what happened. It seems, that the problem is around operator+ when
> > > the left side is char* and the right side is String. I think the
> > > compiler can't determine which operator+ should be used here:
> > > a) const QString operator+(const char*, const QString&)
> > > b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)
> > 
> > I guess you missed that comment.
> 
> I think the simplest solution to that is to explicitly overload operator+ for the case where the left side is const char* and the right side is const String&. While it’s inelegant to have to have that extra function, it would do no harm.
I'll try Adams suggestion first.

> 
> On a separate note, I also think we want that case optimized so that the char* doesn’t get converted to a string before we create the concatenated string. Is that optimized or not?
That's the purpose of the templatified operator+:
template<typename T>
StringAppend<String, T> operator+(const String& string1, T string2)
To optimize for the case, where you concat a String with eg. a char*.

Going to try to fix the Qt build by supllying the <T, String> operator now...

[Nikolas Zimmermann]

(In reply to comment #67)
> Can't we fix this by adding this operator?
> 
> WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&);

I remember why I removed it initially, if we provide both operator+(T, const String&) and operator+(const String&, T) it clashes if T=String.

/Users/nzimmermann/Coding/WebKit/Source/WebCore/storage/AbstractDatabase.cpp:272: error: ambiguous overload for 'operator+' in 'WTF::operator+(T, const WTF::String&) [with T = const char*](((const WTF::String&)((const WTF::String*)WebCore::AbstractDatabase::databaseInfoTableName()))) + " (key TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,value TEXT NOT NULL ON CONFLICT FAIL);"'
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/StringOperators.h:95: note: candidates are: WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&) [with T = WTF::StringAppend<const char*, WTF::String>]
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/StringOperators.h:89: note:                 WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T) [with T = const char*]

Let's see if I can solve that ambiguity.

[Nikolas Zimmermann]

Created attachment 92201 [details]
Patch v14

Experimenting with Darins suggestion, to see whether Qt builds fine now.

[Early Warning System Bot]

Attachment 92201 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8552473

[Build Bot]

Attachment 92201 [details] did not build on win:
Build output: http://queues.webkit.org/results/8556475

[Nikolas Zimmermann]

Created attachment 92215 [details]
Patch v15

Additionally supply "StringAppend<const char*, AtomicString> operator+(const char* string1, const AtomicString& string2)", exposed by editing/markup.cpp (Qt build failure).
Even with that fix win won't build properly, there's still a cyclic include problem, to be investigated.

[WebKit Review Bot]

Attachment 92201 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8556492

[WebKit Review Bot]

Attachment 92201 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8552501

[Build Bot]

Attachment 92215 [details] did not build on win:
Build output: http://queues.webkit.org/results/8552502

[WebKit Review Bot]

Attachment 92201 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8560004

[WebKit Review Bot]

Attachment 92201 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8558494

[Adam Roben (:aroben)]

(In reply to comment #70)
> (In reply to comment #67)
> > Can't we fix this by adding this operator?
> > 
> > WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&);
> 
> I remember why I removed it initially, if we provide both operator+(T, const String&) and operator+(const String&, T) it clashes if T=String.
> 
> /Users/nzimmermann/Coding/WebKit/Source/WebCore/storage/AbstractDatabase.cpp:272: error: ambiguous overload for 'operator+' in 'WTF::operator+(T, const WTF::String&) [with T = const char*](((const WTF::String&)((const WTF::String*)WebCore::AbstractDatabase::databaseInfoTableName()))) + " (key TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,value TEXT NOT NULL ON CONFLICT FAIL);"'
> /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/StringOperators.h:95: note: candidates are: WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&) [with T = WTF::StringAppend<const char*, WTF::String>]
> /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/StringOperators.h:89: note:                 WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T) [with T = const char*]
> 
> Let's see if I can solve that ambiguity.

Maybe adding an operator that explicitly takes two Strings would do it.

[Nikolas Zimmermann]

(In reply to comment #80)
> > Let's see if I can solve that ambiguity.
> 
> Maybe adding an operator that explicitly takes two Strings would do it.
I already tried that :-) Adding an explicit "StringAppend<String, String> operator+(const String&, const String&)" just results in a third case that's ambiguous, according to gcc.

See Patch v15 for the solution that actually works with Qt - that problem is resolved now.
Patch v15 should build everywhere except win, unfortunately cr-linux bot is behind, gtk results will come soon.

The win build failure is an include issue, I'm trying to reorganize the StringOperators.h/StringConcatenate.h inclusion to avoid the problem, still waiting for my local mac build to finish.

I hope that solution will work on win as well, tired of 1.5h build-test-build cycles... :-) Stay tuned...

[Darin Adler]

Once we get this compiling I want to make sure that we get the optimal case for various combinations of types that are being concatenated. In theory, as long as either the first or second item is a String, the entire concatenation should work with the same performance as a makeString call.

[Nikolas Zimmermann]

(In reply to comment #82)
> Once we get this compiling I want to make sure that we get the optimal case for various combinations of types that are being concatenated. In theory, as long as either the first or second item is a String, the entire concatenation should work with the same performance as a makeString call.

Note, the first approach (https://bugs.webkit.org/attachment.cgi?id=91364&action=prettypatch) directly used makeString() calls for 2/3/4 arguments that are added, and fell back to StringBuilder for > 4 arguments.

The current approach generates the same code as using makeString() for N arguments, where N is arbitary, as the template is defined recursive. The special StringTypeAdapter<> for StringAppend<T1,T2> types take care of that. The required allocations, etc. should be equal to using makeString - if it's not it's a bug. I'll look again closely, once the build problems are fixed.

Please verify that my comments are correct. We should get consensus that this approach is fine.

[Darin Adler]

(In reply to comment #83)
> We should get consensus that this approach is fine.

The approach is great. I just want to be sure that for cases like this:

    const char* a;
    String b;
    const char* c;
    String d = a + b + c;

We get the single makeString call. I didn’t study the template well enough to be sure that would be the case.

[Nikolas Zimmermann]

Created attachment 92308 [details]
Patch v16

New patch, new luck.

[Build Bot]

Attachment 92308 [details] did not build on win:
Build output: http://queues.webkit.org/results/8553635

[Nikolas Zimmermann]

Created attachment 92399 [details]
Patch v17

New day, new patch. Another attempt to fix the win build, by moving StringAppend right into StringConcatenate.h

[Build Bot]

Attachment 92399 [details] did not build on win:
Build output: http://queues.webkit.org/results/8554869

[WebKit Review Bot]

Attachment 92399 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8558857

[WebKit Review Bot]

Attachment 92399 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8571417

[WebKit Review Bot]

Attachment 92399 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8557918

[Adam Roben (:aroben)]

I think this is the problem on Windows.

PathWalker.cpp (e.g.) has these #includes:

#include "config.h"
#include "PathWalker.h"

#include <wtf/text/StringConcatenate.h>

(It uses makeString() in its implementation.)

StringConcatenate.h then leads to these recursive #includes:

AtomicString.h
   WTFString.h

But note this code near the end of WTFString.h:

#ifndef AtomicString_h
#include "StringOperators.h"
#endif

So StringOperators.h never gets pulled in. Then StringConcatenate.h tries to use StringAppend (which is defined in StringOperators.h) and fails.

[Nikolas Zimmermann]

Created attachment 92950 [details]
Patch v18

New attempt - including StringConcatenate.h directly hopefully works now... (Adams hint)

[WebKit Review Bot]

Comment on attachment 92950 [details]
Patch v18

Attachment 92950 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8653969

[WebKit Review Bot]

Comment on attachment 92950 [details]
Patch v18

Attachment 92950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8655948

[WebKit Review Bot]

Comment on attachment 92950 [details]
Patch v18

Attachment 92950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8685026

[Nikolas Zimmermann]

Created attachment 92995 [details]
Patch v19

Adapt to chromium gtest change, try to fix cr-linux/cr-mac builds.

[Nikolas Zimmermann]

Created attachment 93105 [details]
Patch v20

Another attempt at fixing the win cyclic include problem. If this still won't work I'm lost....
Note: This is likely to break other ports again :( If so, Patch v19 is the reference patch that builds everywhere except on win.

[Adam Roben (:aroben)]

Comment on attachment 93105 [details]
Patch v20

I built this patch without trouble on Windows.

[Nikolas Zimmermann]

(In reply to comment #99)
> (From update of attachment 93105 [details])
> I built this patch without trouble on Windows.
Patch v20?

[Adam Roben (:aroben)]

(In reply to comment #100)
> (In reply to comment #99)
> > (From update of attachment 93105 [details] [details])
> > I built this patch without trouble on Windows.
> Patch v20?

Yes. (That's what attachment 93105 [details] is.)

[Darin Adler]

Comment on attachment 93105 [details]
Patch v20

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 ([e-mail])
> + * 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.

[Nikolas Zimmermann]

(In reply to comment #102)
> (From update of attachment 93105 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93105&action=review
> 
> Congratulations on getting this to compile!
Thanks :-)

> 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.
Great, I'd still upload my new patch with r? to give it a EWS run.

> 
> > 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.
Agreed. I think AtomicString.h / StringConcatenate.h includes should all be replaced by a single WTFString.h include, if there aren't any convincing arguments against this.
 
> > Source/JavaScriptCore/wtf/text/StringOperators.h:4
> > + * Copyright (C) 1999 Lars Knoll ([e-mail])
> > + * 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.
Right, fixed.

> 
> > 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.
Trying.

> 
> > 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.
Or using DEFINE_STATIC_LOCAL(String, emptyString, (StringImpl::empty()) ?

> 
> > 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.
The problem is the ternary operator usage, both arguments of the operator have to be of the same type, otherwhise the operator AtomicString() can't be invoked on the StringAppend<...> object. This workaround is needed in each place using the ternary operator with concatted Strings and either a String/AtomicString operand.

> 
> > 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.
Fixed by introducing a static local "xmlns:" xmlnsWithColon String.

> 
> > 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.
Ok, I'll introduce two other xDotString/yDotString statics.

> 
> > 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?
Right -- most of the things you found are relicts from the time, where I didn't have the specialied const char* operator+ variants, it should work now without these hacks.

> 
> > 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.
Previously, we implicitly converted the first argument of the ? operator to String(), because the String operator+ always returned a String object. Both arguments of operator? have to be of the same type, so the conversion worked fine before. Now with StringAppend, we explicitely have to convert the first side or the second side to a string, otherwhise the operator String() can't be invoked -- this just won't build.

> 
> > 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.
Fixed.
e
> 
> > 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?
Fixed.

> 
> > 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.
Fixed.

> 
> > 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.
Fixed.

> 
> > 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?
GTest defines a template<typename T1, Typename T2> .. operator+(T1, T2), we conflict with it, I need to cast to WebCore types first, unfortunately. But this is the only place which exposes this problem, so I think it's fine to 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.
:( Yeah...

Okay, I'll try a local build first, let's see how many of the changes work :-)
Note: I'm not landing the patch as-is, but I rather want to fix Darins comments first.
So I'll upload the patch with r?, to let EWS run, if everything still builds I'm landing it.

[Nikolas Zimmermann]

(In reply to comment #103)

I'm replying again with detailed compiler logs, for each of the changes Darin commented, that I've fixed now:

> > > 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.
> The problem is the ternary operator usage, both arguments of the operator have to be of the same type, otherwhise the operator AtomicString() can't be invoked on the StringAppend<...> object. This workaround is needed in each place using the ternary operator with concatted Strings and either a String/AtomicString operand.

Reverting the XMLDocumentParserLibxml2.cpp changes results in:
/Users/nzimmermann/Coding/WebKit/Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:742: error: call of overloaded 'AtomicString(WTF::StringAppend<WTF::String, WTF::String>)' is ambiguous
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/AtomicString.h:50: note: candidates are: WTF::AtomicString::AtomicString(const WTF::String&)
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/AtomicString.h:39: note:                 WTF::AtomicString::AtomicString(const WTF::AtomicString&)

StringAppend offers both operator String() and operator AtomicString(), so it's not clear which AtomicString constructor should be invoked.

Just removing the AtomicString(...) makes it work fine, as the first argument of the ternary operator is already an AtomicString, so for the second argument, operator AtomicString() of StringAppend<..> is automatically invoked:
-        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : AtomicString(attrPrefix + ":" + toString(attributes[i].localname));
+        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : attrPrefix + ":" + toString(attributes[i].localname);

This resolves the problem.

> 
> > 
> > > 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.
> Fixed by introducing a static local "xmlns:" xmlnsWithColon String.

-    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : "xmlns";
-    return !element->hasAttribute(attr);
+    if (prefix.isEmpty())
+        return !element->hasAttribute(xmlnsAtom);
+
+    DEFINE_STATIC_LOCAL(String, xmlnsWithColon, ("xmlns:"));
+    return !element->hasAttribute(xmlnsWithColon + prefix);

This is the new solution, which should be cleaner and faster.

> 
> > 
> > > 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.
> Ok, I'll introduce two other xDotString/yDotString statics.
Solved this way:
     const AtomicString& name = element()->name();
-    encoding.appendData(name.isEmpty() ? "x" : (name + ".x"), m_clickLocation.x());
-    encoding.appendData(name.isEmpty() ? "y" : (name + ".y"), m_clickLocation.y());
-    if (!name.isEmpty() && !element()->value().isEmpty())
+    if (name.isEmpty()) {
+        encoding.appendData("x", m_clickLocation.x());
+        encoding.appendData("y", m_clickLocation.y());
+        return true;
+    }
+
+    DEFINE_STATIC_LOCAL(String, dotXString, (".x"));
+    DEFINE_STATIC_LOCAL(String, dotYString, (".y"));
+    encoding.appendData(name + dotXString, m_clickLocation.x());
+    encoding.appendData(name + dotYString, m_clickLocation.y());
+
+    if (!!element()->value().isEmpty())
         encoding.appendData(name, element()->value());
     return true;

Arguable more verbose, but also more efficient.

> 
> > 
> > > 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?
> Right -- most of the things you found are relicts from the time, where I didn't have the specialied const char* operator+ variants, it should work now without these hacks.
When reverting all changes to HTMLTreeBuilder, I see:

/Users/nzimmermann/Coding/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:672: error: call of overloaded 'AtomicString(WTF::StringAppend<WTF::String, WTF::AtomicString>)' is ambiguous
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/AtomicString.h:50: note: candidates are: WTF::AtomicString::AtomicString(const WTF::String&)
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/AtomicString.h:39: note:                 WTF::AtomicString::AtomicString(const WTF::AtomicString&)

Fix is very simple:
-        AtomicString prefixColonLocalName(prefix + ":" + localName);
+        AtomicString prefixColonLocalName = prefix + ':' + localName;

> 
> > 
> > > 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.
> Previously, we implicitly converted the first argument of the ? operator to String(), because the String operator+ always returned a String object. Both arguments of operator? have to be of the same type, so the conversion worked fine before. Now with StringAppend, we explicitely have to convert the first side or the second side to a string, otherwhise the operator String() can't be invoked -- this just won't build.

I reverted the change to test:
/Users/nzimmermann/Coding/WebKit/Source/WebCore/loader/CrossOriginAccessControl.cpp:113: error: no match for ternary 'operator?:' in 'WTF::operator==(((const WTF::String&)((const WTF::String*)accessControlOriginString)), ((const char*)"*")) ? "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true." : WTF::operator+(const WTF::String&, T) [with T = const char*](((const char*)" is not allowed by Access-Control-Allow-Origin."))'

A possible fix is to unwrap the ternary operator:
-        errorDescription = (accessControlOriginString == "*") ? "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true."
-            : "Origin " + securityOrigin->toString() + " is not allowed by Access-Control-Allow-Origin.";
+        if (accessControlOriginString == "*")
+            errorDescription = "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true.";
+        else
+            errorDescription =  "Origin " + securityOrigin->toString() + " is not allowed by Access-Control-Allow-Origin.";

This will build just fine now, I changed my patch.

> 
> > 
> > > 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?
> GTest defines a template<typename T1, Typename T2> .. operator+(T1, T2), we conflict with it, I need to cast to WebCore types first, unfortunately. But this is the only place which exposes this problem, so I think it's fine to change.

From patch v18, cr-linux build output:

Source/WebKit/chromium/testing/gtest/include/gtest/gtest.h:1353:   instantiated from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = UChar [4], T2 = WTF::String, bool lhs_is_null_literal = false]'
Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:183:   instantiated from here
Source/WebKit/chromium/testing/gtest/include/gtest/gtest.h:1316: error: ambiguous overload for 'operator==' in 'expected == actual'
Source/JavaScriptCore/wtf/text/AtomicString.h:138: note: candidates are: bool WTF::operator==(const WTF::String&, const WTF::AtomicString&)
Source/JavaScriptCore/wtf/text/AtomicString.h:136: note:                 bool WTF::operator==(const WTF::AtomicString&, const WTF::String&)
Source/JavaScriptCore/wtf/text/AtomicString.h:133: note:                 bool WTF::operator==(const WTF::AtomicString&, const WTF::AtomicString&)
Source/JavaScriptCore/wtf/text/WTFString.h:361: note:                 bool WTF::operator==(const WTF::String&, const WTF::String&)

Nothing I can do to fix that, from our side...

[Nikolas Zimmermann]

Created attachment 93271 [details]
Patch v21

Testing Darins suggestions with EWS.

[Early Warning System Bot]

Comment on attachment 93271 [details]
Patch v21

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

[Nikolas Zimmermann]

Created attachment 93274 [details]
Patch v22

[Nikolas Zimmermann]

Comment on attachment 93274 [details]
Patch v22

Oops patch v22, won't link on mac as I accidentally removed "__ZNK3JSC10JSFunction23isHostFunctionNonInlineEv" from the JavaScriptCore.exp file -- the String append operator changes themselves build/link just fine.

[WebKit Review Bot]

Comment on attachment 93271 [details]
Patch v21

Attachment 93271 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8689677

[WebKit Review Bot]

Comment on attachment 93274 [details]
Patch v22

Attachment 93274 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8691555

New failing tests:
fast/forms/input-image-submit.html

[WebKit Review Bot]

Created attachment 93276 [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

[WebKit Review Bot]

Comment on attachment 93271 [details]
Patch v21

Attachment 93271 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8685720

[Nikolas Zimmermann]

Landed in r86330.

[WebKit Review Bot]

http://trac.webkit.org/changeset/86330 might have broken Qt Linux Release
The following tests are not passing:
fast/forms/input-image-submit.html

[Nikolas Zimmermann]

Fixed typo leading to regression in fast/forms/input-image-submit.html in r86332.

[Nikolas Zimmermann]

WinCE/WinCairo still have problems - no EWS no way to catch these errors before landing.
Anyhow, landed a possible wince build fix in r86333.
Trying to fix WinCE/WinCairo linking by exporting three symbols in the JSC.def file in r86334.

[Nikolas Zimmermann]

(In reply to comment #116)
> Trying to fix WinCE/WinCairo linking by exporting three symbols in the JSC.def file in r86334.
Reverted again in r86335 - it broke the win build. WinCE build is fixed even w/o this problem.
To summarize: WinCairo is the only platform which remains broken, it doesn't link.

5>WebCore_debug.lib(InspectorDOMAgent.obj) : error LNK2001: unresolved external symbol "class WTF::String __cdecl WTF::operator+(class WTF::String const &,class WTF::String const &)" (??HWTF@@YA?AVString@0@ABV10@0@Z)
5>WebCore_debug.lib(ResourceHandleManager.obj) : error LNK2001: unresolved external symbol "class WTF::String __cdecl WTF::operator+(class WTF::String const &,class WTF::String const &)" (??HWTF@@YA?AVString@0@ABV10@0@Z)

This operator+ variant is declared inline in StringOperators.h, not sure why this happens. I guess I'll have to wait for Brent Fulgham...