Currently, one byte from a contiguous array per GCBlock/LargeBlock is assigned to a GCObject for its GC bits (kMarked, kQueued, kFinalizable, kHasWeakRef, kLargeObject, kVirtualGCTrace). An extra bit is introduced in later AsymmGC patches, but is ignored for now. This patch provides thread-safe access to a GCObject's GC bits. The intent is to be as efficient as possible as the bits are hot. Allocation: Unsynchronized access to a GCObject's GC bits is allowed during its allocation as the object should be unpublished to other threads. It is a thread-safety hazard to allow a thread to observe a partially allocated object. Free: Unsynchronized access to a GCObject's GC bits is allowed during its free. The object can be freed by either a single-threaded safepoint task, or by a mutator. In the mutator case, it is a thread-safety hazard to free an object which is still being accessed by another thread. Collector thread: All collection tasks are implemented as single threaded safepoint tasks, so they gain unsynchronized access to an object's GC bits via GC::GetGCBitsInSafepoint(), rather than the existing GC::GetGCBits(). GC::GetGCBitsInSafepoint() asserts that the calling thread is executing a safepoint task. Note that GC safepoint tasks have happens-before semantics with all safepointed threads registered with the GC's SafepointManager, so mutator updates to an object's GC bits will be visible. Mutator threads: By default, GC::GetGCBits() returns a const gcbits_t& rather than the current gcbits_t&. This is to make it explicit when a mutator plans to update the bits and hence requires synchronization. To gain a writable version of an object's GC bits, a mutator must use GC::GetGCBitsForModify(). All* read-modify-write operations on a gcbits_t& gained via GC::GetGCBitsForModify() are performed via a CAS (which is 4 byte aligned, we cannot just CAS the specific byte). *This is not currently done for the kVirtualGCTrace bit, which is by convention only set in an object's ctor and only cleared when it is freed. It is a discussion point whether this convention is strong enough to warrant no synchronization, given the assumption that no partially constructed objects are visible to other mutators.

