101421 2012-11-06 19:12:56 -0800 StringBuilder::append(UChar) with an 8 bit quantity shouldn't change the contents to 16 bits 2012-11-08 11:09:35 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 msaboff msaboff benjamin ojan webkit.review.bot oldest_to_newest 760297 0 msaboff 2012-11-06 19:12:56 -0800 StringBuilder objects keep track of whether or not the contents are all 8 bit in m_is8Bit. If appendChar(UChar) is called, m_is8Bit is set to false. An improvement is to look at the argument to StringBuilder::append(UChar) when m_is8Bit is true and see if the value can fit in 8 bits. If so, keep m_is8Bit true and append the character to the 8 bit buffer. 760396 1 172720 msaboff 2012-11-06 22:14:07 -0800 Created attachment 172720 Patch This appears to be a progression on most of the Parser performance tests. Here is a comparison of the arithmetic average of 8 test runs for the parser tests: Test Baseline With Patch Change html-parser 2026.46ms 1989.21ms +2.8% html5-full-render 4274.28ms 4176.83ms +2.3% innerHTML-setter 263.14runs/s 269.21runs/s +2.3% simple-url 48.47runs/s 51.14runs/s +5.5% textarea-parsing 82.73runs/s 83.00runs/s +0.3% tiny-inner-HTML 5.08runs/s 5.07runs/s -0.2% url-parser 111.32runs/r 117.61runs/s +5.7% xml-parser 7.07runs/s 7.20runs/s +1.8% 760418 2 172720 webkit.review.bot 2012-11-06 23:13:48 -0800 Comment on attachment 172720 Patch Clearing flags on attachment: 172720 Committed r133726: <http://trac.webkit.org/changeset/133726> 760419 3 webkit.review.bot 2012-11-06 23:13:51 -0800 All reviewed patches have been landed. Closing bug. 760746 4 172720 darin 2012-11-07 08:48:56 -0800 Comment on attachment 172720 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172720&action=review > Source/WTF/wtf/text/StringBuilder.cpp:251 > + if (length == 1 && !(*characters & ~0xff)) { > + // Append as 8 bit character > + LChar lChar = static_cast<LChar>(*characters); > + append(&lChar, 1); > + return; > + } It seems a little strange to have the 1-character special case here instead of in the append(UChar) function. 760821 5 172720 darin 2012-11-07 10:08:49 -0800 Comment on attachment 172720 Patch Now that I have some time to think about it, I realize I would have done this differently. I would have left the StringBuilder::append(const UChar, unsigned) function alone. And I would have added this to the start of append(UChar c): if (!(c & ~0xff)) { append(static_cast<LChar>(c)); return; } I wonder whether that version of the patch would have better or worse performance than the one you landed? 761105 6 msaboff 2012-11-07 15:28:18 -0800 (In reply to comment #5) > (From update of attachment 172720 [details]) > Now that I have some time to think about it, I realize I would have done this differently. I would have left the StringBuilder::append(const UChar, unsigned) function alone. And I would have added this to the start of append(UChar c): > > if (!(c & ~0xff)) { > append(static_cast<LChar>(c)); > return; > } > > I wonder whether that version of the patch would have better or worse performance than the one you landed? I'll try that out, measure performance and report. I suspect that it will be pretty close to equal. The reason I went the way I did was to reduce bloat due to a larger inlined function. Therefore I'll also check object size differences as well. 761939 7 ojan 2012-11-08 10:30:35 -0800 It's hard to tell with all the noise, but it also looks like this was a ~11% improvement in setting style. http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstyleprototype/report.html?rev=166663&graph=jslib_style_prototype_Prototype___setStyle__&trace=score&history=50 761985 8 ojan 2012-11-08 11:09:35 -0800 Actually, it's an improvement on a bunch of the jquery benchmarks: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=166613&graph=jslib_style_jquery_jQuery___height___x10&trace=score&history=150 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=166613&graph=jslib_style_jquery_jQuery___width___x10&trace=score&history=150 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=166664&graph=jslib_style_jquery_jQuery___css_color__x100&trace=score&history=150 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=166664&graph=jslib_style_jquery_jQuery____is__visible_&trace=score&history=150 Probably some others too, but I'll stop there. :) 172720 2012-11-06 22:14:07 -0800 2012-11-07 10:08:49 -0800 Patch 101421.patch text/plain 2548 msaboff SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxMzM3MjApCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDEyLTExLTA2ICBNaWNoYWVsIFNhYm9mZiAgPG1z YWJvZmZAYXBwbGUuY29tPgorCisgICAgICAgIFN0cmluZ0J1aWxkZXI6OmFwcGVuZChVQ2hhcikg d2l0aCBhbiA4IGJpdCBxdWFudGl0eSBzaG91bGRuJ3QgY2hhbmdlIHRoZSBjb250ZW50cyB0byAx NiBiaXRzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MDE0MjEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJ ZiB0aGUgc3RyaW5nIGJ1aWxkZXIgY29udGFpbnMgb25seSA4IGJpdCBkYXRhLCBjaGVjayBpZiB0 aGUgY2hhcmFjdGVyIGJlaW5nIGFwcGVuZGVkIGNvbnRhaW5zCisgICAgICAgIDggYml0IGRhdGEu ICBJZiBzbywgYXBwZW5kIGl0IHRvIHRoZSA4IGJpdCBidWZmZXIgaW5zdGVhZCBvZiBjb252ZXJ0 aW5nIHRoZSBidWZmZXIgdG8gMTYgYml0cy4KKworICAgICAgICAqIHd0Zi90ZXh0L1N0cmluZ0J1 aWxkZXIuY3BwOgorICAgICAgICAoV1RGOjpTdHJpbmdCdWlsZGVyOjphcHBlbmQpOgorICAgICAg ICAqIHd0Zi90ZXh0L1N0cmluZ0J1aWxkZXIuaDoKKyAgICAgICAgKFdURjo6U3RyaW5nQnVpbGRl cjo6YXBwZW5kKToKKwogMjAxMi0xMS0wNiAgQmVuamFtaW4gUG91bGFpbiAgPGJlbmphbWluQHdl YmtpdC5vcmc+CiAKICAgICAgICAgU3BlZWQgdXAgVHJhbnNmb3JtYXRpb25NYXRyaXg6Om11bHRp cGx5KCkgb24gbW9kZXJuIEFSTQpJbmRleDogU291cmNlL1dURi93dGYvdGV4dC9TdHJpbmdCdWls ZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0J1aWxkZXIu Y3BwCShyZXZpc2lvbiAxMzM2NzIpCisrKyBTb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0J1aWxk ZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yNDMsNiArMjQzLDEzIEBAIHZvaWQgU3RyaW5nQnVp bGRlcjo6YXBwZW5kKGNvbnN0IFVDaGFyKiAKICAgICBBU1NFUlQoY2hhcmFjdGVycyk7CiAKICAg ICBpZiAobV9pczhCaXQpIHsKKyAgICAgICAgaWYgKGxlbmd0aCA9PSAxICYmICEoKmNoYXJhY3Rl cnMgJiB+MHhmZikpIHsKKyAgICAgICAgICAgIC8vIEFwcGVuZCBhcyA4IGJpdCBjaGFyYWN0ZXIK KyAgICAgICAgICAgIExDaGFyIGxDaGFyID0gc3RhdGljX2Nhc3Q8TENoYXI+KCpjaGFyYWN0ZXJz KTsKKyAgICAgICAgICAgIGFwcGVuZCgmbENoYXIsIDEpOworICAgICAgICAgICAgcmV0dXJuOwor ICAgICAgICB9CisKICAgICAgICAgLy8gQ2FsY3VsYXRlIHRoZSBuZXcgc2l6ZSBvZiB0aGUgYnVp bGRlciBhZnRlciBhcHBlbmRpbmcuCiAgICAgICAgIHVuc2lnbmVkIHJlcXVpcmVkTGVuZ3RoID0g bGVuZ3RoICsgbV9sZW5ndGg7CiAgICAgICAgIGlmIChyZXF1aXJlZExlbmd0aCA8IGxlbmd0aCkK SW5kZXg6IFNvdXJjZS9XVEYvd3RmL3RleHQvU3RyaW5nQnVpbGRlci5oCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9XVEYvd3RmL3RleHQvU3RyaW5nQnVpbGRlci5oCShyZXZpc2lvbiAxMzM2NzIpCisr KyBTb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0J1aWxkZXIuaAkod29ya2luZyBjb3B5KQpAQCAt MTExLDEwICsxMTEsMTggQEAgcHVibGljOgogCiAgICAgdm9pZCBhcHBlbmQoVUNoYXIgYykKICAg ICB7Ci0gICAgICAgIGlmIChtX2J1ZmZlciAmJiAhbV9pczhCaXQgJiYgbV9sZW5ndGggPCBtX2J1 ZmZlci0+bGVuZ3RoKCkgJiYgbV9zdHJpbmcuaXNOdWxsKCkpCi0gICAgICAgICAgICBtX2J1ZmZl ckNoYXJhY3RlcnMxNlttX2xlbmd0aCsrXSA9IGM7Ci0gICAgICAgIGVsc2UKLSAgICAgICAgICAg IGFwcGVuZCgmYywgMSk7CisgICAgICAgIGlmIChtX2J1ZmZlciAmJiBtX2xlbmd0aCA8IG1fYnVm ZmVyLT5sZW5ndGgoKSAmJiBtX3N0cmluZy5pc051bGwoKSkgeworICAgICAgICAgICAgaWYgKCFt X2lzOEJpdCkgeworICAgICAgICAgICAgICAgIG1fYnVmZmVyQ2hhcmFjdGVyczE2W21fbGVuZ3Ro KytdID0gYzsKKyAgICAgICAgICAgICAgICByZXR1cm47CisgICAgICAgICAgICB9CisKKyAgICAg ICAgICAgIGlmICghKGMgJiB+MHhmZikpIHsKKyAgICAgICAgICAgICAgICBtX2J1ZmZlckNoYXJh Y3RlcnM4W21fbGVuZ3RoKytdID0gc3RhdGljX2Nhc3Q8TENoYXI+KGMpOworICAgICAgICAgICAg ICAgIHJldHVybjsKKyAgICAgICAgICAgIH0KKyAgICAgICAgfQorICAgICAgICBhcHBlbmQoJmMs IDEpOwogICAgIH0KIAogICAgIHZvaWQgYXBwZW5kKExDaGFyIGMpCg==