I think the way to do this is to make JSRuntime derive from MMgc::GCFinalizedObject (or maybe JSFinalizedGCThing) rather than MMgc::GCRoot, and give it its own CustomMark() method that calls js_TraceRuntime(). Of course this means we need an MMgc::GCRoot somewhere, and I think the answer for that is to create a custom class JSRuntimeGCRoot : public MMgc::GCRoot and have it and the JSRuntime point to each other.

Why not MI? Why make an extra object/allocation if you don't need one? /be

- should probably assert that we do have a tracer// - js_TraceRuntime is supposed to be called with true if it's GC_LAST_DITCH.. - no need to delete the JSRuntime in Destroy -- it's gc managed - jscntxt.cpp the place for defining JSRuntime methods?

(In reply to comment #1) > Why not MI? Why make an extra object/allocation if you don't need one? Can I depend on the compiler to lay out the first base class at offset 0? MMgc requires the GCFinalizedObject vptr to be at offset 0. Either way, I think I can avoid the extra allocation somehow.

Left-most direct inherited real base first, AFAIK on all platforms we care about. Is there a bug requesting that MMgc deal with inner-object pointers? /be

Because we're using conservative GC, some JSObjects may be left over for JS_DestroyRuntime(). There is no way to prevent this. Those objects' finalizers will be called; MMgc::GC::~GC does this for all remaining kFinalize objects; it would be a hack to prevent this. I think those finalizers must happen *before* the JSRuntime itself is destroyed; therefore the JSRuntime itself must not be GC-allocated. It must be destroyed after the GC is torn down. Since JSRuntime can't be GC-allocated, it can't have a CustomMark() method. So we introduce a separate object, JSRuntimeGCMarker, that is GC-allocated and exists solely so that it's CustomMark() method can call JS_TraceRuntime(). We also have a GCRoot that roots the gcMarker. This patch aims to get the order of setup and teardown right. JSRuntime creation looks like this: 1 - Allocate and set up JSRuntime. 2 - Create the MMgc::GC object. 3 - Create the JSRuntimeGCMarker object (gc-allocated). 4 - Create the GCRoot to root the JSRuntimeGCMarker. Destruction is the same thing in reverse: 4 - Destroy the GCRoot. 3 - (no-op, we'll let the JSRuntimeGCMarker be gc'd) 2 - Destroy the MMgc::GC object. 1 - Tear down and deallocate JSRuntime. The patch moves GC initialization after lock initialization, for symmetry. GC init/finish stuff that had crept into jsapi.cpp has been moved into js_InitGC()/js_FinishGC(). As Mardak mentioned above, js_TraceRuntime must be called with allAtoms=true if the collection is GC_LAST_DITCH. This patch does this by wrapping most of the body of js_GC() in a JS_{KEEP/UNKEEP}_ATOMS pair. Then allAtoms does not need to be explicitly passed to js_TraceRuntime; it can just test the rc->gcKeepAtoms field directly. This leads to some other simplifications. This can be simplified to get rid of one or two malloc()s, but it would require a pretty big rewrite. The nice thing about |new| is that it lets you initialize things whenever and from wherever you want. It'll do for today. Tomorrow, RAII ftw.

js/tests pass with the new patch.

Comment on attachment 274727 [details] [diff] [review] another stab This patch is ugly. Hang on.

Should keep Igor in the loop here. /be

OK, this is a little nicer. gcRunning, instead of being a boolean, becomes a GCInvocationKind. Fewer pointless edits in js_GC().

(In reply to comment #9) > Created an attachment (id=274830) [details] > v3 > > OK, this is a little nicer. gcRunning, instead of being a boolean, becomes a > GCInvocationKind. Fewer pointless edits in js_GC(). Is the attached patch the one that goes with this comment? I see no gcRunning change. I have some review comments, but I'll hold them pending confirmation that this is the right patch. /be

If that patch is for another bug, I'll gladly review over in that other bug. /be

Brendan: You're right, that was the wrong patch entirely. This is the one.

(In reply to comment #11) > If that patch is for another bug, I'll gladly review over in that other bug. It was for bug 389420. Already checked in, but it's never too late for a review. Please do.

Mostly just refreshing the patch. Plus: - JSRuntimeGCMarker::CustomMark now calls js_MarkScriptFilenames and ScanDelayedChildren. It's important; the point of the bug is to get that stuff out of js_GC and into a spot where MMgc will call them. - Now js_GC calls JSRuntimeGCMarker::CustomMark (instead of calling js_TraceRuntime directly), so it's actually exercised. Tests pass.

Comment on attachment 275432 [details] [diff] [review] v4 >- if (!js_InitGC(rt, maxbytes)) >- goto bad; Sorry, can you remind me why this moved down? >-} >- >-JS_PUBLIC_API(void) >-JS_TraceRuntime(JSTracer *trc) >-{ >- JSBool allAtoms = trc->context->runtime->gcKeepAtoms != 0; >- >- js_TraceRuntime(trc, allAtoms); > } Please keep API entry points in jsapi.c as much as possible. Only recent changes put a few new APIs (and moved one or two old ones, IIRC) to jsgc.c. This one can stay here. >-void >-js_TraceRuntime(JSTracer *trc, JSBool allAtoms) >+JS_PUBLIC_API(void) >+JS_TraceRuntime(JSTracer *trc) > { > JSRuntime *rt = trc->context->runtime; >+ JSBool allAtoms = JS_GC_KEEP_ATOMS(rt); No need for single-use variable, and indeed since you eliminated js_MarkScriptFilenames' keepAtoms param, you might do the same for js_TraceLockedAtoms (have it test GC_KEEP_ATOMS(rt)) -- or at a minimum just get rid of allAtoms here. >+/* Tracing routines may use this to determine whether to mark all atoms. */ >+#define JS_GC_KEEP_ATOMS(rt) ((rt)->gcRunning == GC_LAST_DITCH \ >+ || (rt)->gcKeepAtoms != 0) No need for JS_ prefix to this macro's name. Nit: || at "end" of first line (not counting / +\\$/ of course). >+js_MarkScriptFilenames(JSRuntime *rt) >+{ >+ JSBool keepAtoms = JS_GC_KEEP_ATOMS(rt); Ditto avoiding single-use variable nit. /be

(In reply to comment #15) > (From update of attachment 275432 [details] [diff] [review]) > > >- if (!js_InitGC(rt, maxbytes)) > >- goto bad; > > Sorry, can you remind me why this moved down? Symmetry with JS_DestroyRuntime. Members should be torn down in the opposite order they're built up. We want to be moving in the direction of RAII. I think the change fits in this patch because this is all about setup/teardown timing. But it would work without this change. > >-JS_PUBLIC_API(void) > >-JS_TraceRuntime(JSTracer *trc) > >-{ > >- JSBool allAtoms = trc->context->runtime->gcKeepAtoms != 0; > >- > >- js_TraceRuntime(trc, allAtoms); > > } > > Please keep API entry points in jsapi.c as much as possible. Only recent > changes put a few new APIs (and moved one or two old ones, IIRC) to jsgc.c. > This one can stay here. OK. Do you mean (a) keep JS_TraceRuntime as a trivial wrapper around js_TraceRuntime; or (b) move the body of js_TraceRuntime (and the 2 dhash-traversal callbacks, and whatever else they depend on) into jsapi.cpp? I did (a) for this updated patch. (branch prediction :)

Comment on attachment 275597 [details] [diff] [review] v4a - addressing brendan's comments JS_TraceRuntime along with other API entry points should go in jsapi.c, after the other JS_*Runtime function definitions. r=me with that, thanks. /be