From c03925edd37b504346c11656a6de5f5e4b791061 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Mon, 9 May 2016 15:12:07 -0400 Subject: [PATCH] runtime: remove unnecessary atomics from heapBitSetType These used to be necessary when racing with updates to the mark bit, but since the mark bit is no longer in the bitmap and the checkmark is only updated with the world stopped, we can now always use regular writes to update the type information in the heap bitmap. Somewhat surprisingly, this has basically no overall performance effect beyond the usual noise, but it does clean up the code. Change-Id: I3933d0b4c0bc1c9bcf6313613515c0b496212105 Reviewed-on: https://go-review.googlesource.com/29277 Run-TryBot: Austin Clements TryBot-Result: Gobot Gobot Reviewed-by: Rick Hudson --- src/runtime/mbitmap.go | 85 ++++++++++++------------------------------ 1 file changed, 23 insertions(+), 62 deletions(-) diff --git a/src/runtime/mbitmap.go b/src/runtime/mbitmap.go index f5b10f3545..0a8e749a08 100644 --- a/src/runtime/mbitmap.go +++ b/src/runtime/mbitmap.go @@ -861,9 +861,6 @@ func (s *mspan) countFree() int { // bits that belong to neighboring objects. Also, on weakly-ordered // machines, callers must execute a store/store (publication) barrier // between calling this function and making the object reachable. -// -// TODO: This still has atomic accesses left over from when it could -// race with GC accessing mark bits in the bitmap. Remove these. func heapBitsSetType(x, size, dataSize uintptr, typ *_type) { const doubleCheck = false // slow but helpful; enable to test modifications to this code @@ -877,11 +874,7 @@ func heapBitsSetType(x, size, dataSize uintptr, typ *_type) { if sys.PtrSize == 8 && size == sys.PtrSize { // It's one word and it has pointers, it must be a pointer. - // In general we'd need an atomic update here if the - // concurrent GC were marking objects in this span, - // because each bitmap byte describes 3 other objects - // in addition to the one being allocated. - // However, since all allocated one-word objects are pointers + // Since all allocated one-word objects are pointers // (non-pointers are aggregated into tinySize allocations), // initSpan sets the pointer bits for us. Nothing to do here. if doubleCheck { @@ -900,7 +893,7 @@ func heapBitsSetType(x, size, dataSize uintptr, typ *_type) { ptrmask := typ.gcdata // start of 1-bit pointer mask (or GC program, handled below) // Heap bitmap bits for 2-word object are only 4 bits, - // so also shared with objects next to it; use atomic updates. + // so also shared with objects next to it. // This is called out as a special case primarily for 32-bit systems, // so that on 32-bit systems the code below can assume all objects // are 4-word aligned (because they're all 16-byte aligned). @@ -917,20 +910,11 @@ func heapBitsSetType(x, size, dataSize uintptr, typ *_type) { if sys.PtrSize == 4 && dataSize == sys.PtrSize { // 1 pointer object. On 32-bit machines clear the bit for the // unused second word. - if gcphase == _GCoff { - *h.bitp &^= (bitPointer | bitScan | ((bitPointer | bitScan) << heapBitsShift)) << h.shift - *h.bitp |= (bitPointer | bitScan) << h.shift - } else { - atomic.And8(h.bitp, ^uint8((bitPointer|bitScan|((bitPointer|bitScan)<= nw { // We know that there is more data, because we handled 2-word objects above. @@ -1299,14 +1264,10 @@ Phase3: // If w == nw+4 then there's nothing left to do: we wrote all nw entries // and can discard the 4 sitting in hb. // But if w == nw+2, we need to write first two in hb. - // The byte is shared with the next object so we may need an atomic. + // The byte is shared with the next object, so be careful with + // existing bits. if w == nw+2 { - if gcphase == _GCoff { - *hbitp = *hbitp&^(bitPointer|bitScan|(bitPointer|bitScan)<