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.
Created attachment 41279 [details] Fix inefficient string concatenation.
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.
(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.
It’d be stronger if you took a few seconds to provide data to support your claim that it is inefficient.
(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.
You’ve just restated what the patch does. I’m interested in specifics about how much of an improvement this change is.
(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.
Thanks!
Comment on attachment 41279 [details] Fix inefficient string concatenation. Clearing flags on attachment: 41279 Committed r49748: <http://trac.webkit.org/changeset/49748>
All reviewed patches have been landed. Closing bug.