Bug 63918

Summary: [JSC] WebKit allocates gigabytes of memory when doing repeated string concatenation
Product: WebKit Reporter: Matthijs van der Vleuten <zr40.nl>
Component: WebCore JavaScriptAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, dbates, dominicc, eric, ggaren, mhahnenberg, mrowe, oliver, sam, tonikitoo, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
URL: http://files.myopera.com/emoller/blog/webkit.html
Attachments:
Description Flags
Patch
none
SunSpider results
none
SunSpider results comparison none

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.