Closed Bug 394297 Opened 17 years ago Closed 16 years ago

Make MMgc thread-safe (as a compile-time option)

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED WONTFIX

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(2 obsolete files)

This was briefly discussed on the tamarin-devel mailing list:
  https://mail.mozilla.org/pipermail/tamarin-devel/2007-August/000013.html

There are several pieces to this.  Here is one possible plan (not necessarily *the* plan):

- Lift the SpiderMonkey request model into MMgc.
  http://developer.mozilla.org/en/docs/SpiderMonkey_Internals:_Thread_Safety

Just as in SpiderMonkey, this would be #ifdef'd out by default, enabled at compile time with -DMMGC_THREADSAFE or such.

- Ensure that GC::Collect scans not only the calling thread's stack, but all threads that are in requests.  (This is pretty easy to implement: when a thread calls SuspendRequest, just root its stack.)

- Make DRC thread-safe.  (It's not clear to me that anyone wants this.  The story could be "RCObject is never thread-safe -- always use GCObject for objects that may be seen by multiple threads".)

- Make incremental GC thread-safe.
  https://mail.mozilla.org/pipermail/tamarin-devel/2007-August/000017.html
Depends on: 395963
This is now an ActionMonkey Stage 1 goal.  I'm doing this now mainly to support efforts currently underway (by bsmedberg and taras) to convert Mozilla's XPCOM objects from AddRef/Release to MMgc.

I think the above is the approach we want to take.  Making DRC at all safe for MT code is not included in this bug for now.
Blocks: 393023
Depends on: 393038, 395962
No longer depends on: 395963
Depends on: 395963
Attached patch v0 - snapshot (obsolete) — Splinter Review
Phew.  I thought this would be done yesterday, or even Wednesday, but I got more confused than I thought I would!

I wanted to make a patch that is simple and contains the essence of the request model.  It would have been easier to submit 2000+ lines of incomprehensible spaghetti code.

Here's a snapshot of the simplified version.  Untested, but the concepts are there.  There are <500 lines of new code.  There are very few changes to MMgc in it's default non-MMGC_THREADSAFE configuration, and I think all of those are trivial.  Flip on MMGC_THREADSAFE, and you get the "multiple application threads or one GC thread" behavior, and each GC scans all threads' stacks.

But this lacks several features SpiderMonkey will need:  sentry objects to enter and leave requests; request contexts; multiple (nested) requests per "context"; multiple contexts per thread; public locking methods for GC::m_lock; and restarting GC as needed when multiple threads ask for GC at around the same time.  I think all these are pretty easy to add.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Attached patch v0a - snapshot (obsolete) — Splinter Review
This patch is for people to skim, not to apply.  It's got "infinite" diff-context, so the Bugzilla "Diff" link should show the whole file.

Take a look; focus on GC.h, where I've added a ton of comments.

Those funny-looking @access comments document the extra access restrictions on methods and fields in MMGC_THREADSAFE builds.  Documenting these restrictions is (imho) very important; using this shorthand is a lot more concise than using English.  For an explanation of how to read them, read the comment on MMgc::GC::m_lock (in the patch), or the first section of http://wiki.mozilla.org/MMgc_thread_safety_annotations .
Attachment #284701 - Attachment is obsolete: true
Depends on: 404879
Comment on attachment 286326 [details] [diff] [review]
v0a - snapshot

A newer version of the patch is in bug 395962.
Attachment #286326 - Attachment is obsolete: true
No longer blocks: 393023
No longer depends on: 404879
Isn't this done now? Bug 395962 is now RESO/FIX.
Bug 395963 is still open.  Coincidentally that's what I'm working on today.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: