30436 2009-10-16 02:33:32 -0700 fast/css/large-list-of-rules-crash.html concatenates strings in an inefficient way 2009-10-18 00:26:05 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 0 yutak webkit-unassigned commit-queue mrowe oldest_to_newest 155051 0 yutak 2009-10-16 02:33:32 -0700 LayoutTest/fast/css/large-list-of-rules-crash.html contains JavaScript code that repeatedly concatenates strings in an inefficient way. Concatenation of strings should be done using Array.join method. 155052 1 41279 yutak 2009-10-16 02:38:49 -0700 Created attachment 41279 Fix inefficient string concatenation. 155056 2 mrowe 2009-10-16 03:11:30 -0700 Can you please explain why this change is necessary? In my measurements with TOT WebKit there’s approximately 10ms of difference between the two approaches, accounting for less than 2% of the execution time of this test. 155447 3 yutak 2009-10-17 22:45:11 -0700 (In reply to comment #2) > Can you please explain why this change is necessary? In my measurements with > TOT WebKit there’s approximately 10ms of difference between the two approaches, > accounting for less than 2% of the execution time of this test. Yes, as for JavaScriptCore, only small improvement can be observed. However, currently debug version of V8 is hitting this problem and consuming test cycles. I admit the argument above is weak..., but I believe there is no need to leave an inefficient test as is. 155448 4 mrowe 2009-10-17 22:46:34 -0700 It’d be stronger if you took a few seconds to provide data to support your claim that it is inefficient. 155451 5 yutak 2009-10-17 22:57:45 -0700 (In reply to comment #4) > It’d be stronger if you took a few seconds to provide data to support your > claim that it is inefficient. Original code look like this: var s = ""; for (i = 0 ; i < 200000 ; i++) { s += "a {}\n"; } This code produces potentially (especially for debug binary) allocation of 200000 string objects with length of 5, 10, 15, ..., 5*200000. The fixed version does not require the intermediate string objects. I believe using Array.join is the common pattern to avoid unneeded allocation. 155452 6 mrowe 2009-10-17 23:02:46 -0700 You’ve just restated what the patch does. I’m interested in specifics about how much of an improvement this change is. 155453 7 yutak 2009-10-17 23:13:29 -0700 (In reply to comment #6) > You’ve just restated what the patch does. I’m interested in specifics about > how much of an improvement this change is. From 30sec to 3sec, as for V8 debug. 155455 8 mrowe 2009-10-17 23:15:14 -0700 Thanks! 155457 9 41279 commit-queue 2009-10-17 23:27:24 -0700 Comment on attachment 41279 Fix inefficient string concatenation. Clearing flags on attachment: 41279 Committed r49748: <http://trac.webkit.org/changeset/49748> 155458 10 commit-queue 2009-10-17 23:27:27 -0700 All reviewed patches have been landed. Closing bug. 155460 11 yutak 2009-10-18 00:26:05 -0700 Thanks! 41279 2009-10-16 02:38:49 -0700 2009-10-17 23:27:24 -0700 Fix inefficient string concatenation. bug-30436-20091016183848.patch text/plain 2123 yutak ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBiMmJhNTY2Li5iYTNmZTc1IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMDktMTAt MTYgIFl1dGEgS2l0YW11cmEgIDx5dXRha0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRml4IGluZWZmaWNpZW50IHN0cmluZyBj b25jYXRlbmF0aW9uLgorICAgICAgICAKKyAgICAgICAgTGF5b3V0VGVzdHMvZmFzdC9jc3MvbGFy Z2UtbGlzdC1vZi1ydWxlcy1jcmFzaC5odG1sIGNvbnRhaW5zIGNvZGUgdGhhdAorICAgICAgICBj b25jYXRlbmF0ZSBzdHJpbmdzIGluIGFuIGluZWZmaWNpZW50IHdheS4gQ29uY2F0ZW5hdGlvbiBv ZiBzdHJpbmdzIHNob3VsZCBiZQorICAgICAgICBkb25lIHdpdGggQXJyYXkuam9pbiBtZXRob2Qu IFRoaXMgcGF0Y2ggZml4ZXMgdGhpcyBpc3N1ZSwgYW5kIGFsc28gZml4ZXMgdHlwb3MKKyAgICAg ICAgaW4gdGhlIHRlc3QuCisKKyAgICAgICAgZmFzdC9jc3MvbGFyZ2UtbGlzdC1vZi1ydWxlcy1j cmFzaC5odG1sIGNvbmNhdGVuYXRlcyBzdHJpbmdzIGluIGFuIGluZWZmaWNpZW50IHdheQorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzA0MzYKKworICAg ICAgICAqIGZhc3QvY3NzL2xhcmdlLWxpc3Qtb2YtcnVsZXMtY3Jhc2guaHRtbDoKKwogMjAwOS0x MC0xNSAgRGFuaWVsIEJhdGVzICA8ZGJhdGVzQHdlYmtpdC5vcmc+CiAKICAgICAgICAgTm8gcmV2 aWV3LCByb2xsaW5nIG91dCByNDk2NDQuCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9mYXN0L2Nz cy9sYXJnZS1saXN0LW9mLXJ1bGVzLWNyYXNoLmh0bWwgYi9MYXlvdXRUZXN0cy9mYXN0L2Nzcy9s YXJnZS1saXN0LW9mLXJ1bGVzLWNyYXNoLmh0bWwKaW5kZXggNTFiYTRiYy4uZDQ4ZGZmNCAxMDA2 NDQKLS0tIGEvTGF5b3V0VGVzdHMvZmFzdC9jc3MvbGFyZ2UtbGlzdC1vZi1ydWxlcy1jcmFzaC5o dG1sCisrKyBiL0xheW91dFRlc3RzL2Zhc3QvY3NzL2xhcmdlLWxpc3Qtb2YtcnVsZXMtY3Jhc2gu aHRtbApAQCAtMywxMCArMywxMSBAQAogICAiaHR0cDovL3d3dy53My5vcmcvVFIvaHRtbDQvc3Ry aWN0LmR0ZCI+CiAKIDxzY3JpcHQgdHlwZT0idGV4dC9qYXZhc2NyaXB0Ij4KLXZhciBzID0gIiI7 Ci1mb3IgKGkgPSAwIDsgaSA8IDIwMDAwMCA7IGkrKykgewotICAgcyArPSAiYSB7fVxuIjsKLX0K K3ZhciBhcnJheSA9IEFycmF5KDIwMDAwMCk7Citmb3IgKHZhciBpID0gMDsgaSA8IDIwMDAwMDsg KytpKQorICAgIGFycmF5W2ldID0gImEge31cbiI7Cit2YXIgcyA9IGFycmF5LmpvaW4oIiIpOwor CiB2YXIgc3R5bGUgPSBkb2N1bWVudC5jcmVhdGVFbGVtZW50KCJzdHlsZSIpOwogc3R5bGUuYXBw ZW5kQ2hpbGQoZG9jdW1lbnQuY3JlYXRlVGV4dE5vZGUocykpOwogZG9jdW1lbnQuZ2V0RWxlbWVu dHNCeVRhZ05hbWUoImhlYWQiKVswXS5hcHBlbmRDaGlsZChzdHlsZSk7CkBAIC0xNyw4ICsxOCw4 IEBAIGlmICh3aW5kb3cubGF5b3V0VGVzdENvbnRyb2xsZXIpCiAKIDwhLS0gbG9hZGluZyBhIGxp bmtlZCBzdHlsZSBzaGVldCBjYXVzZXMgdGhlIGxpc3Qgb2YgQ1NTIFJ1bGVzIHRvIGJlIHJlYnVp bHQsCiAgICAgIHdoaWNoIGNhdXNlZCBhIGNyYXNoIChzdGFjayBvdmVyZmxvdykgLS0+Ci08bGlu ayByZWw9InN0eWxlc2hlZXQiIGhyZWY9InJlc3VsdHMvbGFyZ2UtbGlzdC1vZi1ydWxlcy1jcmFz aC5jc3MiPiAKLTxsaW5rIHJlbD0ic3R5bGVzaGVldCIgaHJlZj0icmVzdWx0cy9sYXJnZS1saXN0 LW9mLXJ1bGVzLWNyYXNoLmNzcyI+Cis8bGluayByZWw9InN0eWxlc2hlZXQiIGhyZWY9InJlc291 cmNlcy9sYXJnZS1saXN0LW9mLXJ1bGVzLWNyYXNoLmNzcyI+Cis8bGluayByZWw9InN0eWxlc2hl ZXQiIGhyZWY9InJlc291cmNlcy9sYXJnZS1saXN0LW9mLXJ1bGVzLWNyYXNoLmNzcyI+CiAKIDxw PlRlc3QgY2FzZSBmb3IKIDxhIGhyZWY9Imh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0yNzc0OCI=