410445 - Large Heap Growth in SunSpider regex-dna test

Status: RESOLVED DUPLICATE of bug 509725

(Core :: JavaScript Engine, defect)

Description

[Robert Sayre]

Attachment: Massif heap graph for one run

Heap profiles show this test causes us to exhibit some strange heap growth.

Comment 1

[Robert Sayre]

Attachment: Massif caller data

Comment 2

[Robert Sayre]

Attachment: Massif heap for WebKit/PCRE

Comment 3

[Robert Sayre]

Attachment: Heap profile with JS_malloc and JS_realloc as allocators

here we see str_replace allocations growing, and replace_glob thrashing a bit

Comment 4

[Robert Sayre]

Attachment: heap with a gc() call in the last loop of the regex-dna test

Comment 5

[Brian Crowder]

I think this is related to the way we handle strings in general.  I wonder if there is a thread-safe way to reuse these "garbage" strings quickly?  I don't know enough right now about our string/threading "magic" to answer authoritatively, though.

Comment 6

[Brendan Eich [:brendan]]

Are these tests run with the js shell, without a -b option? Need to test something that schedules GCs other than the last-ditch, if so. There's no point in testing such a config (alas, alioth.debian... does that, making us look even worse than we should look -- good thing we still beat Ruby [for now]).

/be

Comment 7

[Brendan Eich [:brendan]]

Existing bugs linked from the js-perf metabug to (re-)read:

Bug 120977 – Investigate reference counting for JS strings (see bug 120977 comment 3 in particular)

Bug 157334 – IE6 3x faster than Mozilla, concatenating two local strings (Igor, any thoughts on string-temporary optimizations?)

/be

Comment 8

[Brian Crowder]

Pretty sure this isn't a regression and not even strictly bad behavior.  The memory "used" in the massif graphs here is just garbage we haven't collected yet.  Not sure we can even assert there's a perf win to be had here without knowing the cost of synchronizing ref-counting thread-safely, and so on.

Brendan's test in bug 120977 I think tends to agree with sayrers result in adding gc() to the main loop of this test, which is that adding a gc() on every loop actually performs comparably with the current test.  The penalty incurred is in growing the malloc-heap.  GCing quickly releases enough heap that the malloc heap itself does not need to grow for the subsequent run, and so malloc() is overall faster.  It's a wash against the cost of the added GC.

Overall, though, there might be wins to be had in bug 120977 for ref-counted strings (using atomic inc/dec, etc? to avoid locking?  Is that sufficient?), perhaps that should be the blocker bug?  I have no idea how hard it is.

Comment 9

[Brian Crowder]

Looking at my last remark from another angle, there might be low-cost footprint wins to be had in the browser from simply GCing more often.  It's possible that delaying GC too long yields a significant footprint hit and that the cost of later allocations is significantly higher because of the heap growth.  We might find that GCing more yields lower footprint with negligible or no cost overall (since mallocs would be faster).  Hard to draw real conclusions from a testcase like this, though.

Comment 10

[Brendan Eich [:brendan]]

crowder: GC'ing more than we do currently for new-profile at startup definitely regresses Ts, so we can't simply GC more all the time.

Better GC scheduling and memory-pressure feedback have been the topics of other bugs -- do a search, see what you can find. My belief remains that temporary GC things should be managed differently altogether, so optimizing double and string temps is going to win more, and avoid regressing elsewhere, compared to fiddling with GC scheduling.

/be

Comment 11

[Brian Crowder]

How do we detect "temporary" for a case like this?

Comment 12

[Brendan Eich [:brendan]]

There's no lifetime declaration or hinting in JS, so of course the general solution is generational GC. We aren't doing that quickly or easily, but we could do approximations of it such as DRC or the like -- these still require a write barrier.

We could use brute force (as Rhino does) for doubles, and maintain a stack of jsdoubles for temps. Strings are not so easy to put on a parallel stack of course. A write barrier is required here too, to do dynamic escape (from callee frame to caller, or to heap) analysis, and copy on escape. Immutability helps: we can copy, not move.

/be

Comment 13

[Mike Schroepfer]

/be do we want to do something in this bug for 1.9 or just cover it in the two bugs you linked?

Comment 14

[Mike Schroepfer]

Moving to the wanted list

Comment 15

[Robert Sayre]

bug 509725 and bug 509826