117586 2013-06-13 03:23:44 -0700 Optimize String::fromUTF8 for ASCII 2013-06-22 19:48:25 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED BlinkMergeCandidate P2 Normal --- 1 gyuyoung.kim gyuyoung.kim benjamin cmarcelo commit-queue rniwa oldest_to_newest 899965 0 gyuyoung.kim 2013-06-13 03:23:44 -0700 We probably need to consider to merge with https://src.chromium.org/viewvc/blink?view=rev&revision=152243. Current String::fromUTF8() implementation converts 8 bit ASCII character into 16 bit. Instead of always trying to convert into a 16 bit buffer, we can add a call to charactersAreAllASCII. In the common case when characters are ASCII, we directly copy it into an 8 bit string buffer. 899969 1 204566 gyuyoung.kim 2013-06-13 03:26:38 -0700 Created attachment 204566 Patch 900141 2 204566 bfulgham 2013-06-13 09:52:40 -0700 Comment on attachment 204566 Patch Looks good. Thanks for porting this over! 900144 3 204566 commit-queue 2013-06-13 09:56:19 -0700 Comment on attachment 204566 Patch Clearing flags on attachment: 204566 Committed r151556: <http://trac.webkit.org/changeset/151556> 900145 4 commit-queue 2013-06-13 09:56:22 -0700 All reviewed patches have been landed. Closing bug. 900926 5 benjamin 2013-06-16 19:36:10 -0700 We looked into this change but I was unconvinced. Did you get actual performance numbers? If not, please get numbers and follow up (or revert). 900927 6 gyuyoung.kim 2013-06-16 20:12:22 -0700 (In reply to comment #5) > We looked into this change but I was unconvinced. > > Did you get actual performance numbers? If not, please get numbers and follow up (or revert). It looks Adam Barth didn't get meaning performance number. Anyway, I will try to get performance number. If failed, let's consider to revert this patch. 902039 7 rniwa 2013-06-20 00:21:28 -0700 (In reply to comment #6) > (In reply to comment #5) > > We looked into this change but I was unconvinced. > > > > Did you get actual performance numbers? If not, please get numbers and follow up (or revert). > > It looks Adam Barth didn't get meaning performance number. Anyway, I will try to get performance number. If failed, let's consider to revert this patch. Did you get a number? I did look into this but it was an overall perf. regression. I'd like to know on which test you saw a performance improvement. 902042 8 gyuyoung.kim 2013-06-20 00:44:29 -0700 (In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > We looked into this change but I was unconvinced. > > > > > > Did you get actual performance numbers? If not, please get numbers and follow up (or revert). > > > > It looks Adam Barth didn't get meaning performance number. Anyway, I will try to get performance number. If failed, let's consider to revert this patch. > > Did you get a number? I did look into this but it was an overall perf. regression. > > I'd like to know on which test you saw a performance improvement. Sorry. I don't get meaningful number on this patch yet. If there is a performance regression after this patch, I don't mind to roll this patch out for now. When I get a number or better number, I will pong you guys. 902076 9 gyuyoung.kim 2013-06-20 03:43:58 -0700 I simply run sunspider test w/ and w/o this patch. However, I fail to get a meaningful difference between them. String tests of sunspider took 45 ms ~ 47 ms generally both of them. string : 47.1 ms - base64: 3.7 ms - fasta: 8.4 ms - tagcloud: 9.2 ms - unpack-code: 20.5 ms - validate-input: 5.3 ms I'm not sure if sunspider is correct tool for this patch. However, it shows this patch doesn't improve js string performance at least. I will try to get a number a little further. 902170 10 benjamin 2013-06-20 09:07:45 -0700 (In reply to comment #9) > I'm not sure if sunspider is correct tool for this patch. However, it shows this patch doesn't improve js string performance at least. I will try to get a number a little further. I think a good test would be checking how much time we spend in this function when loading a non-latin1 website. For example, take a well known Korean website, can it. Load the canned version with and without the patch and measure the function time with DTrace. 902478 11 gyuyoung.kim 2013-06-21 01:43:42 -0700 (In reply to comment #10) > For example, take a well known Korean website, can it. Load the canned version with and without the patch and measure the function time with DTrace. I measure total execution time on String::fromUTF8() using "struct timeval". I used popular korean web sites (http://www.naver.com and http://www.daum.net) I measure the execution time with entry point and entrance point. I calculated the time gap using both of them. Result below shows total execution time in fromUTF8() until finishing front page loading. When using this patch, 1. naver : 250 us ~ 300 us 2. daum : 200 us ~ 250 us When using previous one, 1. naver : 1,160 us ~ 1,300 us 2. daum : 800 us ~ 900 us According to this profiling, this patch looks reduce execution time by about 2 ~ 3 times. If there is any problems or mistakes in my test, please let me know. 902819 12 benjamin 2013-06-22 19:48:25 -0700 Sounds good to me then. Thank you for getting the numbers. 204566 2013-06-13 03:26:38 -0700 2013-06-13 09:56:19 -0700 Patch bug-117586-20130613192456.patch text/plain 3080 gyuyoung.kim U3VidmVyc2lvbiBSZXZpc2lvbjogMTUxNTM0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGNmYTU1Nzk3NDdlNzY3ZWE5MDc3YzA0 Njk1NmRhMTMyY2M1NDk4MmUuLmM1YmJiNjU4NThkM2IyNDUyZjg4MjM1ZjM0ZjhlOWM1OTYwNDEx MTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTMtMDYtMTIgIEd5dXlvdW5nIEtpbSAgPGd5dXlv dW5nLmtpbUBzYW1zdW5nLmNvbT4KKworICAgICAgICBPcHRpbWl6ZSBTdHJpbmc6OmZyb21VVEY4 IGZvciBBU0NJSQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTE3NTg2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgRnJvbSBCbGluayByMTUyMjQzIGJ5IDxhYmFydGhAY2hyb21pdW0ub3JnPgorCisgICAgICAg IEN1cnJlbnQgU3RyaW5nOjpmcm9tVVRGOCgpIGltcGxlbWVudGF0aW9uIGNvbnZlcnRzIDggYml0 IEFTQ0lJIGNoYXJhY3RlciBpbnRvIDE2IGJpdC4KKyAgICAgICAgSW5zdGVhZCBvZiBhbHdheXMg dHJ5aW5nIHRvIGNvbnZlcnQgaW50byBhIDE2IGJpdCBidWZmZXIsIHdlIGNhbiBhZGQgYSBjYWxs IHRvIGNoYXJhY3RlcnNBcmVBbGxBU0NJSS4KKyAgICAgICAgSW4gdGhlIGNvbW1vbiBjYXNlIHdo ZW4gY2hhcmFjdGVycyBhcmUgQVNDSUksIHdlIGRpcmVjdGx5IGNvcHkgaXQgaW50byBhbiA4IGJp dCBzdHJpbmcgYnVmZmVyLgorCisgICAgICAgICogd3RmL3RleHQvV1RGU3RyaW5nLmNwcDoKKyAg ICAgICAgKFdURjo6U3RyaW5nOjpmcm9tVVRGOCk6CisKIDIwMTMtMDYtMTIgIEJyZW50IEZ1bGdo YW0gIDxiZnVsZ2hhbUBhcHBsZS5jb20+CiAKICAgICAgICAgW1dpbmRvd3NdIEFjdGl2YXRlIHN1 cHBvcnRlZCBDKysxMSBGZWF0dXJlcyBmb3IgVlMyMDEwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RG L3d0Zi90ZXh0L1dURlN0cmluZy5jcHAgYi9Tb3VyY2UvV1RGL3d0Zi90ZXh0L1dURlN0cmluZy5j cHAKaW5kZXggYmU1YTBjMjM5ZTIwODhjOWI2N2Q1NjA0YmUzNzg0ZTQ1NjQ4OGY5ZC4uOTJiMjBl ZmRjZjdjNTIyMDQ3MGQ0YTgyNmNkYWRjOTg1NTk2ZjNiMiAxMDA2NDQKLS0tIGEvU291cmNlL1dU Ri93dGYvdGV4dC9XVEZTdHJpbmcuY3BwCisrKyBiL1NvdXJjZS9XVEYvd3RmL3RleHQvV1RGU3Ry aW5nLmNwcApAQCAtOTEyLDI5ICs5MTIsMjAgQEAgU3RyaW5nIFN0cmluZzo6ZnJvbVVURjgoY29u c3QgTENoYXIqIHN0cmluZ1N0YXJ0LCBzaXplX3QgbGVuZ3RoKQogICAgIGlmICghbGVuZ3RoKQog ICAgICAgICByZXR1cm4gZW1wdHlTdHJpbmcoKTsKIAotICAgIC8vIFdlJ2xsIHVzZSBhIFN0cmlu Z0ltcGwgYXMgYSBidWZmZXI7IGlmIHRoZSBzb3VyY2Ugc3RyaW5nIG9ubHkgY29udGFpbnMgYXNj aWkgdGhpcyBzaG91bGQgYmUKLSAgICAvLyB0aGUgcmlnaHQgbGVuZ3RoLCBpZiB0aGVyZSBhcmUg YW55IG11bHRpLWJ5dGUgc2VxdWVuY2VzIHRoaXMgYnVmZmVyIHdpbGwgYmUgdG9vIGxhcmdlLgot ICAgIFVDaGFyKiBidWZmZXI7Ci0gICAgU3RyaW5nIHN0cmluZ0J1ZmZlcihTdHJpbmdJbXBsOjpj cmVhdGVVbmluaXRpYWxpemVkKGxlbmd0aCwgYnVmZmVyKSk7Ci0gICAgVUNoYXIqIGJ1ZmZlckVu ZCA9IGJ1ZmZlciArIGxlbmd0aDsKKyAgICBpZiAoY2hhcmFjdGVyc0FyZUFsbEFTQ0lJKHN0cmlu Z1N0YXJ0LCBsZW5ndGgpKQorICAgICAgICByZXR1cm4gU3RyaW5nSW1wbDo6Y3JlYXRlKHN0cmlu Z1N0YXJ0LCBsZW5ndGgpOworCisgICAgVmVjdG9yPFVDaGFyLCAxMDI0PiBidWZmZXIobGVuZ3Ro KTsKKyAgICBVQ2hhciogYnVmZmVyU3RhcnQgPSBidWZmZXIuZGF0YSgpOwogIAotICAgIC8vIFRy eSBjb252ZXJ0aW5nIGludG8gdGhlIGJ1ZmZlci4KKyAgICBVQ2hhciogYnVmZmVyQ3VycmVudCA9 IGJ1ZmZlclN0YXJ0OwogICAgIGNvbnN0IGNoYXIqIHN0cmluZ0N1cnJlbnQgPSByZWludGVycHJl dF9jYXN0PGNvbnN0IGNoYXIqPihzdHJpbmdTdGFydCk7Ci0gICAgYm9vbCBpc0FsbEFTQ0lJOwot ICAgIGlmIChjb252ZXJ0VVRGOFRvVVRGMTYoJnN0cmluZ0N1cnJlbnQsIHJlaW50ZXJwcmV0X2Nh c3Q8Y29uc3QgY2hhciAqPihzdHJpbmdTdGFydCArIGxlbmd0aCksICZidWZmZXIsIGJ1ZmZlckVu ZCwgJmlzQWxsQVNDSUkpICE9IGNvbnZlcnNpb25PSykKKyAgICBpZiAoY29udmVydFVURjhUb1VU RjE2KCZzdHJpbmdDdXJyZW50LCByZWludGVycHJldF9jYXN0PGNvbnN0IGNoYXIgKj4oc3RyaW5n U3RhcnQgKyBsZW5ndGgpLCAmYnVmZmVyQ3VycmVudCwgYnVmZmVyQ3VycmVudCArIGJ1ZmZlci5z aXplKCkpICE9IGNvbnZlcnNpb25PSykKICAgICAgICAgcmV0dXJuIFN0cmluZygpOwogCi0gICAg aWYgKGlzQWxsQVNDSUkpCi0gICAgICAgIHJldHVybiBTdHJpbmcoc3RyaW5nU3RhcnQsIGxlbmd0 aCk7Ci0KLSAgICAvLyBzdHJpbmdCdWZmZXIgaXMgZnVsbCAodGhlIGlucHV0IG11c3QgaGF2ZSBi ZWVuIGFsbCBhc2NpaSkgc28ganVzdCByZXR1cm4gaXQhCi0gICAgaWYgKGJ1ZmZlciA9PSBidWZm ZXJFbmQpCi0gICAgICAgIHJldHVybiBzdHJpbmdCdWZmZXI7Ci0KLSAgICAvLyBzdHJpbmdCdWZm ZXIgc2VydmVkIGl0cyBwdXJwb3NlIGFzIGEgYnVmZmVyLCBjb3B5IHRoZSBjb250ZW50cyBvdXQg aW50byBhIG5ldyBzdHJpbmcuCi0gICAgdW5zaWduZWQgdXRmMTZMZW5ndGggPSBidWZmZXIgLSBz dHJpbmdCdWZmZXIuY2hhcmFjdGVycygpOworICAgIHVuc2lnbmVkIHV0ZjE2TGVuZ3RoID0gYnVm ZmVyQ3VycmVudCAtIGJ1ZmZlclN0YXJ0OwogICAgIEFTU0VSVCh1dGYxNkxlbmd0aCA8IGxlbmd0 aCk7Ci0gICAgcmV0dXJuIFN0cmluZyhzdHJpbmdCdWZmZXIuY2hhcmFjdGVycygpLCB1dGYxNkxl bmd0aCk7CisgICAgcmV0dXJuIFN0cmluZ0ltcGw6OmNyZWF0ZShidWZmZXJTdGFydCwgdXRmMTZM ZW5ndGgpOwogfQogCiBTdHJpbmcgU3RyaW5nOjpmcm9tVVRGOChjb25zdCBMQ2hhciogc3RyaW5n KQo=