Wed, 25 Mar 2015 11:03:16 +0100
8065358: Refactor G1s usage of save_marks and reduce related races
Summary: Stop using save_marks in G1 related code and make setting the replacement field less racy.
Reviewed-by: brutisso, tschatzl
1.1 --- a/src/share/vm/gc_implementation/g1/g1Allocator.cpp Tue Mar 24 10:04:10 2015 +0000 1.2 +++ b/src/share/vm/gc_implementation/g1/g1Allocator.cpp Wed Mar 25 11:03:16 2015 +0100 1.3 @@ -59,7 +59,7 @@ 1.4 !(retained_region->top() == retained_region->end()) && 1.5 !retained_region->is_empty() && 1.6 !retained_region->isHumongous()) { 1.7 - retained_region->record_top_and_timestamp(); 1.8 + retained_region->record_timestamp(); 1.9 // The retained region was added to the old region set when it was 1.10 // retired. We have to remove it now, since we don't allow regions 1.11 // we allocate to in the region sets. We'll re-add it later, when 1.12 @@ -94,6 +94,9 @@ 1.13 // want either way so no reason to check explicitly for either 1.14 // condition. 1.15 _retained_old_gc_alloc_region = old_gc_alloc_region(context)->release(); 1.16 + if (_retained_old_gc_alloc_region != NULL) { 1.17 + _retained_old_gc_alloc_region->record_retained_region(); 1.18 + } 1.19 1.20 if (ResizePLAB) { 1.21 _g1h->_survivor_plab_stats.adjust_desired_plab_sz(no_of_gc_workers);
2.1 --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Tue Mar 24 10:04:10 2015 +0000 2.2 +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Wed Mar 25 11:03:16 2015 +0100 2.3 @@ -6808,7 +6808,7 @@ 2.4 // We really only need to do this for old regions given that we 2.5 // should never scan survivors. But it doesn't hurt to do it 2.6 // for survivors too. 2.7 - new_alloc_region->record_top_and_timestamp(); 2.8 + new_alloc_region->record_timestamp(); 2.9 if (survivor) { 2.10 new_alloc_region->set_survivor(); 2.11 _hr_printer.alloc(new_alloc_region, G1HRPrinter::Survivor);
3.1 --- a/src/share/vm/gc_implementation/g1/g1RemSet.cpp Tue Mar 24 10:04:10 2015 +0000 3.2 +++ b/src/share/vm/gc_implementation/g1/g1RemSet.cpp Wed Mar 25 11:03:16 2015 +0100 3.3 @@ -147,11 +147,9 @@ 3.4 3.5 // Set the "from" region in the closure. 3.6 _oc->set_region(r); 3.7 - HeapWord* card_start = _bot_shared->address_for_index(index); 3.8 - HeapWord* card_end = card_start + G1BlockOffsetSharedArray::N_words; 3.9 - Space *sp = SharedHeap::heap()->space_containing(card_start); 3.10 - MemRegion sm_region = sp->used_region_at_save_marks(); 3.11 - MemRegion mr = sm_region.intersection(MemRegion(card_start,card_end)); 3.12 + MemRegion card_region(_bot_shared->address_for_index(index), G1BlockOffsetSharedArray::N_words); 3.13 + MemRegion pre_gc_allocated(r->bottom(), r->scan_top()); 3.14 + MemRegion mr = pre_gc_allocated.intersection(card_region); 3.15 if (!mr.is_empty() && !_ct_bs->is_card_claimed(index)) { 3.16 // We make the card as "claimed" lazily (so races are possible 3.17 // but they're benign), which reduces the number of duplicate
4.1 --- a/src/share/vm/gc_implementation/g1/heapRegion.cpp Tue Mar 24 10:04:10 2015 +0000 4.2 +++ b/src/share/vm/gc_implementation/g1/heapRegion.cpp Wed Mar 25 11:03:16 2015 +0100 4.3 @@ -338,7 +338,7 @@ 4.4 _orig_end = mr.end(); 4.5 hr_clear(false /*par*/, false /*clear_space*/); 4.6 set_top(bottom()); 4.7 - record_top_and_timestamp(); 4.8 + record_timestamp(); 4.9 } 4.10 4.11 CompactibleSpace* HeapRegion::next_compaction_space() const { 4.12 @@ -426,9 +426,9 @@ 4.13 4.14 // If we're within a stop-world GC, then we might look at a card in a 4.15 // GC alloc region that extends onto a GC LAB, which may not be 4.16 - // parseable. Stop such at the "saved_mark" of the region. 4.17 + // parseable. Stop such at the "scan_top" of the region. 4.18 if (g1h->is_gc_active()) { 4.19 - mr = mr.intersection(used_region_at_save_marks()); 4.20 + mr = mr.intersection(MemRegion(bottom(), scan_top())); 4.21 } else { 4.22 mr = mr.intersection(used_region()); 4.23 } 4.24 @@ -980,7 +980,7 @@ 4.25 4.26 void G1OffsetTableContigSpace::clear(bool mangle_space) { 4.27 set_top(bottom()); 4.28 - set_saved_mark_word(bottom()); 4.29 + _scan_top = bottom(); 4.30 CompactibleSpace::clear(mangle_space); 4.31 reset_bot(); 4.32 } 4.33 @@ -1012,41 +1012,42 @@ 4.34 return _offsets.threshold(); 4.35 } 4.36 4.37 -HeapWord* G1OffsetTableContigSpace::saved_mark_word() const { 4.38 +HeapWord* G1OffsetTableContigSpace::scan_top() const { 4.39 G1CollectedHeap* g1h = G1CollectedHeap::heap(); 4.40 - assert( _gc_time_stamp <= g1h->get_gc_time_stamp(), "invariant" ); 4.41 HeapWord* local_top = top(); 4.42 OrderAccess::loadload(); 4.43 - if (_gc_time_stamp < g1h->get_gc_time_stamp()) { 4.44 + const unsigned local_time_stamp = _gc_time_stamp; 4.45 + assert(local_time_stamp <= g1h->get_gc_time_stamp(), "invariant"); 4.46 + if (local_time_stamp < g1h->get_gc_time_stamp()) { 4.47 return local_top; 4.48 } else { 4.49 - return Space::saved_mark_word(); 4.50 + return _scan_top; 4.51 } 4.52 } 4.53 4.54 -void G1OffsetTableContigSpace::record_top_and_timestamp() { 4.55 +void G1OffsetTableContigSpace::record_timestamp() { 4.56 G1CollectedHeap* g1h = G1CollectedHeap::heap(); 4.57 unsigned curr_gc_time_stamp = g1h->get_gc_time_stamp(); 4.58 4.59 if (_gc_time_stamp < curr_gc_time_stamp) { 4.60 - // The order of these is important, as another thread might be 4.61 - // about to start scanning this region. If it does so after 4.62 - // set_saved_mark and before _gc_time_stamp = ..., then the latter 4.63 - // will be false, and it will pick up top() as the high water mark 4.64 - // of region. If it does so after _gc_time_stamp = ..., then it 4.65 - // will pick up the right saved_mark_word() as the high water mark 4.66 - // of the region. Either way, the behaviour will be correct. 4.67 - Space::set_saved_mark_word(top()); 4.68 - OrderAccess::storestore(); 4.69 + // Setting the time stamp here tells concurrent readers to look at 4.70 + // scan_top to know the maximum allowed address to look at. 4.71 + 4.72 + // scan_top should be bottom for all regions except for the 4.73 + // retained old alloc region which should have scan_top == top 4.74 + HeapWord* st = _scan_top; 4.75 + guarantee(st == _bottom || st == _top, "invariant"); 4.76 + 4.77 _gc_time_stamp = curr_gc_time_stamp; 4.78 - // No need to do another barrier to flush the writes above. If 4.79 - // this is called in parallel with other threads trying to 4.80 - // allocate into the region, the caller should call this while 4.81 - // holding a lock and when the lock is released the writes will be 4.82 - // flushed. 4.83 } 4.84 } 4.85 4.86 +void G1OffsetTableContigSpace::record_retained_region() { 4.87 + // scan_top is the maximum address where it's safe for the next gc to 4.88 + // scan this region. 4.89 + _scan_top = top(); 4.90 +} 4.91 + 4.92 void G1OffsetTableContigSpace::safe_object_iterate(ObjectClosure* blk) { 4.93 object_iterate(blk); 4.94 } 4.95 @@ -1080,6 +1081,8 @@ 4.96 void G1OffsetTableContigSpace::initialize(MemRegion mr, bool clear_space, bool mangle_space) { 4.97 CompactibleSpace::initialize(mr, clear_space, mangle_space); 4.98 _top = bottom(); 4.99 + _scan_top = bottom(); 4.100 + set_saved_mark_word(NULL); 4.101 reset_bot(); 4.102 } 4.103
5.1 --- a/src/share/vm/gc_implementation/g1/heapRegion.hpp Tue Mar 24 10:04:10 2015 +0000 5.2 +++ b/src/share/vm/gc_implementation/g1/heapRegion.hpp Wed Mar 25 11:03:16 2015 +0100 5.3 @@ -101,28 +101,25 @@ 5.4 // OffsetTableContigSpace. If the two versions of BlockOffsetTable could 5.5 // be reconciled, then G1OffsetTableContigSpace could go away. 5.6 5.7 -// The idea behind time stamps is the following. Doing a save_marks on 5.8 -// all regions at every GC pause is time consuming (if I remember 5.9 -// well, 10ms or so). So, we would like to do that only for regions 5.10 -// that are GC alloc regions. To achieve this, we use time 5.11 -// stamps. For every evacuation pause, G1CollectedHeap generates a 5.12 -// unique time stamp (essentially a counter that gets 5.13 -// incremented). Every time we want to call save_marks on a region, 5.14 -// we set the saved_mark_word to top and also copy the current GC 5.15 -// time stamp to the time stamp field of the space. Reading the 5.16 -// saved_mark_word involves checking the time stamp of the 5.17 -// region. If it is the same as the current GC time stamp, then we 5.18 -// can safely read the saved_mark_word field, as it is valid. If the 5.19 -// time stamp of the region is not the same as the current GC time 5.20 -// stamp, then we instead read top, as the saved_mark_word field is 5.21 -// invalid. Time stamps (on the regions and also on the 5.22 -// G1CollectedHeap) are reset at every cleanup (we iterate over 5.23 -// the regions anyway) and at the end of a Full GC. The current scheme 5.24 -// that uses sequential unsigned ints will fail only if we have 4b 5.25 +// The idea behind time stamps is the following. We want to keep track of 5.26 +// the highest address where it's safe to scan objects for each region. 5.27 +// This is only relevant for current GC alloc regions so we keep a time stamp 5.28 +// per region to determine if the region has been allocated during the current 5.29 +// GC or not. If the time stamp is current we report a scan_top value which 5.30 +// was saved at the end of the previous GC for retained alloc regions and which is 5.31 +// equal to the bottom for all other regions. 5.32 +// There is a race between card scanners and allocating gc workers where we must ensure 5.33 +// that card scanners do not read the memory allocated by the gc workers. 5.34 +// In order to enforce that, we must not return a value of _top which is more recent than the 5.35 +// time stamp. This is due to the fact that a region may become a gc alloc region at 5.36 +// some point after we've read the timestamp value as being < the current time stamp. 5.37 +// The time stamps are re-initialized to zero at cleanup and at Full GCs. 5.38 +// The current scheme that uses sequential unsigned ints will fail only if we have 4b 5.39 // evacuation pauses between two cleanups, which is _highly_ unlikely. 5.40 class G1OffsetTableContigSpace: public CompactibleSpace { 5.41 friend class VMStructs; 5.42 HeapWord* _top; 5.43 + HeapWord* volatile _scan_top; 5.44 protected: 5.45 G1BlockOffsetArrayContigSpace _offsets; 5.46 Mutex _par_alloc_lock; 5.47 @@ -166,10 +163,11 @@ 5.48 void set_bottom(HeapWord* value); 5.49 void set_end(HeapWord* value); 5.50 5.51 - virtual HeapWord* saved_mark_word() const; 5.52 - void record_top_and_timestamp(); 5.53 + HeapWord* scan_top() const; 5.54 + void record_timestamp(); 5.55 void reset_gc_time_stamp() { _gc_time_stamp = 0; } 5.56 unsigned get_gc_time_stamp() { return _gc_time_stamp; } 5.57 + void record_retained_region(); 5.58 5.59 // See the comment above in the declaration of _pre_dummy_top for an 5.60 // explanation of what it is. 5.61 @@ -193,6 +191,8 @@ 5.62 virtual HeapWord* allocate(size_t word_size); 5.63 HeapWord* par_allocate(size_t word_size); 5.64 5.65 + HeapWord* saved_mark_word() const { ShouldNotReachHere(); return NULL; } 5.66 + 5.67 // MarkSweep support phase3 5.68 virtual HeapWord* initialize_threshold(); 5.69 virtual HeapWord* cross_threshold(HeapWord* start, HeapWord* end);