[Webkit-unassigned] [Bug 66851] Fix CSSPrimitiveValue::cssText() to use StringBuilder

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66851


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105144|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2011-08-25 07:40:02 PST ---
(From update of attachment 105144)
View in context: https://bugs.webkit.org/attachment.cgi?id=105144&action=review

My comments include many ideas that are ways to do better in the future. The one thing you should definitely fix before landing is the append(c) thing. Please consider the others too, though. We don’t want any changes that actually make things slower.

> Source/WebCore/css/CSSPrimitiveValue.cpp:638
> +            text.append(formatNumber(m_value.num));

Suggestion for further improvement:

There’s a significant performance gain available if we can come up with a way to do these without creating a string and then appending it. We need an equivalent of formatNumber that works directly with the StringBuilder, or some way that does not involve memory allocation other than what StringBuilder does.

> Source/WebCore/css/CSSPrimitiveValue.cpp:716
> -            text = quoteCSSStringIfNeeded(m_value.string);
> +            text.append(quoteCSSStringIfNeeded(m_value.string));

Similarly, for the quoted string case it's much more efficient to do the quoting without allocating an entire new string.

> Source/WebCore/css/CSSPrimitiveValue.cpp:732
>              DEFINE_STATIC_LOCAL(const String, attrParen, ("attr("));

There's no value in having this pre-allocated in a String. It can just be an append of a const char*.

> Source/WebCore/css/CSSPrimitiveValue.cpp:755
>              DEFINE_STATIC_LOCAL(const String, rectParen, ("rect("));

There's no value in having this pre-allocated in a String. It can just be an append of a const char*.

> Source/WebCore/css/CSSPrimitiveValue.cpp:-737
> -            Vector<UChar> result;
> -            result.reserveInitialCapacity(32);

Are you sure the StringBuilder is more efficient than the Vector<UChar> was?

> Source/WebCore/css/CSSPrimitiveValue.cpp:762
> +            result.append(rectVal->top()->cssText());

This is another opportunity to be much more efficient. If we had a function that appended the CSS text to a StringBuilder without ever creating a string, we could probably save many extra string allocations here.

> Source/WebCore/css/CSSPrimitiveValue.cpp:794
> -            appendNumber(result, static_cast<unsigned char>(color.red()));
> -            append(result, commaSpace);
> +            result.append(String::number(color.red()));

Again, making a string out of the number just to put it in the string builder is quite inefficient. It’s a step backwards to go from the appendNumber function, which can and has been optimized, to an implementation that explicitly allocates a string each time.

> Source/WebCore/css/CSSPrimitiveValue.cpp:853
>              char c = static_cast<char>(m_value.ident);
> -            text = String(&c, 1U);
> +            text.append(String(&c, 1U));

This is wrong. It can just be append(c) or even append(static_cast<char>(m_value.ident)). Explicitly creating a string makes us dow double allocation.

> Source/WebCore/css/CSSPrimitiveValue.cpp:867
> -    cssTextCache().set(this, text);
> +    cssTextCache().set(this, text.toString());
>      m_hasCachedCSSText = true;
> -    return text;
> +    return text.toString();

Calling toString() twice seems a bit inelegant. I would put the string into a local variable instead.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.