395962 - ActionMonkey: Add SpiderMonkey request model to MMgc

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Jason Orendorff [:jorendorff]]

SpiderMonkey uses a request model for two things:  to prevent one thread from touching GC-managed objects while another thread is doing GC; and to prevent threads from touching the same object at the same time.  The goal of this bug is to lift the former piece into tamarin-central, enabled at compile time with an MMGC_THREADSAFE macro.

The cross-platform build will support this with an option to configure.py.

Actionmonkey will be updated to use the new MMGC_THREADSAFE features to implement JS_BeginRequest and so forth.

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> SpiderMonkey uses a request model for two things:  to prevent one thread from
> touching GC-managed objects while another thread is doing GC; and to prevent
> threads from touching the same object at the same time.

The thread-safety for object access is more subtle than that. Two or more requests  (JS API activations including of the interpreter) running in two or more threads can in fact share access to a given object on a fine-grained basis, but at the cost of compare-and-swap optimized synchronization that falls back to OS-level mutexes when there's contention.

The win of the request model is for most objects, which are not shared among threads racing in requests.

Most objects are ST. A few shared ones are (likely) shared sequentially. For both of these cases, the request model eliminates all locking overhead of the CAS or OS mutex kind, imposing only a short and usually well-predicted branch test depending on a couple of extra loads.

/be

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1

I'm pretty happy with this patch, except maybe it makes it look a little *too* easy.  It wasn't that easy to write, honest. :)

This patch is against actionmonkey-tamarin, but the only relevant difference between that and tamarin-central at this point is a few extra callbacks.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 289741 [details] [diff] [review]
v1

Adding r?=treilly.

Tom, if you want you can wait for me to post the version that targets tamarin-central (next Monday). It'll be pretty much the same patch though (see previous comment).

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v1 against tamarin-central

Basically the same patch, but this one works on top of tamarin-central.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v2

I merged in the latest Tamarin changes, and now I'm getting sporadic crashes when running tests against the Release build **without** MMGC_THREADSAFE.  Looking into it.  In the meantime here's an updated patch, just a snapshot.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 291554 [details] [diff] [review]
v2

The crashes were nothing.  "xcodebuild -blahblah clean" fixed it.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: changes from v2 to v3 (approximate)

v3 coming up.  Here are the significant differences from v2.  (v3 might also incorporate some merging, or dropping an unrelated patch from my queue, trivial stuff.)

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v3

Two changes suggested by treilly:

* move assertion macros to GC.h so they can be used in GCAlloc.cpp
* clean up GCWeakRef allocation in MMGC_THREADSAFE mode

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 292067 [details] [diff] [review]
v3

Tommy, if you'd rather have Ed do the review, let me know.

Comment 10

[Tommy Reilly]

Comment on attachment 292067 [details] [diff] [review]
v3

Looks good to me.

Comment 11

[Brendan Eich [:brendan]]

Oop, I was waiting for a new patch -- should I review the attached one? Thanks,

/be

Comment 12

[Jason Orendorff [:jorendorff]]

Yes, please review this one.  I do need to unbitrot it, but nothing really significant has changed.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v4 - same as v3, just unbitrotted

Actually I did tweak one line of a comment in here:

                /**
                 * Wait for the current GC operation, if any, to finish.
                 *
-                * @access Requires(m_lock)
+                * @access Requires(!request && m_lock)
                 */
                void WaitForGCDone()

but otherwise v4 is just a newer v3.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 293149 [details] [diff] [review]
v4 - same as v3, just unbitrotted

Up to date still? I thought I'd see precollect, but it's not removed by this patch, and not visible in the context. It's still in my up-to-date actionmonkey-tamarin MMgc/GC.cpp.

/be

Comment 15

[Jason Orendorff [:jorendorff]]

This patch is against tamarin-central, so precollect isn't there.

The idea is to land this in tamarin-central, then merge it over to actionmonkey-tamarin.  I've already done that merge in my own repo; for the effects on precollect(), see lines 71-88 of this patch:

http://hg.mozilla.org/users/jorendorff_mozilla.com/actionmonkey-tamarin-patches/?file/tip/mmgc-threadsafe

Comment 16

[Brendan Eich [:brendan]]

Just wondering why precollect *and* the nested-lock enterExclusiveGC callback?

/be

Comment 17

[Jason Orendorff [:jorendorff]]

It's to retain existing SpiderMonkey behavior.

The JSGC_BEGIN gcCallback has to be called from outside the gc lock.  precollect() is for that.

A few things have to happen while the gc lock is held, after the thread has decided to do GC but before the lock is released.  For example, rt->gcThread and rt->gcContext are set.  enterExclusiveGC() is for those.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 293149 [details] [diff] [review]
v4 - same as v3, just unbitrotted

>+
>+		{
>+			USING_CALLBACK_LIST(this);
>+			for (GCCallback *cb = m_callbacks; cb; cb = cb->nextCB)
>+				cb->enterExclusiveGC();

Only thought is to comment this method in the class, warning about it nesting in the GC lock (first in its own callback list lock), a deadlock hazard.

Looks good otherwise. I'll be reviewing more tonight.

/be

Comment 19

[Jason Orendorff [:jorendorff]]

Attachment: changes from v4 to v5

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: v5 - (pushed)

pushed changeset 4d9bc2b6eebd to tamarin-central

(but see bug 410839)