We need to start using MMgc, and delete all the JSGCArena stuff. For the sake of having sane-sized patches, I think we should remove newborns and stack TempValueRooters in separate bugs. We already have bug 388622 for newborns.

MMgc::GC::~GC calls finalizers for all remaining kFinalize allocations. This is a problem for SpiderMonkey as it stands, because the GC is torn down within JS_DestroyRuntime, *after* the last JSContext, leading quickly to crashes. E.g. JSClass::finalize callbacks expect to be passed a JSContext *. They're unhappy if you pass NULL. Proposed solution: tear down GC when the last context is destroyed. However, there are comments in JS_DestroyRuntime that suggest that Mozilla leaks contexts. Are these comments still relevant? If so, I'll just leak the GC object in this case.

The above idea can't work. JS_AddNewRootRT and quite a few other GC-related APIs require only a JSRuntime pointer. I'm running out of ideas. I'm going to try making the destructors check for NULL gcContext. So if you manage to leak a JSObject, well, its JSClass::finalizer just won't get called. Scary. Conservative GC means it's always possible a JSObject just won't be collected until ~GC. So there's a practical possibility that an important finalizer just won't be called. OK, here's one other thought: maybe the LAST_CONTEXT gc should not scan the stack. So the last GC is exact. This might be OK because JS_DestroyContext (unlike general GC) happens only in very specific places that the embedder already understands to be special.

Concerning comment 1 and comment 2: Per Brendan, if a JSObject leaks, we won't call its finalize hook. In other words, we'll let this slide. If it turns out to be a problem in practice, we've got the LAST_CONTEXT idea in our back pocket.

My current snapshot. This is an MQ patch. Currently it depends on some patches found in other bugs, not yet committed. It definitely needs bug 390314, and it needs JSFunction to be a subclass of JSFinalizedGCThing. What's missing: JS_SetGCThingCallback doesn't work (just a matter of hooking it up, I hope). Two tests fail, both of them reporting the same leak, really, js1_5/GC/regress-383269-01.js and -02.js. I think the leak can be blamed on conservative stack scanning, but I'm not sure.

This patch is against actionmonkey-tamarin. No prerequisites. The changes are: 1. MMgc::GC needs a virtual destructor so I can subclass it. 2. The order of object finalization is reversed, so that the largest objects are collected *first*; JSObject finalizers need to be able to count on the name and other fields not yet being collected. Sort of a hack at the moment.

Updated. gcThingCallback should work now; the only way I know of to test it is by building Firefox, though, so I'm doing that now.

Igor, Brendan, anyone else: Your code reviews on these patches would be helpful.

Various comments are in the patch with some extra things removed. So I'm commenting on my changes below... - * Ensure that GC-allocated JSFunction and JSObject would go to different - * lists so we can easily finalize JSObject before JSFunction. See comments - * in js_GC. - */ -JS_STATIC_ASSERT(GC_FREELIST_INDEX(sizeof(JSFunction)) != - GC_FREELIST_INDEX(sizeof(JSObject))); Igor asked a while back about a way to maintain the "JSObject first". We can still do this by doing something similar of checking sizes. We were thinking about lastmark? @@ -364,17 +356,16 @@ js_InitGC(JSRuntime *rt, uint32 maxbytes - rt->gc->nogc = true; Already set in the constructor. About |nogc|, is that only to avoid |Collect| if |greedy| is set? But relying on MMgc for trying to GC during NewGCThing doesn't keep the same GC behavior of try alloc and try again after GC if failed the first time. -JSGC::foundEdgeTo(const void *buf) +JSGC::foundEdgeTo(const void *thing) Do we want to be consistent with the other gcThingCallback call? -#define DEBUG_gchist gchist is gone from NewGCThing @@ -787,17 +768,19 @@ js_NewGCThing(JSContext *cx, uintN flags - : 0); + // zero GCThingFlags for jsdouble, but not + // really necessary because it gets set below + : MMgc::GC::kZero); Probably saves some overhead if we don't kZero it..

