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

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough 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 91364: Patch v9
https://bugs.webkit.org/attachment.cgi?id=91364&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
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.