54452 2011-02-15 05:35:34 -0800 Optimize Color::serialized() 2011-02-15 11:48:16 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 kling webkit-unassigned darin oldest_to_newest 351207 0 kling 2011-02-15 05:35:34 -0800 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().) 351209 1 82442 kling 2011-02-15 05:38:47 -0800 Created attachment 82442 Proposed patch 351366 2 82442 darin 2011-02-15 09:55:59 -0800 Comment on attachment 82442 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. 351392 3 82442 kling 2011-02-15 10:32:43 -0800 Comment on attachment 82442 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(). 351435 4 kling 2011-02-15 11:48:16 -0800 Committed r78596: <http://trac.webkit.org/changeset/78596> 82442 2011-02-15 05:38:47 -0800 2011-02-15 10:32:43 -0800 Proposed patch bug-54452.diff text/plain 3059 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAzNWE0YzU5Li5hNzE4MTNiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEg QEAKKzIwMTEtMDItMTUgIEFuZHJlYXMgS2xpbmcgIDxrbGluZ0B3ZWJraXQub3JnPgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE9wdGltaXplIENvbG9y OjpzZXJpYWxpemVkKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTU0NDUyCisKKyAgICAgICAgQXBwbHkgdGhlIG9wdGltaXphdGlvbnMgZnJvbSBDU1NQ cmltaXRpdmVWYWx1ZTo6Y3NzVGV4dCgpIFtDU1NfUkdCQ09MT1JdCisgICAgICAgIHRvIENvbG9y OjpzZXJpYWxpemVkKCkgLSBidWlsZCB0aGUgc2VyaWFsaXplZCBjb2xvciBzdHJpbmdzIG1hbnVh bGx5CisgICAgICAgIGluc3RlYWQgb2YgdXNpbmcgU3RyaW5nOjpmb3JtYXQoKS4KKworICAgICAg ICBUaGlzIGlzIHRoZSBmaXJzdCBzdGVwIGluIGNvbnNvbGlkYXRpbmcgdGhlIHZhcmlvdXMgY29s b3Igc2VyaWFsaXphdGlvbgorICAgICAgICBjb2RlIHBhdGhzIGluIFdlYkNvcmUuCisKKyAgICAg ICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9Db2xvci5jcHA6CisgICAgICAgIChXZWJDb3JlOjphcHBl bmRIZXhOdW1iZXIpOgorICAgICAgICAoV2ViQ29yZTo6Q29sb3I6OnNlcmlhbGl6ZWQpOgorCiAy MDExLTAyLTE1ICBEaXJrIFNjaHVsemUgIDxrcml0QHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmV2 aWV3ZWQgYnkgTmlrb2xhcyBaaW1tZXJtYW5uLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvQ29sb3IuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvQ29sb3IuY3BwCmluZGV4IGZhNzM0NmUuLmRmZTk5ZGQgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0NvbG9yLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9ncmFwaGljcy9Db2xvci5jcHAKQEAgLTE4MCwxNiArMTgwLDUyIEBAIENvbG9y OjpDb2xvcihjb25zdCBjaGFyKiBuYW1lKQogICAgIH0KIH0KIAorc3RhdGljIGlubGluZSB2b2lk IGFwcGVuZEhleE51bWJlcihWZWN0b3I8VUNoYXI+JiB2ZWN0b3IsIHVuc2lnbmVkIGNoYXIgbnVt YmVyKQoreworICAgIHN0YXRpYyBjb25zdCBjaGFyIGhleERpZ2l0c1sxN10gPSAiMDEyMzQ1Njc4 OWFiY2RlZiI7CisKKyAgICBzaXplX3QgdmVjdG9yU2l6ZSA9IHZlY3Rvci5zaXplKCk7CisgICAg dmVjdG9yLmdyb3codmVjdG9yU2l6ZSArIDIpOworCisgICAgdmVjdG9yW3ZlY3RvclNpemVdID0g aGV4RGlnaXRzW251bWJlciA+PiA0XTsKKyAgICB2ZWN0b3JbdmVjdG9yU2l6ZSArIDFdID0gaGV4 RGlnaXRzW251bWJlciAmIDB4Rl07Cit9CisKIFN0cmluZyBDb2xvcjo6c2VyaWFsaXplZCgpIGNv bnN0CiB7Ci0gICAgaWYgKGFscGhhKCkgPT0gMHhGRikKLSAgICAgICAgcmV0dXJuIFN0cmluZzo6 Zm9ybWF0KCIjJTAyeCUwMnglMDJ4IiwgcmVkKCksIGdyZWVuKCksIGJsdWUoKSk7CisgICAgREVG SU5FX1NUQVRJQ19MT0NBTChjb25zdCBTdHJpbmcsIGNvbW1hU3BhY2UsICgiLCAiKSk7CisgICAg REVGSU5FX1NUQVRJQ19MT0NBTChjb25zdCBTdHJpbmcsIHJnYmFQYXJlbiwgKCJyZ2JhKCIpKTsK KyAgICBERUZJTkVfU1RBVElDX0xPQ0FMKGNvbnN0IFN0cmluZywgemVyb1BvaW50WmVybywgKCIw LjAiKSk7CisKKyAgICBWZWN0b3I8VUNoYXI+IHJlc3VsdDsKKworICAgIGlmIChhbHBoYSgpID09 IDB4RkYpIHsKKyAgICAgICAgcmVzdWx0LnJlc2VydmVJbml0aWFsQ2FwYWNpdHkoOCk7CisgICAg ICAgIHJlc3VsdC5hcHBlbmQoJyMnKTsKKyAgICAgICAgYXBwZW5kSGV4TnVtYmVyKHJlc3VsdCwg c3RhdGljX2Nhc3Q8dW5zaWduZWQgY2hhcj4ocmVkKCkpKTsKKyAgICAgICAgYXBwZW5kSGV4TnVt YmVyKHJlc3VsdCwgc3RhdGljX2Nhc3Q8dW5zaWduZWQgY2hhcj4oZ3JlZW4oKSkpOworICAgICAg ICBhcHBlbmRIZXhOdW1iZXIocmVzdWx0LCBzdGF0aWNfY2FzdDx1bnNpZ25lZCBjaGFyPihibHVl KCkpKTsKKyAgICAgICAgcmV0dXJuIFN0cmluZzo6YWRvcHQocmVzdWx0KTsKKyAgICB9CisKKyAg ICByZXN1bHQucmVzZXJ2ZUluaXRpYWxDYXBhY2l0eSgzMik7CisKKyAgICBhcHBlbmQocmVzdWx0 LCByZ2JhUGFyZW4pOworICAgIGFwcGVuZE51bWJlcihyZXN1bHQsIHN0YXRpY19jYXN0PHVuc2ln bmVkIGNoYXI+KHJlZCgpKSk7CisgICAgYXBwZW5kKHJlc3VsdCwgY29tbWFTcGFjZSk7CisgICAg YXBwZW5kTnVtYmVyKHJlc3VsdCwgc3RhdGljX2Nhc3Q8dW5zaWduZWQgY2hhcj4oZ3JlZW4oKSkp OworICAgIGFwcGVuZChyZXN1bHQsIGNvbW1hU3BhY2UpOworICAgIGFwcGVuZE51bWJlcihyZXN1 bHQsIHN0YXRpY19jYXN0PHVuc2lnbmVkIGNoYXI+KGJsdWUoKSkpOworICAgIGFwcGVuZChyZXN1 bHQsIGNvbW1hU3BhY2UpOwogCiAgICAgLy8gTWF0Y2ggR2Vja28gKCIwLjAiIGZvciB6ZXJvLCA1 IGRlY2ltYWxzIGZvciBhbnl0aGluZyBlbHNlKQogICAgIGlmICghYWxwaGEoKSkKLSAgICAgICAg cmV0dXJuIFN0cmluZzo6Zm9ybWF0KCJyZ2JhKCV1LCAldSwgJXUsIDAuMCkiLCByZWQoKSwgZ3Jl ZW4oKSwgYmx1ZSgpKTsKKyAgICAgICAgYXBwZW5kKHJlc3VsdCwgemVyb1BvaW50WmVybyk7Cisg ICAgZWxzZQorICAgICAgICBhcHBlbmQocmVzdWx0LCBTdHJpbmc6OmZvcm1hdCgiJS41ZiIsIGFs cGhhKCkgLyAyNTUuMGYpKTsKIAotICAgIHJldHVybiBTdHJpbmc6OmZvcm1hdCgicmdiYSgldSwg JXUsICV1LCAlLjVmKSIsIHJlZCgpLCBncmVlbigpLCBibHVlKCksIGFscGhhKCkgLyAyNTUuMGYp OworICAgIHJlc3VsdC5hcHBlbmQoJyknKTsKKyAgICByZXR1cm4gU3RyaW5nOjphZG9wdChyZXN1 bHQpOwogfQogCiBTdHJpbmcgQ29sb3I6Om5hbWUoKSBjb25zdAo=