Comment on attachment 276260 [details] [diff] [review] v1 - tamarin (In reply to comment #10) > -JS_STATIC_ASSERT(GC_FREELIST_INDEX(sizeof(JSFunction)) != > - GC_FREELIST_INDEX(sizeof(JSObject))); > Igor asked a while back about a way to maintain the "JSObject first". We can > still do this by doing something similar of checking sizes. We were thinking > about lastmark? >diff -r e4cd7a043e92 MMgc/GC.cpp >- for(int i=0; i < kNumSizeClasses; i++) { >+ for(int i=kNumSizeClasses; i-- > 0;) { Nevermind! ;) >+ if(gc) { Are we following spacing of the existing code? for |if | and |for | above? >+ void Destroy() { SetGC(NULL); } Again syntax.. we have this on split lines, but I suppose just a few lines above this it's a single line. :p

Thanks for the review. Regarding |if(...)| and |for(...)| spacing, I'm carefully following the practice of the immediately nearby code. Which in that code varies from line to line. Just looking at the patch, that's unobvious. Apparently strings can be I am concerned that this patch crashes Firefox. There's something squirrelly going on regarding JSString finalization.

(In reply to comment #10) > About |nogc|, is that only to avoid |Collect| if |greedy| is set? But relying > on MMgc for trying to GC during NewGCThing doesn't keep the same GC behavior of > try alloc and try again after GC if failed the first time. How it used to work is, when you call js_NewGCThing, it might decide to call js_GC. It heuristically guesses when it's a good idea to do this. MMgc behaves the same way: when you call Alloc, sometimes that triggers a collection. |nogc| suppresses that behavior, so Alloc never triggers a collection. In short, GC happens *only* when jsgc.cpp specifically asks for it: only via js_GC. The request model depends on GC happening *only* from js_GC. We will likely push the request model into Tamarin and MMgc someday. > -JSGC::foundEdgeTo(const void *buf) > +JSGC::foundEdgeTo(const void *thing) > Do we want to be consistent with the other gcThingCallback call? It's consistent with the interface it's implementing, and a perhaps too-subtle reminder that MMgc doesn't guarantee that's a GCThing. It could be something else someone allocated from the same GC. > @@ -787,17 +768,19 @@ js_NewGCThing(JSContext *cx, uintN flags > - : 0); > + // zero GCThingFlags for jsdouble, but not > + // really necessary because it gets set below > + : MMgc::GC::kZero); > Probably saves some overhead if we don't kZero it.. Insignificant overhead probably. But as it's unnecessary, let's skip it. Thanks for all the comments.

OK, incorporated those comments. Also put back some code transferring cx->thread->gcMallocBytes to rt->gcMallocBytes; I shouldn't be deleting that. I just noticed that this patch has a comment in JS_MaybeGC() that's kind of dumb: + /* Bug ### - Find new heuristic to determine when to GC. */ I really do want to leave this for another bug. I'll file that bug and post on tamarin-devel today. And of course I'll check in with the real bug number. Admittedly this is a regression in spirit, but it's a nice isolated problem; I'll make the bug block stage 0.

(In reply to comment #13) > MMgc behaves the same way: when you call Alloc, sometimes that triggers a > collection. Ah yup, it was several calls down, but eventually Alloc returns to the GC for AllocBlock which can decide to start an incremental or full Collect. > > Probably saves some overhead if we don't kZero it.. > Insignificant overhead probably. But as it's unnecessary, let's skip it. Sure.

Comment on attachment 277092 [details] [diff] [review] v2 - actionmonkey (In reply to comment #14) > cx->thread->gcMallocBytes to rt->gcMallocBytes; I shouldn't be deleting that. Should we try to remove this check and the gcZeal and perhaps combine it with the logic for MaybeGC? But MMgc doesn't have a way to set a "max bytes", so I suppose this spot is our last chance to enforce it. > + /* Bug ### - Find new heuristic to determine when to GC. */ (In reply to comment #15) > Ah yup, it was several calls down, but eventually Alloc returns to the GC for > AllocBlock which can decide to start an incremental or full Collect. Yeah, the logic for AllocBlock does something along the lines of what we want for conditionally GCing.

Same as v2-actionmonkey, but unbitrotted (there were a few largeish changes in trunk js/src). Also changed the comment in JS_MaybeGC to have the correct followup bug number.

Comment on attachment 276260 [details] [diff] [review] v1 - tamarin > void GC::Finalize() > { >- for(int i=0; i < kNumSizeClasses; i++) { >+ for(int i=kNumSizeClasses; i-- > 0;) { Comment why this loop iterates in reverse size class order. I didn't review the GC{,Edge}Callback code, but wanted to point out that head-less (non-circular) doubly linked lists can be more efficiently coded (without going crazy with the xor trick) using a double-indirect prevCB pointer: void GC::AddCallback(GCCallback *cb) { CheckThread(); cb->prevCB = &m_callbacks; cb->nextCB = m_callbacks; if(m_callbacks) m_callbacks->prevCB = &cb->nextCB; m_callbacks = cb; } void GC::RemoveCallback(GCCallback *cb) { CheckThread(); *cb->prevCB = cb->nextCB; if(cb->nextCB) cb->nextCB->prevCB = cb->prevCB; } Saves an if/else in Remove, not a huge deal but worth it -- especially if you could make a helper subclass for m_callbacks and m_edgeCallbacks to share. /be

To review the patches in the bug I need to check details about MMgc works to avoid clueless rubber stumping. So my review would come some time during the next time.

Comment on attachment 280516 [details] [diff] [review] v4 - actionmonkey (same as v3 but unbitrotted) >+++ b/js/src/jscntxt.h >@@ -185,25 +182,23 @@ struct JSRuntime { > struct JSRuntime { > /* Runtime state, synchronized by the stateChange/gcLock condvar/lock. */ > JSRuntimeState state; > > /* Context create/destroy callback. */ > JSContextCallback cxCallback; > > /* Garbage collector state, used by jsgc.c. */ >- MMgc::GC *gc; >+ JSGC *gc; > MMgc::GCRoot *gcMarkerRoot; /* The sole GCRoot of gc. */ Why using "JSGC *gc" and not "JSGC gc" here? >+void >+JSGC::presweep() >+{ >+ JSContext *cx = rt->gcContext; >+ >+ /* The mark phase is over. */ >+ rt->gcMarkingTracer = NULL; >+ >+ if (cx) { How it is possible for cx to be null here? > if (js_PushLocalRoot(cx, lrs, (jsval) thing) < 0) { > /* > * When we fail for a thing allocated through the tail of the last > * arena, thing's flag byte is not initialized. So to prevent GC > * accessing the uninitialized flags during the finalization, we > * always mark the thing as final. See bug 337407. >+ * >+ * UPDATE - with MMgc, it's dangerous to leave this allocation >+ * lying around, since MMgc is going to try to call the virtual >+ * destructor through a vptr that's no there; and it's *not* >+ * dangerous that PushLocalRoot failed, since MMgc scans the >+ * stack. So we now succeed instead of failing here. > */ Why not to remove local roots all together?

(In reply to comment #21) > > struct JSRuntime { > > .... > > /* Garbage collector state, used by jsgc.c. */ > >- MMgc::GC *gc; > >+ JSGC *gc; > > MMgc::GCRoot *gcMarkerRoot; /* The sole GCRoot of gc. */ > > Why using "JSGC *gc" and not "JSGC gc" here? Partly just to make the minimum possible patch. There's another reason, though. While it's a good idea to move all JSRuntime fields to C++-style initialization eventually, I'd rather change them all at once. Mixing C and C++ initialization styles introduces some unobvious changes and makes the code a little tricky. For example, logically the GC should be torn down before the gcLock that protects it. But C++ destructors are always called last, after C cleanup code. That would mean JSRuntime::gc gets destroyed after the locks. I don't think there's any danger if they're destroyed that way, but I'm not familiar enough with the code to go asking for trouble. Switching to RAII one class at a time seems like the Right Thing. > >+JSGC::presweep() > >+{ > >+ ... > >+ if (cx) { > > How it is possible for cx to be null here? MMgc::GC::~GC does a final collection. This happens from JS_DestroyRuntime, after all JSContexts are gone. All the GCThing destructors have to handle this case specially, too. Ideally this final collection shouldn't find any garbage, but thanks to conservative GC it's always possible. (Brendan and I talked about this at some length. I was considering things like keeping a JSRuntime around (in order to be able to call the destructor hook if a JSObject gets collected right at the end) or doing a special, exact, no-stack-scanning collection when the last JSContext is destroyed. Brendan's advice was "don't borrow trouble". That's why JSObject::~JSObject behaves the way it does in this patch.) > Why not to remove local roots all together? I will--see bug 388622, which depends on this bug. But that is a big patch, and so is this one. They're logically separate. The comment you quoted ("UPDATE - with MMgc, ...") is temporary. Bug 388622 will remove all that code.

Pushed to actionmonkey! changeset 3e577a78ce37

Pushed to actionmonkey-tamarin, too, belatedly. changeset 01d17c7a52cb

Comment on attachment 276260 [details] [diff] [review] v1 - tamarin I forgot to r+ this earlier.