95810 2012-09-04 18:29:29 -0700 16 bit JSRopeString up converts an 8 bit fibers to 16 bits during resolution 2012-09-06 18:28:40 -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 711640 0 msaboff 2012-09-04 18:29:29 -0700 A JSRopeString can contain both 8 and 16 bit fibers. If there is one 16 bit fiber, the resulting resolved JSRopeString will also be 16 bits. The problem is that during resolution an 8 bit fiber is first up converted itself to 16 bits and then treated like a 16 bit fiber. The up conversion will use extra memory and hurt performance for the common case that the string is only the fiber of one JSRopeString. Instead the up conversion should be done directly as part of the resolution. 712532 1 162332 msaboff 2012-09-05 14:17:39 -0700 Created attachment 162332 Patch 712585 2 benjamin 2012-09-05 15:24:26 -0700 This looks awesome but it could have an impact on performance (if the same strings are used repeatedly). Resolving ropes is va performance sensitive area. Have you verified the benchmarks? 712605 3 msaboff 2012-09-05 15:43:12 -0700 (In reply to comment #2) > This looks awesome but it could have an impact on performance (if the same strings are used repeatedly). > > Resolving ropes is va performance sensitive area. Have you verified the benchmarks? I haven't verified performance. I'll verify with SunSpider, V8 and Kraken and Dromaeo string tests. Any others you want? 712624 4 benjamin 2012-09-05 16:02:29 -0700 > I haven't verified performance. I'll verify with SunSpider, V8 and Kraken and Dromaeo string tests. Any others you want? I think SunSpider should do the trick. It has good coverage for strings. I have heard Filip has a good tool to test the benchmarks, better ping him. 712885 5 162332 sam 2012-09-05 22:26:50 -0700 Comment on attachment 162332 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162332&action=review > Source/JavaScriptCore/runtime/JSString.cpp:208 > + StringImpl::copyChars(position, string->characters(), length); Should this call characters16() to avoid the extra branch? 712892 6 benjamin 2012-09-05 22:36:54 -0700 > > Source/JavaScriptCore/runtime/JSString.cpp:208 > > + StringImpl::copyChars(position, string->characters(), length); > > Should this call characters16() to avoid the extra branch? Yep, you are right. 713398 7 msaboff 2012-09-06 10:11:10 -0700 (In reply to comment #4) > > I haven't verified performance. I'll verify with SunSpider, V8 and Kraken and Dromaeo string tests. Any others you want? > > I think SunSpider should do the trick. It has good coverage for strings. > > I have heard Filip has a good tool to test the benchmarks, better ping him. Here are the results for SunSpider and V8: VMs tested: "BaseLine" at /Volumes/Data/src/webkit.baseline/WebKitBuild/Release/DumpRenderTree (r127525) "95810" at /Volumes/Data/src/webkit/WebKitBuild/Release/DumpRenderTree (r127525) Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. BaseLine 95810 SunSpider: 3d-cube 8.5670+-0.5119 ? 8.9346+-0.5255 ? might be 1.0429x slower 3d-morph 8.2499+-0.1111 8.2225+-0.1247 3d-raytrace 12.2917+-0.3964 ? 12.6080+-0.3119 ? might be 1.0257x slower access-binary-trees 2.2971+-0.4272 ? 2.3416+-0.4374 ? might be 1.0194x slower access-fannkuch 6.8547+-0.0200 ? 6.8976+-0.0840 ? access-nbody 4.2014+-0.0695 ? 4.2518+-0.0845 ? might be 1.0120x slower access-nsieve 3.4389+-0.0473 3.3642+-0.0569 might be 1.0222x faster bitops-3bit-bits-in-byte 1.3670+-0.0139 1.3652+-0.0141 bitops-bits-in-byte 5.7495+-0.0125 5.7362+-0.0115 bitops-bitwise-and 2.1783+-0.0872 ? 2.2376+-0.0641 ? might be 1.0272x slower bitops-nsieve-bits 3.5173+-0.0171 ? 3.5415+-0.0363 ? controlflow-recursive 2.5044+-0.0069 2.5032+-0.0191 crypto-aes 9.7332+-0.5201 ? 9.7493+-0.5317 ? crypto-md5 3.9798+-0.1409 3.9523+-0.1279 crypto-sha1 3.1893+-0.0437 ? 3.1950+-0.0661 ? date-format-tofte 14.3015+-0.6807 ? 14.5108+-0.9050 ? might be 1.0146x slower date-format-xparb 11.9714+-0.6316 11.4960+-0.1827 might be 1.0414x faster math-cordic 4.2393+-0.0759 ? 4.2980+-0.0703 ? might be 1.0138x slower math-partial-sums 10.1456+-0.0613 10.1065+-0.0189 math-spectral-norm 3.0143+-0.0189 3.0132+-0.0168 regexp-dna 10.4226+-0.3497 10.4187+-0.3661 string-base64 5.9329+-0.4557 5.7017+-0.4059 might be 1.0405x faster string-fasta 8.1435+-0.2758 ? 8.1897+-0.2468 ? string-tagcloud 14.0988+-0.3792 13.7988+-0.2379 might be 1.0217x faster string-unpack-code 23.4974+-0.5536 23.4540+-0.5765 string-validate-input 8.7743+-0.5667 8.5747+-0.5756 might be 1.0233x faster <arithmetic> * 7.4100+-0.1313 7.4024+-0.1264 might be 1.0010x faster <geometric> 5.9050+-0.1044 ? 5.9076+-0.1061 ? might be 1.0004x slower <harmonic> 4.6297+-0.0826 ? 4.6426+-0.0884 ? might be 1.0028x slower BaseLine 95810 V8Real: encrypt 0.40161+-0.00098 ? 0.40167+-0.00059 ? decrypt 6.94212+-0.01194 6.93901+-0.01066 deltablue x2 0.57076+-0.00408 ? 0.57362+-0.00593 ? earley 1.23547+-0.01809 1.22780+-0.00944 boyer 13.76285+-0.06688 13.71783+-0.06507 raytrace x2 4.53568+-0.03303 ? 4.55272+-0.04241 ? regexp x2 26.63586+-0.08221 ? 26.68346+-0.11123 ? richards x2 0.27006+-0.00167 0.26990+-0.00202 splay x2 0.74024+-0.00986 0.73810+-0.00648 navier-stokes x2 12.73918+-0.01668 ? 12.74327+-0.02574 ? <arithmetic> 7.08285+-0.01590 ? 7.08803+-0.01532 ? might be 1.0007x slower <geometric> * 2.42818+-0.00593 ? 2.42897+-0.00709 ? might be 1.0003x slower <harmonic> 0.89872+-0.00298 ? 0.89887+-0.00443 ? might be 1.0002x slower BaseLine 95810 All benchmarks: <arithmetic> 7.2854+-0.0847 7.2826+-0.0803 might be 1.0004x faster <geometric> 4.2089+-0.0487 ? 4.2105+-0.0484 ? might be 1.0004x slower <harmonic> 1.7932+-0.0108 ? 1.7946+-0.0122 ? might be 1.0008x slower BaseLine 95810 Geomean of preferred means: <scaled-result> 13.4128+-0.1306 13.4080+-0.1223 might be 1.0004x faster 713401 8 msaboff 2012-09-06 10:11:51 -0700 (In reply to comment #6) > > > Source/JavaScriptCore/runtime/JSString.cpp:208 > > > + StringImpl::copyChars(position, string->characters(), length); > > > > Should this call characters16() to avoid the extra branch? > > Yep, you are right. Made this change locally. Want a new patch? 713616 9 162332 benjamin 2012-09-06 13:16:28 -0700 Comment on attachment 162332 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162332&action=review The patch is great. And the bench are good so let's land this :) Please fix the characters() call and check 2 tiny things: > Source/WTF/wtf/text/StringImpl.h:541 > + static void copyChars(UChar* destination, const LChar* source, unsigned numCharacters) > + { > + if (numCharacters == 1) { > + *destination = *source; > + return; > + } > + > + for (unsigned i = 0; i < numCharacters; ++i) > + destination[i] = source[i]; > + } > + I have the feeling the other copyChars is always inlined. Could you check and add ALWAYS_INLINE if necessary? I am not sure the case numCharacters == 1 helps much here because you don't have as much overhead with short-ish strings. 713965 10 msaboff 2012-09-06 18:28:40 -0700 Committed r127809: <http://trac.webkit.org/changeset/127809> 162332 2012-09-05 14:17:39 -0700 2012-09-06 13:16:28 -0700 Patch 95810.patch text/plain 3243 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTI3NjQ1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDEyLTA5LTA1ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIDE2IGJpdCBKU1JvcGVTdHJpbmcgdXAgY29udmVydHMgYW4gOCBiaXQgZmliZXJzIHRvIDE2 IGJpdHMgZHVyaW5nIHJlc29sdXRpb24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTk1ODEwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgQWRkZWQgOCBiaXQgcGF0aCB0aGF0IGNvcGllcyB0aGUgY29udGVudHMg b2YgYW4gOCBiaXQgZmliZXIgdG8gdGhlIDE2IGJpdCBidWZmZXIKKyAgICAgICAgd2hlbiByZXNv bHZpbmcgYSAxNiBiaXQgcm9wZS4KKworICAgICAgICAqIHJ1bnRpbWUvSlNTdHJpbmcuY3BwOgor ICAgICAgICAoSlNDOjpKU1JvcGVTdHJpbmc6OnJlc29sdmVSb3BlU2xvd0Nhc2UpOgorCiAyMDEy LTA5LTA1ICBHZW9mZnJleSBHYXJlbiAgPGdnYXJlbkBhcHBsZS5jb20+CiAKICAgICAgICAgUmVm YWN0b3JlZCBjYWxsZWUgYWNjZXNzIGluIHRoZSBERkcgdG8gc3VwcG9ydCBpdCBpbiB0aGUgZ2Vu ZXJhbCBjYXNlCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmluZy5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNTdHJpbmcu Y3BwCShyZXZpc2lvbiAxMjc2MDQpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9K U1N0cmluZy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIwMiw3ICsyMDIsMTAgQEAgdm9pZCBKU1Jv cGVTdHJpbmc6OnJlc29sdmVSb3BlU2xvd0Nhc2UoVQogICAgICAgICBTdHJpbmdJbXBsKiBzdHJp bmcgPSBzdGF0aWNfY2FzdDxTdHJpbmdJbXBsKj4oY3VycmVudEZpYmVyLT5tX3ZhbHVlLmltcGwo KSk7CiAgICAgICAgIHVuc2lnbmVkIGxlbmd0aCA9IHN0cmluZy0+bGVuZ3RoKCk7CiAgICAgICAg IHBvc2l0aW9uIC09IGxlbmd0aDsKLSAgICAgICAgU3RyaW5nSW1wbDo6Y29weUNoYXJzKHBvc2l0 aW9uLCBzdHJpbmctPmNoYXJhY3RlcnMoKSwgbGVuZ3RoKTsKKyAgICAgICAgaWYgKHN0cmluZy0+ aXM4Qml0KCkpCisgICAgICAgICAgICBTdHJpbmdJbXBsOjpjb3B5Q2hhcnMocG9zaXRpb24sIHN0 cmluZy0+Y2hhcmFjdGVyczgoKSwgbGVuZ3RoKTsKKyAgICAgICAgZWxzZQorICAgICAgICAgICAg U3RyaW5nSW1wbDo6Y29weUNoYXJzKHBvc2l0aW9uLCBzdHJpbmctPmNoYXJhY3RlcnMoKSwgbGVu Z3RoKTsKICAgICB9CiAKICAgICBBU1NFUlQoYnVmZmVyID09IHBvc2l0aW9uKTsKSW5kZXg6IFNv dXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hhbmdlTG9nCShy ZXZpc2lvbiAxMjc2NDUpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpA QCAtMSwzICsxLDE3IEBACisyMDEyLTA5LTA1ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBw bGUuY29tPgorCisgICAgICAgIDE2IGJpdCBKU1JvcGVTdHJpbmcgdXAgY29udmVydHMgYW4gOCBi aXQgZmliZXJzIHRvIDE2IGJpdHMgZHVyaW5nIHJlc29sdXRpb24KKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk1ODEwCisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTmV3IGNvcHkgcm91dGluZSB0aGF0IHRha2Vz IGFuIDggYml0IHNvdXJjZSBhbmQgYSAxNiBiaXQgZGVzdGluYXRpb24uICBVc2VkIHdoZW4gY29w eWluZworICAgICAgICB0aGUgY29udGVudHMgb2YgYW4gOCBiaXQgZmliZXIgdG8gdGhlIDE2IGJp dCBidWZmZXIgd2hlbiByZXNvbHZpbmcgYSAxNiBiaXQgcm9wZS4KKworICAgICAgICAqIHd0Zi90 ZXh0L1N0cmluZ0ltcGwuaDoKKyAgICAgICAgKFdURjo6U3RyaW5nSW1wbDo6Y29weUNoYXJzKToK KyAgICAgICAgKFN0cmluZ0ltcGwpOgorCiAyMDEyLTA5LTA1ICBHYWJvciBSYXBjc2FueWkgIDxy Z2Fib3JAd2Via2l0Lm9yZz4KIAogICAgICAgICBERkcgSklUIGRvZXNuJ3Qgd29yayBwcm9wZXJs eSBvbiBBUk0gaGFyZGZwCkluZGV4OiBTb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0ltcGwuaAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBTb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0ltcGwuaAkocmV2aXNpb24g MTI3NjA0KQorKysgU291cmNlL1dURi93dGYvdGV4dC9TdHJpbmdJbXBsLmgJKHdvcmtpbmcgY29w eSkKQEAgLTUyOCw2ICs1MjgsMTcgQEAgcHVibGljOgogICAgICAgICAgICAgbWVtY3B5KGRlc3Rp bmF0aW9uLCBzb3VyY2UsIG51bUNoYXJhY3RlcnMgKiBzaXplb2YoVCkpOwogICAgIH0KIAorICAg IHN0YXRpYyB2b2lkIGNvcHlDaGFycyhVQ2hhciogZGVzdGluYXRpb24sIGNvbnN0IExDaGFyKiBz b3VyY2UsIHVuc2lnbmVkIG51bUNoYXJhY3RlcnMpCisgICAgeworICAgICAgICBpZiAobnVtQ2hh cmFjdGVycyA9PSAxKSB7CisgICAgICAgICAgICAqZGVzdGluYXRpb24gPSAqc291cmNlOworICAg ICAgICAgICAgcmV0dXJuOworICAgICAgICB9CisgICAgICAgIAorICAgICAgICBmb3IgKHVuc2ln bmVkIGkgPSAwOyBpIDwgbnVtQ2hhcmFjdGVyczsgKytpKQorICAgICAgICAgICAgZGVzdGluYXRp b25baV0gPSBzb3VyY2VbaV07CisgICAgfQorCiAgICAgLy8gU29tZSBzdHJpbmcgZmVhdHVyZXMs IGxpa2UgcmVmY291bnRpbmcgYW5kIHRoZSBhdG9taWNpdHkgZmxhZywgYXJlIG5vdAogICAgIC8v IHRocmVhZC1zYWZlLiBXZSBhY2hpZXZlIHRocmVhZCBzYWZldHkgYnkgaXNvbGF0aW9uLCBnaXZp bmcgZWFjaCB0aHJlYWQKICAgICAvLyBpdHMgb3duIGNvcHkgb2YgdGhlIHN0cmluZy4K