[webkit-reviews] review denied: [Bug 66851] Fix CSSPrimitiveValue::cssText() to use StringBuilder : [Attachment 105144] Fix CSSPrimitiveValue::cssText() to use StringBuilder

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 25 07:40:02 PDT 2011


Darin Adler <darin at apple.com> has denied Oliver Varga
<voliver at inf.u-szeged.hu>'s request for review:
Bug 66851: Fix CSSPrimitiveValue::cssText() to use StringBuilder
https://bugs.webkit.org/show_bug.cgi?id=66851

Attachment 105144: Fix CSSPrimitiveValue::cssText() to use StringBuilder
https://bugs.webkit.org/attachment.cgi?id=105144&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list