Summary: | Optimize Color::serialized() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Andreas Kling
2011-02-15 05:35:34 PST
Created attachment 82442 [details]
Proposed patch
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 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(). Committed r78596: <http://trac.webkit.org/changeset/78596> |