(In reply to comment #0) > This is not currently done for the kVirtualGCTrace bit, which is by convention > only set in an object's ctor and only cleared when it is freed. It is a > discussion point whether this convention is strong enough to warrant no > synchronization, given the assumption that no partially constructed objects are > visible to other mutators. Bug 626612 attempts to simplify this protocol so that kVirtualGCTrace is only set during an object's allocation and free. If so, we do not need to worry about the CAS; this bit will be covered by the arguments 'Allocation:' and 'Free:' above.

Comment on attachment 515825 [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

I've only skimmed this but I have some overall impressions. One is that it may be time for us to decide for or against FASTBITS so that we can reduce the amount of code we have to maintain (I doubt we're testing the !FASTBITS code at all). Another is that ClearQueued, ClearFinalized, SetFinalize were not worth factoring when they were simple but the amount of code introduced here makes it worthwhile to reconsider that. Since GetGCBitsForModify is identical to GCBits (apart from its name). In the block comment above it's asserted that a CAS will be used to write it and I see that happening, but that does not really motivate a duplicate function, does it?

(In reply to comment #4) > Since GetGCBitsForModify is identical to GCBits (apart from its name). In the > block comment above it's asserted that a CAS will be used to write it and I see > that happening, but that does not really motivate a duplicate function, does > it? GCBits now returns a const gcbits_t&, you have to explicitly use GetGCBitsForModify to get a gcbits_t&. GCBits (and GCBitsInSafepoint) could call GetGCBitsForModify to remove the duplication of the two-line body. Is this what you mean?

Ah, no, that's just me skimming code and missing the 'const'. We could also refactor as you suggest, but I'm not sure that's a big deal for a two-liner exactly.

Comment on attachment 515825 [details] [diff] [review] MQ rev 250 Some nits/suggestions. * Could we compile the bit-twiddling logic unconditionally, and make only the atomic ops conditional on MMGC_THREADSAFE_GC? It's not like the memory bus can address individual bytes, it's a convenient C and/or CPU illusion, but since we're going to carry the maintenance cost of the bit twiddling code anyway, we might want to control the slide toward #ifdef hell. * Somewhat in the same vein, seems unlikely that we could call GetGCBits() while not in a safepoint task or not deleting the GC object, and not need lengthy and brittle argumentation as to why the call is safe. If this is the case, then maybe GetGCBitsInSafepoint() wouldn't be necessary, and the safety assert could move inside GetGCBits(), conditional on MMGC_THREADSAFE_GC.* How about a locally reusable macro that handles recurring logic of this form: #ifdef VMCFG_LITTLE_ENDIAN uint32_t mask = kHasWeakRef << (((intptr_t)bits & 3) << 3); #else uint32_t mask = kHasWeakRef << ((3 - ((intptr_t)bits & 3)) << 3); #endif * the ForModify suffix sounds weird to me. I suspect the pattern of two accessors, offering access to either an immutable or a mutable view of state, may recur in the future, and may be worth standardizing. getFoo vs getMutableFoo ? * in GC::IsMarkedThenMakeQueued(): we're assured consistency. -> we have assured consistency? we are assuring consistency?

(In reply to comment #7) > Comment on attachment 515825 [details] [diff] [review] > MQ rev 250 > > Some nits/suggestions. > > * Could we compile the bit-twiddling logic unconditionally, and make only the > atomic ops conditional on MMGC_THREADSAFE_GC? It's not like the memory bus can > address individual bytes, it's a convenient C and/or CPU illusion, but since > we're going to carry the maintenance cost of the bit twiddling code anyway, we > might want to control the slide toward #ifdef hell. I think at least #ifdef purgatory is unavoidable for a while. > * Somewhat in the same vein, seems unlikely that we could call GetGCBits() > while not in a safepoint task or not deleting the GC object, and not need > lengthy and brittle argumentation as to why the call is safe. If this is the > case, then maybe GetGCBitsInSafepoint() wouldn't be necessary, and the safety > assert could move inside GetGCBits(), conditional on MMGC_THREADSAFE_GC. GetGCBits() is called in many places to just inspect the bits, hence it returns a const, and you have to use GetGCBitsInSafepoint() or GetGCBitsForModify() for a mutable reference. > * How > about a locally reusable macro that handles recurring logic of this form: > > > > #ifdef VMCFG_LITTLE_ENDIAN > uint32_t mask = kHasWeakRef << (((intptr_t)bits & 3) << 3); > #else > uint32_t mask = kHasWeakRef << ((3 - ((intptr_t)bits & 3)) << 3); > #endif Will do. > > * the ForModify suffix sounds weird to me. I suspect the pattern of two > accessors, offering access to either an immutable or a mutable view of state, > may recur in the future, and may be worth standardizing. getFoo vs > getMutableFoo ? I'm easy. GetGCBits() was an existing function that returned a non-const reference. The ForModify suffix was initially for my own sanity, but the name has stuck. FWIW, 'InSafepoint' can probably be renamed too, it's somewhat inconsistent with the latest terminology from the safepoint docs. > * in GC::IsMarkedThenMakeQueued(): we're assured consistency. -> we have > assured consistency? we are assuring consistency? The sentence that comes from is redundant. I'll just remove it.

Comment on attachment 515825 [details] [diff] [review] MQ rev 250 The !FASTBITS case will go, but it doesn't have to go yet so let's not worry about it here. For my money, the GetGCBits / GetGCBitsForModify distinction is useful (the former need not return a reference obviously) and does not have to be for threadsafe-only code. However I don't know if it changes the amount of #ifdeffery much to introduce it generally - your call. While it's clear that GetGCBitsInSafepoint is a threadsafe-only mechanism I could live with this being used generally in the GC code without #ifdeffery, as it would only alias GetGCBitsForModify in the non-threadsafe case. Making this change unconditionally would probably reduce the amount of #ifdefs enough to matter. We clearly want some macros or inline functions for the shifting and masking for the thread-safe bit setting. Doing so would take pressure off refactoring ClearFinalized and so on.

General tidy-up and macro-ization of bit shifting and masking. The macros could do with some docs. I'll add these in the next rev.

Is the patch complete for landing review?

(In reply to comment #11) > Is the patch complete for landing review? Not yet. I'd like Felix to review the macro-ization of the bit shifting and masking I've done since the last review.

Haven't done the actual review that Simon requested, but this stray tab stuck out and needs to be fixed: + const uint32_t markMask = ~GCBIT_ALIGN32(kMark, bits); + uint32_t oldValue, newValue; + volatile int32_t* const bits32 = (volatile int32_t*)GCBITS_PTR_ALIGN32(bits);

(I think Bugzilla auto-converted the tab I tried to post in comment 13. You can see if if you search in the patch.)

(In reply to comment #12) > (In reply to comment #11) > > Is the patch complete for landing review? > > Not yet. I'd like Felix to review the macro-ization of the bit shifting and > masking I've done since the last review. The macro-ization looks correct to me. I'm the kind of person that has to re-derive what's going on every time I see such things (e.g. the literal 3 has distinct meanings in several separate occurrences, and that always trips me up), so I wouldn't object to an explanatory single-line comment if you can come up with a suitable one. But it shouldn't hold up landing this. Related note: are we going to be running into similarly unaligned addresses in other contexts? If so, should vmbase be extended with and32WithBarrierUnalignedAddress, or something similar...? (Its a throw-away thought; again not really necessary to address that comment in order to land this patch.)

There are several macros similar to this one: #define GCBIT_ALIGN32(_bit_, _addr_) (_bit_ << (((intptr_t)_addr_ & 3) << 3)) The arguments should be parenthesized wherever they are used to prevent unintended reassociation of operators if a nontrivial expression is passed in.

(In reply to comment #15) > (In reply to comment #12) > > (In reply to comment #11) > > > Is the patch complete for landing review? > > > > Not yet. I'd like Felix to review the macro-ization of the bit shifting and > > masking I've done since the last review. > > The macro-ization looks correct to me. I'm the kind of person that has to > re-derive what's going on every time I see such things (e.g. the literal 3 has > distinct meanings in several separate occurrences, and that always trips me > up), so I wouldn't object to an explanatory single-line comment if you can come > up with a suitable one. But it shouldn't hold up landing this. Brief comment added. > Related note: are we going to be running into similarly unaligned addresses in > other contexts? If so, should vmbase be extended with > and32WithBarrierUnalignedAddress, or something similar...? (Its a throw-away > thought; again not really necessary to address that comment in order to land > this patch.) Extending it with something like and8WithBarrier and16.. etc could be useful.

Retargeting to Dolores.