100575 2012-10-26 16:46:57 -0700 Try to create AtomicString as 8 bit where possible 2012-10-28 14:33:16 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 msaboff msaboff benjamin webkit.review.bot oldest_to_newest 752577 0 msaboff 2012-10-26 16:46:57 -0700 Most AtomicString's contain strings composed of 8 bit data even when created using a UChar* source. When creating an AtomicString with UChar data, the data should be checked for 8 bit characters and an 8 bit string should be created if possible. 752611 1 171058 msaboff 2012-10-26 17:23:37 -0700 Created attachment 171058 Patch 752829 2 171058 webkit.review.bot 2012-10-27 13:33:32 -0700 Comment on attachment 171058 Patch Clearing flags on attachment: 171058 Committed r132739: <http://trac.webkit.org/changeset/132739> 752830 3 webkit.review.bot 2012-10-27 13:33:36 -0700 All reviewed patches have been landed. Closing bug. 752837 4 171058 darin 2012-10-27 14:04:58 -0700 Comment on attachment 171058 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171058&action=review > Source/WTF/wtf/text/StringImpl.cpp:276 > + LChar* data; > + RefPtr<StringImpl> string = createUninitialized(length, data); > + > + for (size_t i = 0; i < length; ++i) { > + if (characters[i] & 0xff00) > + return create(characters, length); > + data[i] = static_cast<LChar>(characters[i]); > + } What’s the performance hit like here? Did you do any measurement. We’re doing a second memory allocation every time we end up creating a 16-bit string, so that case is a bit slow. Also, I could imagine we might get better speed if we did the Latin-1 preflight using the techniques from ASCIIFastPath that process many characters at a time quickly, even though that would be a separate loop. 752888 5 msaboff 2012-10-27 21:50:31 -0700 (In reply to comment #4) > (From update of attachment 171058 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171058&action=review > > > Source/WTF/wtf/text/StringImpl.cpp:276 > > + LChar* data; > > + RefPtr<StringImpl> string = createUninitialized(length, data); > > + > > + for (size_t i = 0; i < length; ++i) { > > + if (characters[i] & 0xff00) > > + return create(characters, length); > > + data[i] = static_cast<LChar>(characters[i]); > > + } > > What’s the performance hit like here? Did you do any measurement. I ran benchmarks on a set of changes including this one. I ran sunspider, V8v7 and kraken. No change in their performance. I then ran the Parser performance tests and overall there wasn't a change. > We’re doing a second memory allocation every time we end up creating a 16-bit string, so that case is a bit slow. Also, I could imagine we might get better speed if we did the Latin-1 preflight using the techniques from ASCIIFastPath that process many characters at a time quickly, even though that would be a separate loop. This function is intended for mostly 8 bit strings. For AtomicStrings, the 16 bit path appears to be rarely used (a little over 1% of the time in my tests). I instrumented the 8 and 16 bit paths. Here are the number of times we took the 16 bit path for a given website: apple.com 0 cnn.com 3 yahoo.com 1 facebook.com 1 daringfireball.net 42 ebay.com 1 google.com 1 reddit.com 0 wikipedia.com 55 amazon.com 0 lemonde.fr 5 japantimes.co.jp 7 weather.com 1 espn.com 0 webkit.org 0 Across these sites, we took the 8 bit path 10960 times and the 16 bit path 117 times. I thought about employing the techniques in ASCIIFastPath, but the performance tests didn't show that there was an issue. If you like, I can file a defect to investigate performance improvements. 752993 6 benjamin 2012-10-28 14:33:16 -0700 Please measure on ARM, both branches and memory accesses are relatively more expensive. Since the 16bits case is only used in 1% of the case, consider branching in 0xFF00 only once at the end instead of for every iteration. 171058 2012-10-26 17:23:37 -0700 2012-10-27 14:04:58 -0700 Patch 100575.patch text/plain 3568 msaboff SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxMzI3MTIpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTEwLTI2ICBNaWNoYWVsIFNhYm9mZiAgPG1z YWJvZmZAYXBwbGUuY29tPgorCisgICAgICAgIFRyeSB0byBjcmVhdGUgQXRvbWljU3RyaW5nIGFz IDggYml0IHdoZXJlIHBvc3NpYmxlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xMDA1NzUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBBZGRlZCBTdHJpbmdJbXBsOjpjcmVhdGU4Qml0SWZQb3NzaWJsZSgpIHRo YXQgZmlyc3QgdHJpZXMgdG8gY3JlYXRlIGFuIDggYml0IHN0cmluZy4gIElmIGl0IGZpbmRzIGEg MTYgYml0IGNoYXJhY3RlcgorICAgICAgICBkdXJpbmcgcHJvY2Vzc2luZywgaXQgY2FsbHMgdGhl IHN0YW5kYXJkIGNyZWF0ZSgpIG1ldGhvZC4gIFRoZSBhc3N1bXB0aW9uIGlzIHRoYXQgdGhpcyB3 aWxsIGJlIHVzZWQgb24gbW9zdGx5IDggYml0CisgICAgICAgIHN0cmluZ3MgYW5kIG9uZXMgdGhh dCBhcmUgc2hvcnRlciAoaW4gdGhlIHRlbnMgb2YgY2hhcmFjdGVycykuICBDaGFuZ2VkIEF0b21p Y1N0cmluZyB0byB1c2UgdGhlIG5ldyBjcmVhdGlvbiBtZXRob2QKKyAgICAgICAgZm9yIFVDaGFy IGJhc2VkIGNvbnN0cnVjdGlvbi4KKworICAgICAgICAqIHd0Zi90ZXh0L0F0b21pY1N0cmluZy5j cHA6CisgICAgICAgIChXVEY6OlVDaGFyQnVmZmVyVHJhbnNsYXRvcjo6dHJhbnNsYXRlKToKKyAg ICAgICAgKiB3dGYvdGV4dC9TdHJpbmdJbXBsLmNwcDoKKyAgICAgICAgKFdURjo6U3RyaW5nSW1w bDo6Y3JlYXRlOEJpdElmUG9zc2libGUpOgorICAgICAgICAqIHd0Zi90ZXh0L1N0cmluZ0ltcGwu aDoKKyAgICAgICAgKFdURjo6U3RyaW5nSW1wbDo6Y3JlYXRlOEJpdElmUG9zc2libGUpOgorCiAy MDEyLTEwLTI2ICBTaGVyaWZmIEJvdCAgPHdlYmtpdC5yZXZpZXcuYm90QGdtYWlsLmNvbT4KIAog ICAgICAgICBVbnJldmlld2VkLCByb2xsaW5nIG91dCByMTMyNjg5LgpJbmRleDogU291cmNlL1dU Ri93dGYvdGV4dC9BdG9taWNTdHJpbmcuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvd3Rm L3RleHQvQXRvbWljU3RyaW5nLmNwcAkocmV2aXNpb24gMTMyNTEwKQorKysgU291cmNlL1dURi93 dGYvdGV4dC9BdG9taWNTdHJpbmcuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMzUsNyArMTM1LDcg QEAgc3RydWN0IFVDaGFyQnVmZmVyVHJhbnNsYXRvciB7CiAKICAgICBzdGF0aWMgdm9pZCB0cmFu c2xhdGUoU3RyaW5nSW1wbComIGxvY2F0aW9uLCBjb25zdCBVQ2hhckJ1ZmZlciYgYnVmLCB1bnNp Z25lZCBoYXNoKQogICAgIHsKLSAgICAgICAgbG9jYXRpb24gPSBTdHJpbmdJbXBsOjpjcmVhdGUo YnVmLnMsIGJ1Zi5sZW5ndGgpLmxlYWtSZWYoKTsKKyAgICAgICAgbG9jYXRpb24gPSBTdHJpbmdJ bXBsOjpjcmVhdGU4Qml0SWZQb3NzaWJsZShidWYucywgYnVmLmxlbmd0aCkubGVha1JlZigpOwog ICAgICAgICBsb2NhdGlvbi0+c2V0SGFzaChoYXNoKTsKICAgICAgICAgbG9jYXRpb24tPnNldElz QXRvbWljKHRydWUpOwogICAgIH0KSW5kZXg6IFNvdXJjZS9XVEYvd3RmL3RleHQvU3RyaW5nSW1w bC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dURi93dGYvdGV4dC9TdHJpbmdJbXBsLmNwcAko cmV2aXNpb24gMTMyNTEwKQorKysgU291cmNlL1dURi93dGYvdGV4dC9TdHJpbmdJbXBsLmNwcAko d29ya2luZyBjb3B5KQpAQCAtMjYxLDYgKzI2MSwyMyBAQCBQYXNzUmVmUHRyPFN0cmluZ0ltcGw+ IFN0cmluZ0ltcGw6OmNyZWF0CiAgICAgcmV0dXJuIHN0cmluZy5yZWxlYXNlKCk7CiB9CiAKK1Bh c3NSZWZQdHI8U3RyaW5nSW1wbD4gU3RyaW5nSW1wbDo6Y3JlYXRlOEJpdElmUG9zc2libGUoY29u c3QgVUNoYXIqIGNoYXJhY3RlcnMsIHVuc2lnbmVkIGxlbmd0aCkKK3sKKyAgICBpZiAoIWNoYXJh Y3RlcnMgfHwgIWxlbmd0aCkKKyAgICAgICAgcmV0dXJuIGVtcHR5KCk7CisKKyAgICBMQ2hhciog ZGF0YTsKKyAgICBSZWZQdHI8U3RyaW5nSW1wbD4gc3RyaW5nID0gY3JlYXRlVW5pbml0aWFsaXpl ZChsZW5ndGgsIGRhdGEpOworCisgICAgZm9yIChzaXplX3QgaSA9IDA7IGkgPCBsZW5ndGg7ICsr aSkgeworICAgICAgICBpZiAoY2hhcmFjdGVyc1tpXSAmIDB4ZmYwMCkKKyAgICAgICAgICAgIHJl dHVybiBjcmVhdGUoY2hhcmFjdGVycywgbGVuZ3RoKTsKKyAgICAgICAgZGF0YVtpXSA9IHN0YXRp Y19jYXN0PExDaGFyPihjaGFyYWN0ZXJzW2ldKTsKKyAgICB9CisKKyAgICByZXR1cm4gc3RyaW5n LnJlbGVhc2UoKTsKK30KKwogUGFzc1JlZlB0cjxTdHJpbmdJbXBsPiBTdHJpbmdJbXBsOjpjcmVh dGUoY29uc3QgTENoYXIqIHN0cmluZykKIHsKICAgICBpZiAoIXN0cmluZykKSW5kZXg6IFNvdXJj ZS9XVEYvd3RmL3RleHQvU3RyaW5nSW1wbC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvd3Rm L3RleHQvU3RyaW5nSW1wbC5oCShyZXZpc2lvbiAxMzI1MTApCisrKyBTb3VyY2UvV1RGL3d0Zi90 ZXh0L1N0cmluZ0ltcGwuaAkod29ya2luZyBjb3B5KQpAQCAtMzUyLDYgKzM1Miw3IEBAIHB1Ymxp YzoKIAogICAgIFdURl9FWFBPUlRfU1RSSU5HX0FQSSBzdGF0aWMgUGFzc1JlZlB0cjxTdHJpbmdJ bXBsPiBjcmVhdGUoY29uc3QgVUNoYXIqLCB1bnNpZ25lZCBsZW5ndGgpOwogICAgIFdURl9FWFBP UlRfU1RSSU5HX0FQSSBzdGF0aWMgUGFzc1JlZlB0cjxTdHJpbmdJbXBsPiBjcmVhdGUoY29uc3Qg TENoYXIqLCB1bnNpZ25lZCBsZW5ndGgpOworICAgIFdURl9FWFBPUlRfU1RSSU5HX0FQSSBzdGF0 aWMgUGFzc1JlZlB0cjxTdHJpbmdJbXBsPiBjcmVhdGU4Qml0SWZQb3NzaWJsZShjb25zdCBVQ2hh ciosIHVuc2lnbmVkIGxlbmd0aCk7CiAgICAgQUxXQVlTX0lOTElORSBzdGF0aWMgUGFzc1JlZlB0 cjxTdHJpbmdJbXBsPiBjcmVhdGUoY29uc3QgY2hhciogcywgdW5zaWduZWQgbGVuZ3RoKSB7IHJl dHVybiBjcmVhdGUocmVpbnRlcnByZXRfY2FzdDxjb25zdCBMQ2hhcio+KHMpLCBsZW5ndGgpOyB9 CiAgICAgV1RGX0VYUE9SVF9TVFJJTkdfQVBJIHN0YXRpYyBQYXNzUmVmUHRyPFN0cmluZ0ltcGw+ IGNyZWF0ZShjb25zdCBMQ2hhciopOwogICAgIEFMV0FZU19JTkxJTkUgc3RhdGljIFBhc3NSZWZQ dHI8U3RyaW5nSW1wbD4gY3JlYXRlKGNvbnN0IGNoYXIqIHMpIHsgcmV0dXJuIGNyZWF0ZShyZWlu dGVycHJldF9jYXN0PGNvbnN0IExDaGFyKj4ocykpOyB9Cg==