481922 - String.slice() memory use

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Oster (:Parasyte)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090306 Minefield/3.2a1pre
Build Identifier: 

See the Dromaeo test in the URL, which shows the bug pretty well. About 60% through the test, Firefox begins eating memory like candy; huge memory spike. On a machine with 1GB RAM or less, the system quickly starts to thrash paging memory.

Happens in latest 3.1b4pre and mozilla-central nightly builds. Does not happen in 3.0.7 release. (No memory spike at all)

I've tested builds as far back as 2009-02-03-02-tracemonkey -- has the bug.

Reproducible: Always

Comment 1

[Jason Oster (:Parasyte)]

See also: Bug 477576.

Comment 2

[David Mandelin [:dmandelin]]

This appears to be related to bug 477576. With this bug, inside js_GC, we get to a point where maxMallocBytes == mallocBytes, but runtime->gcPoke is not set, so we don't GC, which was the same situation as bug 477576.

In bug 477576, the problem was a missing call to GC_POKE. For some reason that path is now not getting hit in this test often enough to get us to GC. And strangely, my reduced test case from before is still OK, but a moderately reduced version of dromaeo-object-string that does pretty much the same thing as the reduced case does spike. I suspect there was some change to how these variables are accessed in TM that bypasses the old path more often. I think next I'll try to bisect, and if that fails (i.e., we get the spike with the changeset of the fix to bug 477576) I'll have to dig deeper into how 'ret' is being set.

Comment 3

[Igor Bukanov]

I think the problem is that jited code never sets gcPoke. In bug 477576 the code is not jited since the example involves closures.

I suspect a possible way to solve this is to drop gcPoke checks in the allocator and to remove most of the places that sets it. The idea is use gcPoke only to restart the GC if a finalizer calls js_RemoveRoot or other APIs that could add more garbage.

Comment 4

[Brendan Eich [:brendan]]

Long ago, and possibly still, gcPoke was important to avoid re-scanning a live heap and finding no garbage. Possibly we've scheduled GC better now and run only when something like closing a DOM window (tab) has happened, giving affirmative advice that there will be garbage to collect.

/be

Comment 5

[Andreas Gal :gal]

FYI, we currently do not trace slice. I am about to add support for it though.

Comment 6

[David Mandelin [:dmandelin]]

OK, I found the cause. It's just a variant of bug 477576 that I didn't notice at the time; it's unrelated to the JIT. In the actual Dromaeo harness, the closure is called after returning from the function that created it, while in my reduced test case for that bug, it's called later. So we go through a different path in CallPropertyOp, namely this part:

    if (!fp) {
        i += CALL_CLASS_FIXED_RESERVED_SLOTS;
        if (kind == JSCPK_VAR)
            i += fun->nargs;
        else
            JS_ASSERT(kind == JSCPK_ARG);
        return setter
               ? JS_SetReservedSlot(cx, obj, i, *vp)
               : JS_GetReservedSlot(cx, obj, i, vp);
    }

Thus we return before hitting the GC_POKE I added for the other bug. So, we could just add a GC_POKE here, and in fact, doing so fixes the bug.

But is that the right thing to do? (igor, brendan?) The GC_POKE could also presumably go in JS_SetReservedSlot. 


Regarding the more general issue of gcPoke, I think we now collect based on "a bunch of stuff happened since the last GC", where "a bunch of stuff" is that we allocated 32MB via JS_Malloc (and gcPoke) or 16x times as much arena space (as at last GC). 

I'm not sure exactly what happens if gcPoke were not used. The main risk scenario that comes to mind is that we allocate 32MB, then GC, don't collect anything, and then we won't collect until we've allocated another 32MB. But presumably, that means we can't have more than 64MB of uncollected garbage. Also, if we do allocate another 32MB and there is any garbage, we'll get it then. So it seems OK to me. I can try it later, although I'm not sure how to extensively test such a thing.

Comment 7

[Brendan Eich [:brendan]]

D'oh! Poke in JS_SetReservedSlot, please.

Yeah, we might be able to lose gcPoke. But I'd rather do it in pre-beta phase, not for b4 the last beta for 3.5.

/be

Comment 8

[David Mandelin [:dmandelin]]

Attachment: Patch

Fixing with GC_POKE for now. Per brendan, the second arg isn't actually used, so I'm not bothering to copy logic to read out the slot value and probably getting it wrong.

Comment 10

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/13fa92323615

Comment 11

[Igor Bukanov]

(In reply to comment #6)
> I'm not sure exactly what happens if gcPoke were not used.

gcPoke serves 2 purposes:

1. Prevent the GC when the allocator hits the malloc limit heuristic. The idea behind this is to avoid frequent GC if a script triggers huge memory allocation without generating GC garbage like in Array(100<<20).join(','); which allocates 200MB using JS_malloc.

2. Restart the GC when during the sweeping phase of the GC a finalizer calls js_RemoveRoot or other API that may generate more garbage. 

I would claim that if the first usage would be switched of, then no observable effects should happen. Moreover, we the allocator with no gcPoke checks would run the GC more frequently, then with the current gcPoke checks we have a bug that should be fixed. The observation here is that if a script can *repeatedly* hit the malloc limit without setting gcPoke, then it means it can repeatedly generate a garbage that is not recorded meaning that gcPoke does not work. The bug 482018 is the latest demonstration of this.

Now, if we disable the gcPoke checks in the allocator, we do not need to set the flag in the interpreter or jited code (see bug 482016) as they cannot run during the GC.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #6)
> Now, if we disable the gcPoke checks in the allocator, we do not need to set
> the flag in the interpreter or jited code (see bug 482016) as they cannot run
> during the GC.

Yes, this is the way to go (I was groping toward your analysis in bug 482018 comment 1 just now).

/be

Comment 13

[Jason Oster (:Parasyte)]

Just another report from a user's POV; I tried the Dromaeo test in this morning's TM nightly. http://dromaeo.com/?id=61880 The memory hunger is definitely solved. Thanks for the quick fix!

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/13fa92323615

Comment 15

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5da95112c554

Comment 16

[Aakash Desai [:aakashd]]

verified FIXED as it passed the dromaeo test suite with no memory spikes on both trunk and shiretoko builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090416 Minefield/3.6a1pre ID:20090416030845

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090417 Shiretoko/3.5b4pre ID:20090417030722