Bug 54452 - Optimize Color::serialized()
Summary: Optimize Color::serialized()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-15 05:35 PST by Andreas Kling
Modified: 2011-02-15 11:48 PST (History)
1 user (show)

See Also:


Attachments
Proposed patch (2.99 KB, patch)
2011-02-15 05:38 PST, Andreas Kling
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-02-15 05:35:34 PST
Apply the optimizations used in CSSPrimitiveValue::cssText()'s handling of CSS_RGBCOLOR.
(Or in other words, build the serialized color strings manually instead of using String::format().)
Comment 1 Andreas Kling 2011-02-15 05:38:47 PST
Created attachment 82442 [details]
Proposed patch
Comment 2 Darin Adler 2011-02-15 09:55:59 PST
Comment on attachment 82442 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82442&action=review

Looks good.

> Source/WebCore/platform/graphics/Color.cpp:183
> +static inline void appendHexNumber(Vector<UChar>& vector, unsigned char number)

In newer code we’ve been using uint8_t for this more and more. Maybe we should make that a standard thing?

> Source/WebCore/platform/graphics/Color.cpp:185
> +    static const char hexDigits[17] = "0123456789abcdef";

Any particular reason to use lowercase hex here? I normally prefer uppercase when we have a choice. I guess the old code was doing lowercase.

> Source/WebCore/platform/graphics/Color.cpp:191
> +    size_t vectorSize = vector.size();
> +    vector.grow(vectorSize + 2);
> +
> +    vector[vectorSize] = hexDigits[number >> 4];
> +    vector[vectorSize + 1] = hexDigits[number & 0xF];

Is this more efficient than calling append twice?

> Source/WebCore/platform/graphics/Color.cpp:203
> +        result.reserveInitialCapacity(8);

Why 8 and not 7?

> Source/WebCore/platform/graphics/Color.cpp:207
> +        appendHexNumber(result, static_cast<unsigned char>(red()));
> +        appendHexNumber(result, static_cast<unsigned char>(green()));
> +        appendHexNumber(result, static_cast<unsigned char>(blue()));

Are these casts needed? Doesn’t the type conversion happen without a warning even without the cast?

> Source/WebCore/platform/graphics/Color.cpp:208
> +        return String::adopt(result);

Strings use memory more efficiently if they are created with the text in them, because then they use a single block. When we adopt a vector we do extra work that costs both space and also a bit of time.

For this code path, we could just use String::createUninitialized since we know the exact length ahead of time.

We could do the same in the other code path for 0 alpha.
Comment 3 Andreas Kling 2011-02-15 10:32:43 PST
Comment on attachment 82442 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82442&action=review

>> Source/WebCore/platform/graphics/Color.cpp:185
>> +    static const char hexDigits[17] = "0123456789abcdef";
> 
> Any particular reason to use lowercase hex here? I normally prefer uppercase when we have a choice. I guess the old code was doing lowercase.

This is in accordance with http://www.whatwg.org/specs/web-apps/current-work/#serialization-of-a-color which was the reason for adding Color::serialized() in the first place.

>> Source/WebCore/platform/graphics/Color.cpp:191
>> +    vector[vectorSize + 1] = hexDigits[number & 0xF];
> 
> Is this more efficient than calling append twice?

Doubtful. The logic was copied from WTF::appendNumber().
Comment 4 Andreas Kling 2011-02-15 11:48:16 PST
Committed r78596: <http://trac.webkit.org/changeset/78596>