Bug 30436 – fast/css/large-list-of-rules-crash.html concatenates strings in an inefficient way

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Yuta Kitamura]

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.

[Yuta Kitamura]

Created attachment 41279 [details]
Fix inefficient string concatenation.

[Mark Rowe (bdash)]

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.

[Yuta Kitamura]

(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.

[Mark Rowe (bdash)]

It’d be stronger if you took a few seconds to provide data to support your claim that it is inefficient.

[Yuta Kitamura]

(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.

[Mark Rowe (bdash)]

You’ve just restated what the patch does.  I’m interested in specifics about how much of an improvement this change is.

[Yuta Kitamura]

(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.

[Mark Rowe (bdash)]

Thanks!

[WebKit Commit Bot]

Comment on attachment 41279 [details]
Fix inefficient string concatenation.

Clearing flags on attachment: 41279

Committed r49748: <http://trac.webkit.org/changeset/49748>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

Thanks!