Currently mmgc's alloca implementation assumes a single mutator per GC-instance. This patch maintains the current API, but provides thread-local management of AllocaStackSegments via the GCalloca class. The alloca slowpath, which registers AllocaStackSegments on the GC's list of rcRootSegments is now also thread-safe.

While doing cursory skim over patch, this struck me and I wanted to note it before I forgot and before formal review starts: From GC.h changes: // A safepoint cannot be called while holding this lock Its not clear to me whether this is being stated as a requirement on someone holding the lock, or as an invariant that one can assume while holding the lock (or a weird mix of both)? (Plus I'm not sure about the phrase "safepoint cannot be called" -- do you apply the verb "call" to safepoints elsewhere in your documentation? I think I've seen "request", "execute", and "reach" as verbs for it, but not "call" as I recall.)

(In reply to comment #1) > From GC.h changes: > > // A safepoint cannot be called while holding this lock > > Its not clear to me whether this is being stated as a requirement on someone > holding the lock, or as an invariant that one can assume while holding the lock > (or a weird mix of both)? > > (Plus I'm not sure about the phrase "safepoint cannot be called" -- do you > apply the verb "call" to safepoints elsewhere in your documentation? I think > I've seen "request", "execute", and "reach" as verbs for it, but not "call" as > I recall.) The word 'safepoint' is very overloaded. It can be used in *at least* the following three ways: 1) noun. A static code location that is 'safe' with respect to some task. 2) noun. The dynamic property of a group of threads all reaching some 'safe' state. E.g. "the VM is at a safepoint", or even "the GC's mutators are in a safepoint". 3) verb. 'To safepoint'. I.e. to cause 2) to happen. In the revised safepointing API (bug 575544) I've tried to only use 'safepoint' in context of meaning 1). In some of the later patches, there'll still be some left-over of meaning 2 and 3. In your example, the verb 'call' is applied to meaning 2. I'll change the comment.

Comment on attachment 515823 [details] [diff] [review] MQ rev 250 See http://asteam/hg/users/siwilkin/asymmGC-01 for an asymmGC patch queue qfinished at rev 250 (against TR rev 5916). See http://asteam.corp.adobe.com/hg/users/fklockii/tr-patch-queues for the current queue

Comment on attachment 515823 [details] [diff] [review] MQ rev 250 removing myself from review; fklockii and treilly know this code far better than I

Cursory comments, no blocking issues: How long will we live with MMGC_THREADSAFE? Ie is it anticpated that soonish we will remove the !MMGC_THREADSAFE stuff or is it for the long term? m_uninteruptableLock - I find this name odd, shouldn't a lock be named after what it protects and how it works? Are all RecursiveMutex's unsafepointable (I need to find the patch containing RecursiveMutex and see how its documented). GCalloca - unfortunate name choice, for what its worth I've been trying unsuccessfully to stop the "GC" prefix practice , not a big deal but if we do start renaming things. Since we already have GCAlloc its doubly bad. MMgc::Alloca or MMgc::StackAlloc would be my preference The patch might be a little cleaner if GCalloca was defined outside MMGC_THREADSAFE, I dislike seeing duplicate popTo impls and duplicate AllocaStackSegment decls.

(In reply to comment #6) > Cursory comments, no blocking issues: > > How long will we live with MMGC_THREADSAFE? Ie is it anticpated that soonish > we will remove the !MMGC_THREADSAFE stuff or is it for the long term? If I had my way it would land without ifdefs. If it does land with ifdefs, they should be removed as soon as new features are are allowed after Serrano. > m_uninteruptableLock - I find this name odd, shouldn't a lock be named after > what it protects and how it works? Are all RecursiveMutex's unsafepointable > (I need to find the patch containing RecursiveMutex and see how its > documented). It's a matter of taste. I've found it more useful to name the RecursiveMutexes used by asymmGC by the invariants they must respect with regard to safepoints. In the case of asymmGC, there are only two RecursiveMutexes used per GC instance: one which protects critical sections which are atomic with respect to safepoints, and one which protects critical sections that are interruptible by safepoints. Note that the blocking modes for RecursiveMutex (IMPLICIT_SAFEPOINT or NO_SAFEPOINT) are actually a property of their associated MutexLocker (i.e. an auto-locker instance), rather than the RecursiveMutex itself, as some RecursiveMutex's protect both interruptible and uninterruptible critical sections. See VMThread.h for details. Having said all this, 'GC::m_uninteruptableLock' (and 'GC::m_coarseLock', the mutex that protects interruptible critical sections) are quite historical names now. I'm open to change them, particularly as 'uninteruptable' is a misspelling. > GCalloca - unfortunate name choice, for what its worth I've been trying > unsuccessfully to stop the "GC" prefix practice , not a big deal but if we do > start renaming things. Since we already have GCAlloc its doubly bad. > MMgc::Alloca or MMgc::StackAlloc would be my preference Oh dear. I have a more GC prefixed classes coming in later patches.

Comment on attachment 521937 [details] [diff] [review] Rebased. TR rev 6090. Patch queue rev 261 I hate the code duplication but it looks fine. +1 to removing ifdefs ASAP, landing w/o would be my preference too.

(note from mtg): I'd like to see a comment above class GCalloca noting why its been refactored, in particular noting that its to support putting the state accessed via fast-path into thread-local storage in AsymmGCAutoEnter

Comment on attachment 521937 [details] [diff] [review] Rebased. TR rev 6090. Patch queue rev 261 R+ conditional on addition of comment requested from comment 10.

Retargeting to Dolores.

Tamarin isn't maintained anymore. WONTFIX remaining bugs.