Attachment 111022 Details for Bug 66851 – Fix CSSPrimitiveValue::cssText() to use StringBuilder

[patch] Fix CSSPrimitiveValue::cssText() to use StringBuilder

0001-UsingStringBuilder_AddStringBuilderAppend.patch (text/plain), 22.54 KB, created by Oliver Varga on 2011-10-14 09:28:01 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Oliver Varga

Created: 2011-10-14 09:28:01 PDT

Size: 22.54 KB

patch

obsolete

>From 72aa4a10a93668ca1226195161edbfa5bbf21782 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Krist=C3=B3f=20Koszty=C3=B3?= <kkristof@inf.u-szeged.hu>
>Date: Fri, 14 Oct 2011 18:23:03 +0200
>Subject: [PATCH] UsingStringBuilder_AddStringBuilderAppend
>
>---
> Source/JavaScriptCore/ChangeLog                |   16 ++
> Source/JavaScriptCore/wtf/text/StringBuilder.h |  262 +++++++++++++++++++++++-
> Source/WebCore/ChangeLog                       |   15 ++
> Source/WebCore/css/CSSPrimitiveValue.cpp       |  149 ++++++++------
> 4 files changed, 373 insertions(+), 69 deletions(-)
>
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 7e99f7c..59ab028 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,19 @@
>+2011-10-14  Oliver Varga  <voliver@inf.u-szeged.hu>
>+
>+        NFix CSSPrimitiveValue::cssText() to use StringBuilder
>+        https://bugs.webkit.org/show_bug.cgi?id=66851
>+
>+        Add two overloads the StringBuilder::append() (append(double), append(int)).
>+        This is an equivalent of the previous CSSPrimitiveValue::formatNumber
>+        but it works directly with the StringBuilder, and it is much faster.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * wtf/text/StringBuilder.h:
>+        (WTF::StringBuilder::getPrecision):
>+        (WTF::StringBuilder::shifting):
>+        (WTF::StringBuilder::append):
>+
> 2011-10-13  Filip Pizlo  <fpizlo@apple.com>
> 
>         Speculation failures in ValueToInt32 are causing a 2x slow-down
>diff --git a/Source/JavaScriptCore/wtf/text/StringBuilder.h b/Source/JavaScriptCore/wtf/text/StringBuilder.h
>index 5edabec..fa16540 100644
>--- a/Source/JavaScriptCore/wtf/text/StringBuilder.h
>+++ b/Source/JavaScriptCore/wtf/text/StringBuilder.h
>@@ -20,12 +20,14 @@
>  * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
>  * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
>+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
> 
> #ifndef StringBuilder_h
> #define StringBuilder_h
> 
>+#include <math.h>
>+#include <wtf/dtoa.h>
> #include <wtf/Vector.h>
> #include <wtf/text/WTFString.h>
> 
>@@ -76,6 +78,264 @@ public:
>             append(&c, 1);
>     }
> 
>+    static inline unsigned getPrecision(int m_exponent, unsigned m_precision, unsigned digitsBeforeDecimalPoint)
>+    {
>+        unsigned length = 0;
>+
>+        if (m_exponent < 0) {
>+            length += -m_exponent - 1;
>+            length += m_precision;
>+            return length;
>+        }
>+
>+        if (m_precision <= digitsBeforeDecimalPoint)
>+            return 0;
>+
>+        length += m_precision - digitsBeforeDecimalPoint;
>+
>+        return length;
>+    }
>+
>+    static inline int shifting(char* num, int shift)
>+    {
>+            // #shift
>+            int p = 1;
>+            while (num[p+1] != '\0' && p <= shift && num[1] != '\0') {
>+                    num[ p ] = num[p+1];
>+                    num[p+1] = '.';
>+                    p++;
>+            }
>+
>+            // #fill0
>+            while (p <= shift) {
>+                    num[ p ] = '0';
>+                    num[p+1] = '.';
>+                    num[p+2] = 0;
>+                    p++;
>+            }
>+
>+            // check&set
>+            const int length = strlen(num);
>+            for (int i = length - 1; i >= p && (num[i] == '0' || num[i] == '.'); --i)
>+                    num[i]=0;
>+
>+            // skip
>+            if (num[0] == '0' && num[1] == '0')
>+                    return 2;
>+
>+            if (num[0] == '0' && num[1] != '.')
>+                    return 1;
>+
>+            if (num[0] != '0')
>+                return 0;
>+
>+            return 2;
>+    }
>+
>+    void append(double d, int precision = 6)
>+     {
>+         bool m_sign;
>+         int m_exponent;
>+         DtoaBuffer m_significand;
>+         unsigned m_precision;
>+
>+         bool isNegative = false;
>+         if (d < 0) {
>+             isNegative = true;
>+             d = -d;
>+             append("-", 1);
>+         }
>+
>+         ASSERT(isfinite(d));
>+         dtoa(m_significand, d, m_sign, m_exponent, m_precision);
>+
>+         ASSERT(m_precision);
>+         // Zero should always have exponent 0.
>+         ASSERT(m_significand[0] != '0' || !m_exponent);
>+         // No values other than zero should have a leading zero.
>+         ASSERT(m_significand[0] != '0' || m_precision == 1);
>+         // No values other than zero should have trailing zeros.
>+         ASSERT(m_significand[0] == '0' || m_significand[m_precision - 1] != '0');
>+
>+         char zero = '0';
>+         char dot = '.';
>+
>+         int absoluteExp = (m_exponent < 0) ? m_exponent * -1 : m_exponent;
>+         int sizeOfsignificand = strlen(m_significand);
>+         Vector<char, 32> number(absoluteExp + sizeOfsignificand + 3);
>+         char* pointer = &number[0];
>+         int sizeOfMSignificand = sizeof(m_significand);
>+         unsigned digitsBeforeDecimalPoint = m_exponent + 1;
>+
>+         precision = getPrecision(m_exponent, m_precision, digitsBeforeDecimalPoint);
>+
>+         bool dotAlreadyWritten = false;
>+         int maxLimit = 0;
>+         int placeOfDot = m_exponent;
>+         int numberOfZeros = (m_exponent > 0) ? 0 : -m_exponent;
>+
>+         bool jump = false;
>+         bool toShif = false;
>+         unsigned from = 0;
>+         unsigned valueOfShift = 0;
>+
>+         if (!precision) {
>+             if (m_exponent < 0) {
>+                m_significand[2] = m_significand[1];
>+                m_significand[1] = m_significand[0];
>+                m_significand[0] = '0';
>+                digitsBeforeDecimalPoint = 1;
>+             }
>+
>+             int i = 0;
>+             for (; i < digitsBeforeDecimalPoint && i < sizeOfsignificand; ++i) {
>+                *pointer = m_significand[i];
>+                pointer++;
>+             }
>+
>+             if (m_significand[i] > '4') {
>+                toShif = true;
>+                jump = true;
>+                dotAlreadyWritten = true;
>+                placeOfDot = 1;
>+                precision += digitsBeforeDecimalPoint;
>+                pointer = &number[0];
>+                *pointer = '0';
>+                pointer[1] = dot;
>+                pointer += 2;
>+                valueOfShift = digitsBeforeDecimalPoint;
>+                if (m_significand[0] == '0') {
>+                    maxLimit--;
>+                    valueOfShift++;
>+                }
>+             }
>+         } else
>+             jump = true;
>+
>+         if (jump) {
>+             for (int i = 0; i < numberOfZeros; i++) {
>+                 if (i == 1 && !dotAlreadyWritten) {
>+                     *pointer = dot;
>+                     pointer++;
>+                     dotAlreadyWritten = true;
>+                 }
>+                 *pointer = zero;
>+                 pointer++;
>+                 maxLimit = (dotAlreadyWritten) ? maxLimit + 1 : maxLimit;
>+                 if (maxLimit == precision) {
>+                     if (numberOfZeros > precision + 1 || (numberOfZeros == (precision + 1) && m_significand[0] < '5')) {
>+                         pointer = &number[1];
>+                         pointer[-1] = '0';
>+                         *pointer = '\0';
>+
>+                         if (toShif)
>+                             from = shifting(&number[0], valueOfShift);
>+
>+                         append(&number[from], strlen(&number[from]));
>+                         return;
>+                     }
>+                     if (numberOfZeros == precision + 1 && m_significand[0] > '4') {
>+                         pointer[-1] = '1';
>+                         *pointer = '\0';
>+
>+                         if (toShif)
>+                             from = shifting(&number[0], valueOfShift);
>+
>+                         append(&number[from], strlen(&number[from]));
>+                         return;
>+                     }
>+                 }
>+             }
>+             for (int i = 0; i < sizeOfsignificand; i++) {
>+                 if (i == placeOfDot + 1 && sizeOfsignificand != i && !dotAlreadyWritten) {
>+                     *pointer = dot;
>+                     pointer++;
>+                     dotAlreadyWritten = true;
>+                 }
>+                 *pointer = m_significand[i];
>+                 maxLimit = (dotAlreadyWritten) ? maxLimit + 1 : maxLimit;
>+                 if (maxLimit == precision) {
>+                     if (i + 1 < sizeOfMSignificand && m_significand[i + 1] < '5') {
>+                         while (*pointer == '0' && dotAlreadyWritten)
>+                              pointer--;
>+                          if (*pointer == dot)
>+                              *pointer = '\0';
>+                          pointer[1] = '\0';
>+
>+                          if (toShif)
>+                              from = shifting(&number[0], valueOfShift);
>+
>+                          append(&number[from], strlen(&number[from]));
>+                          return;
>+                      }
>+                      if (m_significand[i] == '9') {
>+                          while (*pointer == '9' && *pointer != dot)
>+                             pointer--;
>+
>+                          if (*pointer == dot)
>+                                pointer--;
>+                          *pointer = *pointer + 1;
>+                           pointer[1] = '\0';
>+
>+                          if (toShif)
>+                              from = shifting(&number[0], valueOfShift);
>+
>+                          append(&number[from], strlen(&number[from]));
>+                          return;
>+                      }
>+
>+                      *pointer = *pointer + 1;
>+                      pointer[1] = '\0';
>+
>+                      if (toShif)
>+                          from = shifting(&number[0], valueOfShift);
>+
>+                      append(&number[from], strlen(&number[from]));
>+                      return;
>+                  }
>+                 pointer++;
>+             }
>+        }
>+        for (int i = 0; i < (m_exponent - sizeOfsignificand) + 1; i++) {
>+            *pointer = zero;
>+            pointer++;
>+        }
>+
>+        *pointer = '\0';
>+
>+        if (toShif)
>+            from = shifting(&number[0], valueOfShift);
>+
>+        append(&number[from], strlen(&number[from]));
>+     }
>+
>+    void append(int n)
>+    {
>+        if (n < 0) {
>+            append("-", 1);
>+            n = -n;
>+        }
>+
>+        if (!(n / 10)) {
>+            char c = static_cast<char>(n + '0');
>+            append(&c, 1);
>+            return;
>+        }
>+
>+        int digitsOfNumber = static_cast<int>(log10(n) + 1);
>+        Vector<char, 32> number(digitsOfNumber + 1);
>+        char* pointer = &number[digitsOfNumber];
>+
>+        *pointer = '\0';
>+        pointer--;
>+        do {
>+            *pointer = (n % 10) + '0';
>+            pointer--;
>+            n /= 10;
>+        } while (n);
>+        append(&number[0], digitsOfNumber);
>+    }
>+
>     String toString()
>     {
>         if (m_string.isNull()) {
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f961e9d..9331921 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,18 @@
>+2011-10-14  Oliver Varga  <voliver@inf.u-szeged.hu>
>+
>+        Fix CSSPrimitiveValue::cssText() to use StringBuilder.
>+        https://bugs.webkit.org/show_bug.cgi?id=66851
>+
>+        Replace the string concatenation (+, +=, and Vector<Uchar> append())
>+        to the much faster StringBuilder append().
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        No new tests this is only a minimal performance tweak.
>+
>+        * css/CSSPrimitiveValue.cpp:
>+        (WebCore::CSSPrimitiveValue::cssText):
>+
> 2011-10-13  Arko Saha  <arko@motorola.com>
> 
>         Microdata: Basic implementation of document.getItems() method.
>diff --git a/Source/WebCore/css/CSSPrimitiveValue.cpp b/Source/WebCore/css/CSSPrimitiveValue.cpp
>index e14541d..c1c3e42 100644
>--- a/Source/WebCore/css/CSSPrimitiveValue.cpp
>+++ b/Source/WebCore/css/CSSPrimitiveValue.cpp
>@@ -680,17 +680,6 @@ int CSSPrimitiveValue::getIdent() const
>     return m_value.ident;
> }
> 
>-static String formatNumber(double number)
>-{
>-    DecimalNumber decimal(number);
>-
>-    StringBuffer buffer(decimal.bufferLengthForStringDecimal());
>-    unsigned length = decimal.toStringDecimal(buffer.characters(), buffer.length());
>-    ASSERT_UNUSED(length, length == buffer.length());
>-
>-    return String::adopt(buffer);
>-}
>-
> String CSSPrimitiveValue::cssText() const
> {
>     // FIXME: return the original value instead of a generated one (e.g. color
>@@ -701,83 +690,105 @@ String CSSPrimitiveValue::cssText() const
>         return cssTextCache().get(this);
>     }
> 
>-    String text;
>+    StringBuilder text;
>     switch (m_type) {
>         case CSS_UNKNOWN:
>             // FIXME
>             break;
>         case CSS_NUMBER:
>         case CSS_PARSER_INTEGER:
>-            text = formatNumber(m_value.num);
>+            text.append(m_value.num);
>             break;
>         case CSS_PERCENTAGE:
>-            text = formatNumber(m_value.num) + "%";
>+            text.append(m_value.num);
>+            text.append('%');
>             break;
>         case CSS_EMS:
>-            text = formatNumber(m_value.num) + "em";
>+            text.append(m_value.num);
>+            text.append("em");
>             break;
>         case CSS_EXS:
>-            text = formatNumber(m_value.num) + "ex";
>+            text.append(m_value.num);
>+            text.append("ex");
>             break;
>         case CSS_REMS:
>-            text = formatNumber(m_value.num) + "rem";
>+            text.append(m_value.num);
>+            text.append("rem");
>             break;
>         case CSS_PX:
>-            text = formatNumber(m_value.num) + "px";
>+            text.append(m_value.num);
>+            text.append("px");
>             break;
>         case CSS_CM:
>-            text = formatNumber(m_value.num) + "cm";
>+            text.append(m_value.num);
>+            text.append("cm");
>             break;
>         case CSS_MM:
>-            text = formatNumber(m_value.num) + "mm";
>+            text.append(m_value.num);
>+            text.append("mm");
>             break;
>         case CSS_IN:
>-            text = formatNumber(m_value.num) + "in";
>+            text.append(m_value.num);
>+            text.append("in");
>             break;
>         case CSS_PT:
>-            text = formatNumber(m_value.num) + "pt";
>+            text.append(m_value.num);
>+            text.append("pt");
>             break;
>         case CSS_PC:
>-            text = formatNumber(m_value.num) + "pc";
>+            text.append(m_value.num);
>+            text.append("pc");
>             break;
>         case CSS_DEG:
>-            text = formatNumber(m_value.num) + "deg";
>+            text.append(m_value.num);
>+            text.append("deg");
>             break;
>         case CSS_RAD:
>-            text = formatNumber(m_value.num) + "rad";
>+            text.append(m_value.num);
>+            text.append("rad");
>             break;
>         case CSS_GRAD:
>-            text = formatNumber(m_value.num) + "grad";
>+            text.append(m_value.num);
>+            text.append("grad");
>             break;
>         case CSS_MS:
>-            text = formatNumber(m_value.num) + "ms";
>+            text.append(m_value.num);
>+            text.append("ms");
>             break;
>         case CSS_S:
>-            text = formatNumber(m_value.num) + "s";
>+            text.append(m_value.num);
>+            text.append('s');
>             break;
>         case CSS_HZ:
>-            text = formatNumber(m_value.num) + "hz";
>+            text.append(m_value.num);
>+            text.append("hz");
>             break;
>         case CSS_KHZ:
>-            text = formatNumber(m_value.num) + "khz";
>+            text.append(m_value.num);
>+            text.append("khz");
>             break;
>         case CSS_TURN:
>-            text = formatNumber(m_value.num) + "turn";
>+            text.append(m_value.num);
>+            text.append("turn");
>             break;
>         case CSS_DIMENSION:
>             // FIXME
>             break;
>         case CSS_STRING:
>-            text = quoteCSSStringIfNeeded(m_value.string);
>+            text.append(quoteCSSStringIfNeeded(m_value.string));
>             break;
>         case CSS_FROM_FLOW:
>-            text = "-webkit-from-flow(" + quoteCSSStringIfNeeded(m_value.string) + ")";
>+            text.append("-webkit-from-flow(");
>+            text.append(quoteCSSStringIfNeeded(m_value.string));
>+            text.append(')');
>             break;
>         case CSS_URI:
>-            text = "url(" + quoteCSSURLIfNeeded(m_value.string) + ")";
>+            text.append("url(");
>+            text.append(quoteCSSURLIfNeeded(m_value.string));
>+            text.append(')');
>             break;
>         case CSS_IDENT:
>-            text = valueOrPropertyName(m_value.ident);
>+            text.append(valueOrPropertyName(m_value.ident));
>             break;
>         case CSS_ATTR: {
>             DEFINE_STATIC_LOCAL(const String, attrParen, ("attr("));
>@@ -789,13 +800,13 @@ String CSSPrimitiveValue::cssText() const
>             result.append(m_value.string);
>             result.append(')');
> 
>-            text = result.toString();
>+            text.append(result.toString());
>             break;
>         }
>         case CSS_COUNTER_NAME:
>-            text = "counter(";
>-            text += m_value.string;
>-            text += ")";
>+            text.append("counter(");
>+            text.append(m_value.string);
>+            text.append(')');
>             break;
>         case CSS_COUNTER: {
>             DEFINE_STATIC_LOCAL(const String, counterParen, ("counter("));
>@@ -817,8 +828,7 @@ String CSSPrimitiveValue::cssText() const
>                 result.append(listStyle);
>             }
>             result.append(')');
>-
>-            text = result.toString();
>+            text.append(result.toString());
>             break;
>         }
>         case CSS_RECT: {
>@@ -841,27 +851,27 @@ String CSSPrimitiveValue::cssText() const
>             result.append(rectVal->left()->cssText());
>             result.append(')');
> 
>-            text = result.toString();
>+            text.append(result.toString());
>             break;
>         }
>         case CSS_QUAD: {
>             Quad* quadVal = getQuadValue();
>-            Vector<UChar> result;
>-            result.reserveInitialCapacity(32);
>-            append(result, quadVal->top()->cssText());
>+            StringBuilder result;
>+            result.reserveCapacity(32);
>+            result.append(quadVal->top()->cssText());
>             if (quadVal->right() != quadVal->top() || quadVal->bottom() != quadVal->top() || quadVal->left() != quadVal->top()) {
>                 result.append(' ');
>-                append(result, quadVal->right()->cssText());
>+                result.append(quadVal->right()->cssText());
>                 if (quadVal->bottom() != quadVal->top() || quadVal->right() != quadVal->left()) {
>                     result.append(' ');
>-                    append(result, quadVal->bottom()->cssText());
>+                    result.append(quadVal->bottom()->cssText());
>                     if (quadVal->left() != quadVal->right()) {
>                         result.append(' ');
>-                        append(result, quadVal->left()->cssText());
>+                        result.append(quadVal->left()->cssText());
>                     }
>                 }
>             }
>-            text = String::adopt(result);
>+           text.append(result.toString());
>             break;
>         }
>         case CSS_RGBCOLOR:
>@@ -895,14 +905,14 @@ String CSSPrimitiveValue::cssText() const
>             }
> 
>             result.append(')');
>-            text = String::adopt(result);
>+            text.append(String::adopt(result));
>             break;
>         }
>         case CSS_PAIR:
>-            text = m_value.pair->first()->cssText();
>+            text.append(m_value.pair->first()->cssText());
>             if (m_value.pair->second() != m_value.pair->first()) {
>-                text += " ";
>-                text += m_value.pair->second()->cssText();
>+                text.append(' ');
>+                text.append(m_value.pair->second()->cssText());
>             }
>             break;
> #if ENABLE(DASHBOARD_SUPPORT)
>@@ -910,12 +920,12 @@ String CSSPrimitiveValue::cssText() const
>             for (DashboardRegion* region = getDashboardRegionValue(); region; region = region->m_next.get()) {
>                 if (!text.isEmpty())
>                     text.append(' ');
>-                text += "dashboard-region(";
>-                text += region->m_label;
>+                text.append("dashboard-region(");
>+                text.append(region->m_label);
>                 if (region->m_isCircle)
>-                    text += " circle";
>+                    text.append(" circle");
>                 else if (region->m_isRectangle)
>-                    text += " rectangle";
>+                    text.append(" rectangle");
>                 else
>                     break;
>                 if (region->top()->m_type == CSS_IDENT && region->top()->getIdent() == CSSValueInvalid) {
>@@ -927,32 +937,35 @@ String CSSPrimitiveValue::cssText() const
>                     ASSERT(region->left()->getIdent() == CSSValueInvalid);
>                 } else {
>                     text.append(' ');
>-                    text += region->top()->cssText() + " ";
>-                    text += region->right()->cssText() + " ";
>-                    text += region->bottom()->cssText() + " ";
>-                    text += region->left()->cssText();
>+                    text.append(region->top()->cssText());
>+                    text.append(' ');
>+                    text.append(region->right()->cssText());
>+                    text.append(' ');
>+                    text.append(region->bottom()->cssText());
>+                    text.append(' ');
>+                    text.append(region->left()->cssText());
>                 }
>-                text += ")";
>+                text.append(')');
>             }
>             break;
> #endif
>         case CSS_PARSER_OPERATOR: {
>-            char c = static_cast<char>(m_value.ident);
>-            text = String(&c, 1U);
>+            text.append(static_cast<char>(m_value.ident));
>             break;
>         }
>         case CSS_PARSER_IDENTIFIER:
>-            text = quoteCSSStringIfNeeded(m_value.string);
>+            text.append(quoteCSSStringIfNeeded(m_value.string));
>             break;
>         case CSS_SHAPE:
>-            text = m_value.shape->cssText();
>+            text.append(m_value.shape->cssText());
>             break;
>     }
> 
>+    String resultOfText = text.toString();
>     ASSERT(!cssTextCache().contains(this));
>-    cssTextCache().set(this, text);
>+    cssTextCache().set(this, resultOfText);
>     m_hasCachedCSSText = true;
>-    return text;
>+    return resultOfText;
> }
> 
> void CSSPrimitiveValue::addSubresourceStyleURLs(ListHashSet<KURL>& urls, const CSSStyleSheet* styleSheet)
>-- 
>1.7.1
>

Flags:

webkit.review.bot: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 66851: 104982 | 105141 | 105144 | 105339 | 107345 | 107347 | 111022 | 111257 | 111441 | 111570 | 111571 | 111955 | 111969 | 112126 | 113473 | 121666