|Summary:||[JSC] WebKit allocates gigabytes of memory when doing repeated string concatenation|
|Product:||WebKit||Reporter:||Matthijs van der Vleuten <zr40.nl>|
|Severity:||Normal||CC:||barraclough, dbates, dominicc, eric, ggaren, mhahnenberg, mrowe, oliver, sam, tonikitoo, webkit.review.bot|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.6|
Description Matthijs van der Vleuten 2011-07-04 13:50:24 PDT
On http://files.myopera.com/emoller/blog/webkit.html, WebKit allocates several gigabytes of memory, while other browsers do not. This happens in the current WebKit nightly (r90370) as well as Safari 5.0.5 (6533.21.1), but not in Firefox 5.0 and Chrome 12.0.742.112.
Comment 1 Dominic Cooney 2011-07-04 19:25:52 PDT
I debugged this a little, and I observed the JSC GC is allocating JSStrings with two fibers during string concatenation. These JSC heap objects are small, so JSC GC is not triggered. However there is a lot of string copying in the C++ heap, which is where I think the heap growth comes from. OP: If you need a workaround, which is probably fragile, you can trick JSC into creating strings with three pieces, which have a different code path, by doing something like changing: for (var i = offs; i < offs + 10; ++i) dest += dest[i]; to this: for (var i = offs; i < offs + 10; i += 2) dest = dest + dest[i] + dest[i + 1];
Comment 2 Gavin Barraclough 2011-07-05 14:18:52 PDT
(In reply to comment #1) > I debugged this a little, and I observed the JSC GC is allocating JSStrings with two fibers during string concatenation. These JSC heap objects are small, so JSC GC is not triggered. There is a mechanism that is meant to help deal with this, that looks a little broken. One of the JSString constructors is calling reportExtraMemoryCost - this should probably be called on all UStrings & StringImpls being passed through the JSString constructors. (That said, Geoff has been doing a lot of work in the heap lately, might be worth verifying this mechanism is still live & hooked up on the other side).
Comment 3 Geoffrey Garen 2011-07-18 16:51:51 PDT
Yes, reportExtraMemoryCost is the way to fix this.
Comment 6 Darin Adler 2011-07-19 10:31:47 PDT
Comment on attachment 101339 [details] Patch Is there a good way for us to test this?
Comment 7 Geoffrey Garen 2011-07-19 11:41:10 PDT
I'd recommend testing this patch with SunSpider and posting the results here. String concatenation can be a hot operation.
Comment 8 Geoffrey Garen 2011-07-19 11:41:50 PDT
(But I can't think of a good way to unit test this, since the only symptom is memory footprint, and our unit testing framework doesn't have a mechanism for measuring that.)
Comment 9 Mark Hahnenberg 2011-07-19 12:03:11 PDT
Created attachment 101356 [details] SunSpider results
Comment 10 Oliver Hunt 2011-07-19 12:03:53 PDT
We actually need results of sunspider-compare-results
Comment 11 Mark Hahnenberg 2011-07-19 12:14:07 PDT
Created attachment 101359 [details] SunSpider results comparison
Comment 12 WebKit Review Bot 2011-07-19 12:49:20 PDT
Comment on attachment 101339 [details] Patch Clearing flags on attachment: 101339 Committed r91288: <http://trac.webkit.org/changeset/91288>
Comment 13 WebKit Review Bot 2011-07-19 12:49:26 PDT
All reviewed patches have been landed. Closing bug.