Last Comment Bug 660734 - Strings are not counted by GC heuristics (high memory use with regexps on large strings)
: Strings are not counted by GC heuristics (high memory use with regexps on lar...
Status: RESOLVED FIXED
[MemShrink:P2][fixed-in-tracemonkey]
: footprint, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Windows 7
: -- normal with 1 vote (vote)
: ---
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: mlk-fx4
  Show dependency treegraph
 
Reported: 2011-05-30 20:39 PDT by Jaroslav Benc
Modified: 2011-06-26 05:04 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Performing String.replace on large string using RE (446.96 KB, text/html)
2011-05-30 20:40 PDT, Jaroslav Benc
no flags Details
Performing String.replace on large string using RE - fixed version (446.93 KB, text/html)
2011-05-30 20:45 PDT, Jaroslav Benc
no flags Details
s/ContextAllocPolicy/TempAllocPolicy/ (13.21 KB, patch)
2011-06-16 16:19 PDT, Luke Wagner [:luke]
igor: review+
Details | Diff | Review
add old ContextAllocPolicy, use in StringBuffer (5.39 KB, patch)
2011-06-16 16:21 PDT, Luke Wagner [:luke]
igor: review+
Details | Diff | Review

Description Jaroslav Benc 2011-05-30 20:39:21 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 4.0.1

Try attached example and see memory usage: 

FF4 uses 109MB - 560MB 
FF3 uses 73MB - 196MB
Chrome 11 uses 55MB - 60MB

The other (and major) problem is that it leaks when embedded as in iframe but I haven't been able to reproduce a simple test case outside of our product, will try and report it as another bug. 

Interesting that Chrome can do the same operation with 5MB, maybe would be worth to overview this as it might improve performance in general. 

Reproducible: Always
Comment 1 Jaroslav Benc 2011-05-30 20:40:22 PDT
Created attachment 536215 [details]
Performing String.replace on large string using RE
Comment 2 Jaroslav Benc 2011-05-30 20:45:16 PDT
Created attachment 536218 [details]
Performing String.replace on large string using RE - fixed version
Comment 3 Tyler Downer [:Tyler] 2011-05-30 20:59:49 PDT
I am seeing pretty high memory usage, it seems to go from roughly 180mb to 630 before garbage collection kicks it and it goes back down. But the number it goes to is always higher than the number is started at the last cycle (eg, 180-630 GC 140-630GC)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Comment 4 David Mandelin [:dmandelin] 2011-06-01 10:32:43 PDT
Luke, could this have anything to do with ropes?
Comment 5 Luke Wagner [:luke] 2011-06-01 17:04:54 PDT
The rope optimization only applies when we can do "flat" matches (e.g., str.replace('a', 'b')), not when we are given a regexp as in the example.

Basically, the testcase just takes a huge string and does 6 regexp replace's on it (each producing a copy), and does that from setTimeout every 10ms.  Each string becomes garbage, so it should all get collected on each GC (which about:memory and ps seems to show is the case).  I watched total usage for several minutes and saw no increase in the peaks or valleys of the sawtooth curve.  I believe the reason we get so high compared to Chrome is b/c we wait so long for a GC whereas Chrome (with its generational gc) is going to collect much more often and thus collect all the garbage being created.  Our own GC work should help this.
Comment 6 Jaroslav Benc 2011-06-01 18:14:46 PDT
Luke, any thoughts why FF3 memory behaviour is better? Is there anything we could do, would WeakMap help in this case?
Comment 7 Boris Zbarsky [:bz] 2011-06-01 19:45:47 PDT
3.6 is a lot more aggressive about running gc than 4.0 is.  That means more time is taken running GC, though, which hurts throughput.
Comment 8 Gregor Wagner [:gwagner] 2011-06-10 09:49:26 PDT
I also noticed this on my very low end netbook where it causes serious memory pressure problems as I told Luke yesterday.
I looked at a shark profile and noticed that most memory is allocated via our vectors and vector reallocs don't account for our GC triggers. So we could probably run into OOM without running the GC.
Luke does this sound reasonable?
Comment 9 Luke Wagner [:luke] 2011-06-10 11:39:21 PDT
Could you point me to the particular Vectors that are doing this?  So, recently, igor changed ContextAllocPolicy to not do gc-malloc-bytes accounting since, in theory, if you can use ContextAllocPolicy, you must be short-lived (else you can get a dangling JSContext, as you know :), and hence you are "temporary" and thats not what gcMallocBytes is measuring.  However, here you have a case where a Vector is long-lived enough to cause real GC pressure.  So it seems we just really need a new AllocPolicy.  There have been a couple of recent examples that our AllocPolicy situation is weird and needs to be refactored; I'll file a bug once you point me to the offending Vector so I can check out the code.
Comment 10 Luke Wagner [:luke] 2011-06-10 17:31:52 PDT
Aha, when ContextAllocPolicy stopped accounting gcMallocBytes, StringBuffer's internal vector stopped accounting for its allcoations.  Since the StringBuffer's buffer is ultimately extracted to be the long-lived buffer of a string, this is totally a hole in the GC heuristic!  No wonder the GC sawtooth gets to high.  The fix is simply to give StringBuffer's Vector a policy that does do accounting.
Comment 11 Nicholas Nethercote [:njn] 2011-06-10 19:22:48 PDT
Luke, I'll assign this to you, hope this is ok.
Comment 12 Luke Wagner [:luke] 2011-06-10 19:48:07 PDT
Sure, I can take it.

What I'd like to do is throw out all these crazy "Context" "System" "Runtime" names and make a new set whose names correspond the pairs:
  { Reporting, NonReporting } x { Accounting, NonAccounting }
(In the process auditing our existing policy use.)
Comment 13 Luke Wagner [:luke] 2011-06-16 16:19:12 PDT
Created attachment 539926 [details] [diff] [review]
s/ContextAllocPolicy/TempAllocPolicy/

I need to put back the old ContextAllocPolicy in the next patch (the one that calls cx->malloc_ instead of js_malloc+report), so rename the thing currently (mis)named as ContextAllocPolicy.  I'd rather not debate the merits of the name "TempAllocPolicy" since all these *AllocPolicy names need to be changed after we settle on the underlying allocator names in bug 647103.
Comment 14 Luke Wagner [:luke] 2011-06-16 16:21:22 PDT
Created attachment 539928 [details] [diff] [review]
add old ContextAllocPolicy, use in StringBuffer

Incidentally, even before Igor's changes, StringBuffer had accounting problems since it called realloc() without passing oldBytes (which means realloc() wouldn't do accounting).  Fortunately, we just added an oldBytes param to AllocPolicy (like, yesterday).
Comment 15 Luke Wagner [:luke] 2011-06-16 16:22:54 PDT
With these two patches (applied to TM tip) I see memory usage drop from FF4 levels to FF3 levels (as reported by comment 0).
Comment 16 Nicholas Nethercote [:njn] 2011-06-16 16:31:44 PDT
(In reply to comment #15)
> With these two patches (applied to TM tip) I see memory usage drop from FF4
> levels to FF3 levels (as reported by comment 0).

Great!  Thanks, Luke.
Comment 17 Igor Bukanov 2011-06-17 04:44:17 PDT
Comment on attachment 539928 [details] [diff] [review]
add old ContextAllocPolicy, use in StringBuffer

Review of attachment 539928 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 18 Luke Wagner [:luke] 2011-06-17 08:46:17 PDT
Igor: there is one other patch needing your r+ (purely syntactic).
Comment 19 Igor Bukanov 2011-06-17 08:57:09 PDT
Comment on attachment 539926 [details] [diff] [review]
s/ContextAllocPolicy/TempAllocPolicy/

Review of attachment 539926 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 21 Gregor Wagner [:gwagner] 2011-06-17 10:35:37 PDT
My netbook says DANKE! :)
Comment 22 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:23:50 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/47b578958aa4
http://hg.mozilla.org/mozilla-central/rev/31e3b521775b

Note You need to log in before you can comment on or make changes to this bug.