388011 - MMgc: Add support for JS_SetGCCallback and JS_SetGCThingCallback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: patch against hg.m.o/actionmonkey-tamarin for MMgc callbacks

1.  Added a precollect() method to the existing GCCallback class.  That was easy.

2.  Added a GCEdgeCallback class, based on GCCallback, with a single method, foundEdgeTo(); and added new method GC::FireFoundEdgeTo(ptr).  (This could have been a GCCallback method.  Likely slower that way.)

3.  Tried to make sure GC::FireFoundEdgeTo() is called in all the right places.  I grepped for SetBit, SetMark, GetBits, |=, and a few others, and found five places.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: simple test program

The dumb little test/debug program I've been using for MMgc hacking.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Jason, did you mean to request review from anyone in particular? You can add a requestee in the field that pops up when you select ? for the review flag.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 272273 [details] [diff] [review]
patch against hg.m.o/actionmonkey-tamarin for MMgc callbacks

Sorry for the noise.

I think Mardak's the right person to review this one.

Comment 5

[Ed Lee :Mardak]

(In reply to comment #1)
> 1.  Added a precollect() method to the existing GCCallback class.
precollect is to maintain the GCcallback ability to cancel/defer at JSGC_BEGIN?
What /should/ be the behavior of differing precollects? (Well, will we even run into that situation?)
The reason for putting the precollect check before the other returns is to make sure the callback is called, yes? So we'll need to make sure the other GCcallbacks aren't skipped because an earlier callback says abort.

> 2.  Added a GCEdgeCallback class,
Is GCEdgeCallback's usage pattern similar enough to GCCallback to use the same data structure? I suppose if it isn't efficient/optimized correctly now, we can worry about that later after we get things working. :)

> 3.  FireFoundEdgeTo() is called in all the right places.
I'll double check the mark -> FireFoundEdge locations in a bit and style nit in a bit :p (actually about style, we should follow MMgc's, but that doesn't seem consistent to begin with in some places..)

Comment 6

[Jason Orendorff [:jorendorff]]

> (In reply to comment #1)
> > 1.  Added a precollect() method to the existing GCCallback class.
> precollect is to maintain the GCcallback ability to cancel/defer
> at JSGC_BEGIN?

Yes.

> What /should/ be the behavior of differing precollects? (Well, will we even run
> into that situation?)

Not through JSAPI, which only allows one JSGCCallback and one JSGCThingCallback at a time.

> The reason for putting the precollect check before the other returns is to make
> sure the callback is called, yes? So we'll need to make sure the other
> GCcallbacks aren't skipped because an earlier callback says abort.

All right, I'll change this.

> > 2.  Added a GCEdgeCallback class,
> Is GCEdgeCallback's usage pattern similar enough to GCCallback to use the same
> data structure? I suppose if it isn't efficient/optimized correctly now, we can
> worry about that later after we get things working. :)

My thinking was: MouseListener vs. MouseMotionListener, you know?  But yeah, it would be a lot simpler just to make foundEdgeTo() a method of GCCallback.  We should ask Tom Reilly whether it's premature optimization (or just good sense) to make them separate classes.

I'll wait for the rest of your comments.  Thanks.

Comment 7

[Ed Lee :Mardak]

Comment on attachment 272273 [details] [diff] [review]
patch against hg.m.o/actionmonkey-tamarin for MMgc callbacks

(In reply to comment #6)
> Not through JSAPI, which only allows one JSGCCallback and one JSGCThingCallback
Okay. We'll just provide the multiple "ThingCallback" functionality to mirror the multiple "GCCallback"s supported by MMgc.

>+	 * GCEdgeCallback is an interface that allows the application to get
>+	 * a callback every time the GC follows a pointer while marking.
So the patch calls FireFoundEdgeTo for 3 things: edges out of a "Thing" (e.g., a root) but not the "Thing" itself (as expected); write barriers; and newly allocated memory during a collection.

For GC{,Large}Alloc::Alloc(), should we be notifying in the first place? The Mark bit is set to make sure it doesn't get collected, but the GC isn't really "finding" it during a traversal. Whatever called ::Alloc wouldn't even know the address of the new memory item when FireFoundEdgeTo is called.

>+			if (!cb->precollect()) {
>+				return;
As mentioned earlier, change to call all callbacks then return if any failed. r? with this change.

Comment 8

[Jason Orendorff [:jorendorff]]

Argh-- the reason the Alloc() methods are calling FireFoundEdgeTo() is to try and make certain finalizer behavior (things like creating objects from within finalizers, which MMgc supports) cycle-collector-safe.

I've spent the past hour trying to figure out if it's really going to work or not, and at this point I think I've found enough holes that it's not worth trying.  So yeah, I will remove those calls to FireFoundEdgeTo() from *::Alloc().

This is slightly scary.  MMgc allows a lot of new and exciting things to happen in finalizers, and I don't know how much Tamarin takes advantage of them.  One more reason to get rid of the cycle collector as soon as possible.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Argh-- the reason the Alloc() methods are calling FireFoundEdgeTo() is to try
> and make certain finalizer behavior (things like creating objects from within
> finalizers, which MMgc supports) cycle-collector-safe.

Please don't over-engineer. Right now jsgc.c does not support allocation from a finalizer, so this can't happen with cycle-collector and the 1.9 cvs trunk. When we get MMgc integrated for both ActionMonkey and the DOM in the context of mozilla 2, we won't care about cycle collector except for legacy XPCOM objects (if that: there's a chance we'll drop XPCOM and its C++ binding -- not its JS binding -- utterly from moz2).

Just because MMgc supports something doesn't mean we have to make that work with Mozilla code that is not yet integrated with MMgc, and that can't yet put that workload on MMgc (because it can't happen in 1.9 with jsgc).

/be

Comment 10

[Brendan Eich [:brendan]]

Sorry if that sounded too harsh. I'm concerned with stage 0 taking on unnecessary work. Best, in fact, if you can continue to assert that no finalizer (yet) calls the allocator, as jsgc.c does.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: patch addressing comments

rolled back changes in *::Alloc(); now calling GCCallback::precollect() on all callbacks, even after a veto.

Comment 12

[Jason Orendorff [:jorendorff]]

pushed to ssh://hg.mozilla.org/actionmonkey-tamarin/