660734 - Strings are not counted by GC heuristics (high memory use with regexps on large strings)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jaroslav Benc]

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]

Attachment: Performing String.replace on large string using RE

Comment 2

[Jaroslav Benc]

Attachment: Performing String.replace on large string using RE - fixed version

Comment 3

[Tyler Downer [He/Him]]

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]]

Luke, could this have anything to do with ropes?

Comment 5

[Luke Wagner [:luke]]

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]

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 [:bzbarsky]]

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]]

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]]

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]]

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 [inactive]]

Luke, I'll assign this to you, hope this is ok.

Comment 12

[Luke Wagner [:luke]]

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]]

Attachment: 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]]

Attachment: 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]]

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 [inactive]]

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

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]]

Igor: there is one other patch needing your r+ (purely syntactic).

Comment 19

[Igor Bukanov]

Comment on attachment 539926 [details] [diff] [review]
s/ContextAllocPolicy/TempAllocPolicy/

Review of attachment 539926 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 20

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/47b578958aa4
http://hg.mozilla.org/tracemonkey/rev/31e3b521775b

Comment 21

[Gregor Wagner [:gwagner]]

My netbook says DANKE! :)

Comment 22

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/47b578958aa4
http://hg.mozilla.org/mozilla-central/rev/31e3b521775b