Bug 63918 - [JSC] WebKit allocates gigabytes of memory when doing repeated string concatenation
Summary: [JSC] WebKit allocates gigabytes of memory when doing repeated string concate...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Mark Hahnenberg
URL: http://files.myopera.com/emoller/blog...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-07-04 13:50 PDT by Matthijs van der Vleuten
Modified: 2011-07-19 12:49 PDT (History)
11 users (show)

See Also:


Attachments
Patch (1.95 KB, patch)
2011-07-19 10:09 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
SunSpider results (6.70 KB, application/x-javascript)
2011-07-19 12:03 PDT, Mark Hahnenberg
no flags Details
SunSpider results comparison (3.26 KB, text/plain)
2011-07-19 12:14 PDT, Mark Hahnenberg
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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 4 Geoffrey Garen 2011-07-18 16:52:38 PDT
<rdar://problem/9796985>
Comment 5 Mark Hahnenberg 2011-07-19 10:09:49 PDT
Created attachment 101339 [details]
Patch
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.