Closed Bug 390314 Opened 17 years ago Closed 17 years ago

ActionMonkey: Make sure js_TraceRuntime is called from MMgc

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Status: NEW → ASSIGNED
Why not MI? Why make an extra object/allocation if you don't need one?

/be
Attached patch a quick stab.. (obsolete) — Splinter Review
- 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
Attached patch another stab (obsolete) — Splinter Review
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.
Assignee: general → jorendorff
Attachment #274727 - Flags: review?(brendan)
js/tests pass with the new patch.
Comment on attachment 274727 [details] [diff] [review]
another stab

This patch is ugly.  Hang on.
Attachment #274727 - Flags: review?(brendan)
Should keep Igor in the loop here.

/be
Attached patch v3 (obsolete) — Splinter Review
OK, this is a little nicer.  gcRunning, instead of being a boolean, becomes a GCInvocationKind.  Fewer pointless edits in js_GC().
Attachment #274653 - Attachment is obsolete: true
Attachment #274727 - Attachment is obsolete: true
Attachment #274830 - Flags: review?(brendan)
Attachment #274830 - Attachment is patch: true
Attachment #274830 - Attachment mime type: application/octet-stream → text/plain
(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
Attached patch v3, really (obsolete) — Splinter Review
Brendan:  You're right, that was the wrong patch entirely.  This is the one.
Attachment #274830 - Attachment is obsolete: true
Attachment #274830 - Flags: review?(brendan)
(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.
Attachment #274837 - Flags: review?(brendan)
Attached patch v4 (obsolete) — Splinter Review
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.
Attachment #274837 - Attachment is obsolete: true
Attachment #275432 - Flags: review?(brendan)
Attachment #274837 - Flags: review?(brendan)
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 :)
Attachment #275432 - Attachment is obsolete: true
Attachment #275597 - Flags: review?(brendan)
Attachment #275432 - Flags: review?(brendan)
Blocks: 387012, 391211
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
Attachment #275597 - Flags: review?(brendan) → review+
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.