Cut down on use of StringBuffer, possibly leading toward removing it entirely
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Mar 2019 00:27:10 +0000 (00:27 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Mar 2019 00:27:10 +0000 (00:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195870

Reviewed by Daniel Bates.

Source/WebCore:

* dom/Document.cpp:
(WebCore::canonicalizedTitle): Fixed all the problems mentioned in "FIXME".
Made this a single function rather than a function template. Switch to
StringBuilder instead of StringBuffer. Return the original string if the
canonicalize operation doesn't change anything.
(WebCore::Document::updateTitle): Updated for the change above.

* platform/Length.cpp:
(WebCore::newCoordsArray): Use createUninitialized instead of StringBuffer.
Also got rid of unneeded use of upconvertedCharacters on a temporary string
that we explicitly created with 16-bit characters. The performance of this
function could be considerably simplified by not copying the original string
at all, but didn't do that at this time.

* platform/text/TextCodecUTF16.cpp:
(WebCore::TextCodecUTF16::decode): Use createUninitialized instead of
StringBuffer. Also renamed numChars to numCodeUnits to both switch to complete
words and to be slightly more accurate.

* rendering/RenderText.cpp:
(WebCore::convertNoBreakSpace): Added.
(WebCore::capitalize): Use Vector instead of StringBuffer. Simplify code by
using convertNoBreakSpace function. Removed code that was using StringImpl
directly for a tiny speed boost; if we want to optimize the performance of
this function we would need to do more than that. Return the original string
if it happens to already be capitalized.

Source/WTF:

* wtf/URL.cpp: Remove a now-inaccurate comment mentioning StringBuffer.

* wtf/text/StringView.cpp:
(WTF::convertASCIICase): Use createUninitialized instead of StringBuffer.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243118 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/URL.cpp
Source/WTF/wtf/text/StringView.cpp
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/platform/Length.cpp
Source/WebCore/platform/text/TextCodecUTF16.cpp
Source/WebCore/rendering/RenderText.cpp

index af2c29f..09b5320 100644 (file)
@@ -1,3 +1,15 @@
+2019-03-18  Darin Adler  <darin@apple.com>
+
+        Cut down on use of StringBuffer, possibly leading toward removing it entirely
+        https://bugs.webkit.org/show_bug.cgi?id=195870
+
+        Reviewed by Daniel Bates.
+
+        * wtf/URL.cpp: Remove a now-inaccurate comment mentioning StringBuffer.
+
+        * wtf/text/StringView.cpp:
+        (WTF::convertASCIICase): Use createUninitialized instead of StringBuffer.
+
 2019-03-18  Xan Lopez  <xan@igalia.com>
 
         [WTF] Remove redundant std::move in StringConcatenate
index fa001a8..67d61c0 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004, 2007-2008, 2011-2013, 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2019 Apple Inc. All rights reserved.
  * Copyright (C) 2012 Research In Motion Limited. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
 #include <wtf/text/StringHash.h>
 #include <wtf/text/TextStream.h>
 
-// FIXME: This file makes too much use of the + operator on String.
-// We either have to optimize that operator so it doesn't involve
-// so many allocations, or change this to use StringBuffer instead.
-
-
 namespace WTF {
 
 typedef Vector<char, 512> CharBuffer;
index 42385e0..ee16b9c 100644 (file)
@@ -34,7 +34,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <wtf/Lock.h>
 #include <wtf/NeverDestroyed.h>
 #include <wtf/Optional.h>
-#include <wtf/text/StringBuffer.h>
 #include <wtf/text/TextBreakIterator.h>
 #include <wtf/unicode/UTF8Conversion.h>
 
@@ -220,11 +219,11 @@ String convertASCIICase(const CharacterType* input, unsigned length)
     if (!input)
         return { };
 
-    StringBuffer<CharacterType> buffer(length);
-    CharacterType* characters = buffer.characters();
+    CharacterType* characters;
+    auto result = String::createUninitialized(length, characters);
     for (unsigned i = 0; i < length; ++i)
         characters[i] = type == ASCIICase::Lower ? toASCIILower(input[i]) : toASCIIUpper(input[i]);
-    return String::adopt(WTFMove(buffer));
+    return result;
 }
 
 String StringView::convertToASCIILowercase() const
index 3453d6c..79f6b46 100644 (file)
@@ -1,3 +1,37 @@
+2019-03-18  Darin Adler  <darin@apple.com>
+
+        Cut down on use of StringBuffer, possibly leading toward removing it entirely
+        https://bugs.webkit.org/show_bug.cgi?id=195870
+
+        Reviewed by Daniel Bates.
+
+        * dom/Document.cpp:
+        (WebCore::canonicalizedTitle): Fixed all the problems mentioned in "FIXME".
+        Made this a single function rather than a function template. Switch to
+        StringBuilder instead of StringBuffer. Return the original string if the
+        canonicalize operation doesn't change anything.
+        (WebCore::Document::updateTitle): Updated for the change above.
+
+        * platform/Length.cpp:
+        (WebCore::newCoordsArray): Use createUninitialized instead of StringBuffer.
+        Also got rid of unneeded use of upconvertedCharacters on a temporary string
+        that we explicitly created with 16-bit characters. The performance of this
+        function could be considerably simplified by not copying the original string
+        at all, but didn't do that at this time.
+
+        * platform/text/TextCodecUTF16.cpp:
+        (WebCore::TextCodecUTF16::decode): Use createUninitialized instead of
+        StringBuffer. Also renamed numChars to numCodeUnits to both switch to complete
+        words and to be slightly more accurate.
+
+        * rendering/RenderText.cpp:
+        (WebCore::convertNoBreakSpace): Added.
+        (WebCore::capitalize): Use Vector instead of StringBuffer. Simplify code by
+        using convertNoBreakSpace function. Removed code that was using StringImpl
+        directly for a tiny speed boost; if we want to optimize the performance of
+        this function we would need to do more than that. Return the original string
+        if it happens to already be capitalized.
+
 2019-03-18  Timothy Hatcher  <timothy@apple.com>
 
         WKWebView.GetContentsShouldReturnAttributedString is crashing on iOS Simulator.
index 2c317a0..d9abd47 100644 (file)
@@ -3,7 +3,7 @@
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
- * Copyright (C) 2004-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2019 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2008, 2009, 2011, 2012 Google Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
@@ -1503,44 +1503,32 @@ Element* Document::scrollingElement()
     return body();
 }
 
-template<typename CharacterType> static inline String canonicalizedTitle(Document& document, const String& title)
+static String canonicalizedTitle(Document& document, const String& title)
 {
-    // FIXME: Compiling a separate copy of this for LChar and UChar is likely unnecessary.
-    // FIXME: Missing an optimized case for when title is fine as-is. This unnecessarily allocates
-    // and keeps around a new copy, and it's even the less optimal type of StringImpl with a separate buffer.
-    // Could probably just use StringBuilder instead.
-
-    auto* characters = title.characters<CharacterType>();
-    unsigned length = title.length();
+    // Collapse runs of HTML spaces into single space characters.
+    // Strip leading and trailing spaces.
+    // Replace backslashes with currency symbols.
 
-    StringBuffer<CharacterType> buffer { length };
-    unsigned bufferLength = 0;
+    StringBuilder builder;
 
     auto* decoder = document.decoder();
     auto backslashAsCurrencySymbol = decoder ? decoder->encoding().backslashAsCurrencySymbol() : '\\';
 
-    // Collapse runs of HTML spaces into single space characters.
-    // Strip leading and trailing spaces.
-    // Replace backslashes with currency symbols.
     bool previousCharacterWasHTMLSpace = false;
-    for (unsigned i = 0; i < length; ++i) {
-        auto character = characters[i];
+    for (auto character : StringView { title }.codeUnits()) {
         if (isHTMLSpace(character))
             previousCharacterWasHTMLSpace = true;
         else {
             if (character == '\\')
                 character = backslashAsCurrencySymbol;
-            if (previousCharacterWasHTMLSpace && bufferLength)
-                buffer[bufferLength++] = ' ';
-            buffer[bufferLength++] = character;
+            if (previousCharacterWasHTMLSpace && !builder.isEmpty())
+                builder.append(' ');
+            builder.append(character);
             previousCharacterWasHTMLSpace = false;
         }
     }
-    if (!bufferLength)
-        return { };
 
-    buffer.shrink(bufferLength);
-    return String::adopt(WTFMove(buffer));
+    return builder == title ? title : builder.toString();
 }
 
 void Document::updateTitle(const StringWithDirection& title)
@@ -1549,14 +1537,9 @@ void Document::updateTitle(const StringWithDirection& title)
         return;
 
     m_rawTitle = title;
-    m_title = title;
 
-    if (!m_title.string.isEmpty()) {
-        if (m_title.string.is8Bit())
-            m_title.string = canonicalizedTitle<LChar>(*this, m_title.string);
-        else
-            m_title.string = canonicalizedTitle<UChar>(*this, m_title.string);
-    }
+    m_title.string = canonicalizedTitle(*this, title.string);
+    m_title.direction = title.direction;
 
     if (auto* loader = this->loader())
         loader->setTitle(m_title);
index 1d048f5..ce9c945 100644 (file)
@@ -2,7 +2,7 @@
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller ( mueller@kde.org )
- * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2019 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Andrew Wellington (proton@wiretapped.net)
  *
  * This library is free software; you can redistribute it and/or
 #include <wtf/MallocPtr.h>
 #include <wtf/NeverDestroyed.h>
 #include <wtf/StdLibExtras.h>
-#include <wtf/text/StringBuffer.h>
 #include <wtf/text/StringView.h>
 #include <wtf/text/TextStream.h>
 
-
 namespace WebCore {
 
 static Length parseLength(const UChar* data, unsigned length)
@@ -91,7 +89,8 @@ static unsigned countCharacter(StringImpl& string, UChar character)
 UniqueArray<Length> newCoordsArray(const String& string, int& len)
 {
     unsigned length = string.length();
-    StringBuffer<UChar> spacified(length);
+    UChar* spacified;
+    auto str = StringImpl::createUninitialized(length, spacified);
     for (unsigned i = 0; i < length; i++) {
         UChar cc = string[i];
         if (cc > '9' || (cc < '0' && cc != '-' && cc != '*' && cc != '.'))
@@ -99,23 +98,21 @@ UniqueArray<Length> newCoordsArray(const String& string, int& len)
         else
             spacified[i] = cc;
     }
-    RefPtr<StringImpl> str = StringImpl::adopt(WTFMove(spacified));
 
     str = str->simplifyWhiteSpace();
 
-    len = countCharacter(*str, ' ') + 1;
+    len = countCharacter(str, ' ') + 1;
     auto r = makeUniqueArray<Length>(len);
 
     int i = 0;
     unsigned pos = 0;
     size_t pos2;
 
-    auto upconvertedCharacters = StringView(str.get()).upconvertedCharacters();
     while ((pos2 = str->find(' ', pos)) != notFound) {
-        r[i++] = parseLength(upconvertedCharacters + pos, pos2 - pos);
+        r[i++] = parseLength(str->characters16() + pos, pos2 - pos);
         pos = pos2+1;
     }
-    r[i] = parseLength(upconvertedCharacters + pos, str->length() - pos);
+    r[i] = parseLength(str->characters16() + pos, str->length() - pos);
 
     ASSERT(i == len - 1);
 
index 386a178..be00656 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -27,7 +27,6 @@
 #include "TextCodecUTF16.h"
 
 #include <wtf/text/CString.h>
-#include <wtf/text/StringBuffer.h>
 #include <wtf/text/WTFString.h>
 
 namespace WebCore {
@@ -71,10 +70,11 @@ String TextCodecUTF16::decode(const char* bytes, size_t length, bool, bool, bool
 
     const unsigned char* p = reinterpret_cast<const unsigned char*>(bytes);
     size_t numBytes = length + m_haveBufferedByte;
-    size_t numChars = numBytes / 2;
+    size_t numCodeUnits = numBytes / 2;
+    RELEASE_ASSERT(numCodeUnits <= std::numeric_limits<unsigned>::max());
 
-    StringBuffer<UChar> buffer(numChars);
-    UChar* q = buffer.characters();
+    UChar* q;
+    auto result = String::createUninitialized(numCodeUnits, q);
 
     if (m_haveBufferedByte) {
         UChar c;
@@ -85,17 +85,17 @@ String TextCodecUTF16::decode(const char* bytes, size_t length, bool, bool, bool
         *q++ = c;
         m_haveBufferedByte = false;
         p += 1;
-        numChars -= 1;
+        numCodeUnits -= 1;
     }
 
     if (m_littleEndian) {
-        for (size_t i = 0; i < numChars; ++i) {
+        for (size_t i = 0; i < numCodeUnits; ++i) {
             UChar c = p[0] | (p[1] << 8);
             p += 2;
             *q++ = c;
         }
     } else {
-        for (size_t i = 0; i < numChars; ++i) {
+        for (size_t i = 0; i < numCodeUnits; ++i) {
             UChar c = (p[0] << 8) | p[1];
             p += 2;
             *q++ = c;
@@ -108,9 +108,7 @@ String TextCodecUTF16::decode(const char* bytes, size_t length, bool, bool, bool
         m_bufferedByte = p[0];
     }
 
-    buffer.shrink(q - buffer.characters());
-
-    return String::adopt(WTFMove(buffer));
+    return result;
 }
 
 Vector<uint8_t> TextCodecUTF16::encode(StringView string, UnencodableHandling)
index b2e07f2..44373b6 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * (C) 1999 Lars Knoll (knoll@kde.org)
  * (C) 2000 Dirk Mueller (mueller@kde.org)
- * Copyright (C) 2004-2007, 2013-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2019 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Andrew Wellington (proton@wiretapped.net)
  * Copyright (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *
@@ -51,7 +51,6 @@
 #include "VisiblePosition.h"
 #include <wtf/IsoMallocInlines.h>
 #include <wtf/NeverDestroyed.h>
-#include <wtf/text/StringBuffer.h>
 #include <wtf/text/StringBuilder.h>
 #include <wtf/text/TextBreakIterator.h>
 #include <wtf/unicode/CharacterNames.h>
@@ -141,30 +140,25 @@ static HashMap<const RenderText*, WeakPtr<RenderInline>>& inlineWrapperForDispla
     return map;
 }
 
-String capitalize(const String& string, UChar previousCharacter)
+static inline UChar convertNoBreakSpace(UChar character)
 {
-    // FIXME: Need to change this to use u_strToTitle instead of u_totitle and to consider locale.
+    return character == noBreakSpace ? ' ' : character;
+}
 
-    if (string.isNull())
-        return string;
+String capitalize(const String& string, UChar previousCharacter)
+{
+    // FIXME: Change this to use u_strToTitle instead of u_totitle and to consider locale.
 
     unsigned length = string.length();
-    auto& stringImpl = *string.impl();
-
-    if (length >= std::numeric_limits<unsigned>::max())
-        CRASH();
 
-    StringBuffer<UChar> stringWithPrevious(length + 1);
-    stringWithPrevious[0] = previousCharacter == noBreakSpace ? ' ' : previousCharacter;
-    for (unsigned i = 1; i < length + 1; i++) {
-        // Replace NO BREAK SPACE with a real space since ICU does not treat it as a word separator.
-        if (stringImpl[i - 1] == noBreakSpace)
-            stringWithPrevious[i] = ' ';
-        else
-            stringWithPrevious[i] = stringImpl[i - 1];
-    }
+    // Prepend the previous character, and convert NO BREAK SPACE to SPACE so ICU will see a word separator.
+    Vector<UChar> wordBreakCharacters;
+    wordBreakCharacters.grow(length + 1);
+    wordBreakCharacters[0] = convertNoBreakSpace(previousCharacter);
+    for (unsigned i = 1; i < length + 1; i++)
+        wordBreakCharacters[i] = convertNoBreakSpace(string[i - 1]);
 
-    auto* boundary = wordBreakIterator(StringView(stringWithPrevious.characters(), length + 1));
+    auto* boundary = wordBreakIterator(StringView { wordBreakCharacters.data(), length + 1 });
     if (!boundary)
         return string;
 
@@ -175,12 +169,12 @@ String capitalize(const String& string, UChar previousCharacter)
     int32_t startOfWord = ubrk_first(boundary);
     for (endOfWord = ubrk_next(boundary); endOfWord != UBRK_DONE; startOfWord = endOfWord, endOfWord = ubrk_next(boundary)) {
         if (startOfWord) // Ignore first char of previous string
-            result.append(stringImpl[startOfWord - 1] == noBreakSpace ? noBreakSpace : u_totitle(stringWithPrevious[startOfWord]));
+            result.append(string[startOfWord - 1] == noBreakSpace ? noBreakSpace : u_totitle(wordBreakCharacters[startOfWord]));
         for (int i = startOfWord + 1; i < endOfWord; i++)
-            result.append(stringImpl[i - 1]);
+            result.append(string[i - 1]);
     }
 
-    return result.toString();
+    return result == string ? string : result.toString();
 }
 
 inline RenderText::RenderText(Node& node, const String& text)