RFR(S/M): 8001985: G1: Backport fix for 7200261 to hsx24

Thu Nov 1 17:07:43 UTC 2012

Hi Bengt,

Thanks for the review. I'm not sure what caused the conflict. The old 
code in the rejected hunks came in as part of:

changeset:   3295:8a2e5a6a19a4
user:        johnc
date:        Wed Apr 25 10:23:12 2012 -0700
summary:     7143490: G1: Remove HeapRegion::_top_at_conc_mark_count

and hg annotate doesn't list r3599 anywhere near the code.

As for the range checking - the liveness card bitmaps only cover the G1 
heap (not the perm gen) but we used inclusive ranges (using the last 
address in an object, region, etc rather than the end address). The only 
place the previous code ran into a problem was trying to set the bit for 
the card that was spanned by NTAMS and so we had some range checking 
there to eliminate the asserts and guarantees in the BitMap class.

Now with using exclusive ranges we have to do the range checking even if 
the Perm Gen is there (above the G1 heap) because the liveness card 
bitmaps are sized only for the G1 heap - otherwise we could see the out 
of range asserts and errors. So IMO the range checking in this code 
should be the same in hs24 and hs25 since we are using an exclusive 
range of cards.

IIRC at least one of the out-of-range bugs, fixed by Jon, the fix was to 
obtain the card for a 'last' address rather than the 'end' address.

Thanks,

JohnC

On 10/31/2012 1:38 AM, Bengt Rutisson wrote:
>
> Hi John,
>
> I think this looks good.
>
> The conflict seems to be due to some changes introduced by the perm 
> gen removal push:
>
> changeset:   3599:da91efe96a93
> user:        coleenp
> date:        Sat Sep 01 13:25:18 2012 -0400
> summary:     6964458: Reimplement class meta-data storage to use 
> native memory
>
> As far as I can tell your new code does about the same range checking 
> that the perm gen fix introduced. Have you verified that? Stefan told 
> me that this was probably an issue that perm gen had to fix since we 
> could sometimes get bitmap indices that pointed outside the bitmap. 
> This was benign as long as they still pointed in to the perm gen 
> bitmap. But once the perm gen bitmap was gone it had to be fixed.
>
> In hs24 there is still a perm gen bitmap, so I think we are safe. It 
> also looks to me like you also have the correct handling of the 
> indices with the new code - so thumbs up!
>
> If you want to dig deeper in to this I think Jon Masamitsu might know 
> more.
>
> Bengt
>
> src/share/vm/gc_implementation/g1/concurrentMark.cpp
>
> On 2012-10-30 19:59, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> Can I have a couple of volunteers review the changes for this CR? The 
>> webrev can be found at: 
>> http://cr.openjdk.java.net/~johnc/8001985/webrev.0/
>>
>> Summary:
>> A patch containing the changes for 7200261, generated by hg export, 
>> did not import cleanly into hsx24 sources - the following two hunks 
>> in concurrentMark.cpp:
>>
>> @@ -1188,29 +1188,14 @@
>>  // liveness counting data.
>>  class CMCountDataClosureBase: public HeapRegionClosure {
>>  protected:
>> +  G1CollectedHeap* _g1h;
>>    ConcurrentMark* _cm;
>> +  CardTableModRefBS* _ct_bs;
>> +
>>    BitMap* _region_bm;
>>    BitMap* _card_bm;
>>
>> -  void set_card_bitmap_range(BitMap::idx_t start_idx, BitMap::idx_t 
>> last_idx) {
>> -    assert(start_idx <= last_idx, "sanity");
>> -
>> -    // Set the inclusive bit range [start_idx, last_idx].
>> -    // For small ranges (up to 8 cards) use a simple loop; otherwise
>> -    // use par_at_put_range.
>> -    if ((last_idx - start_idx) < 8) {
>> -      for (BitMap::idx_t i = start_idx; i <= last_idx; i += 1) {
>> -        _card_bm->par_set_bit(i);
>> -      }
>> -    } else {
>> -      assert(last_idx < _card_bm->size(), "sanity");
>> -      // Note BitMap::par_at_put_range() is exclusive.
>> -      BitMap::idx_t max_idx = MAX2(last_idx+1, _card_bm->size());
>> -      _card_bm->par_at_put_range(start_idx, max_idx, true);
>> -    }
>> -  }
>> -
>> -  // It takes a region that's not empty (i.e., it has at least one
>> +  // Takes a region that's not empty (i.e., it has at least one
>>    // live object in it and sets its corresponding bit on the region
>>    // bitmap to 1. If the region is "starts humongous" it will also set
>>    // to 1 the bits on the region bitmap that correspond to its
>>
>> @@ -1548,24 +1555,29 @@
>>      if (ntams < top) {
>>        // This definitely means the region has live objects.
>>        set_bit_for_region(hr);
>> -    }
>> -
>> -    // Now set the bits for [ntams, top]
>> -    BitMap::idx_t start_idx = _cm->card_bitmap_index_for(ntams);
>> -    // set_card_bitmap_range() expects the last_idx to be with
>> -    // the range of the bit map (see assertion in 
>> set_card_bitmap_range()),
>> -    // so limit it to that range with this application of MIN2.
>> -    BitMap::idx_t last_idx = MIN2(_cm->card_bitmap_index_for(top),
>> -                                  _card_bm->size()-1);
>> -    if (start_idx < _card_bm->size()) {
>> -    set_card_bitmap_range(start_idx, last_idx);
>> -    } else {
>> -      // To reach here start_idx must be beyond the end of
>> -      // the bit map and last_idx must have been limited by
>> -      // the MIN2().
>> -      assert(start_idx == last_idx + 1,
>> -        err_msg("Not beyond end start_idx " SIZE_FORMAT " last_idx "
>> -                SIZE_FORMAT, start_idx, last_idx));
>> +
>> +      // Now set the bits in the card bitmap for [ntams, top)
>> +      BitMap::idx_t start_idx = _cm->card_bitmap_index_for(ntams);
>> +      BitMap::idx_t end_idx = _cm->card_bitmap_index_for(top);
>> +
>> +      // Note: if we're looking at the last region in heap - top
>> +      // could be actually just beyond the end of the heap; end_idx
>> +      // will then correspond to a (non-existent) card that is also
>> +      // just beyond the heap.
>> +      if (_g1h->is_in_g1_reserved(top) && 
>> !_ct_bs->is_card_aligned(top)) {
>> +        // end of object is not card aligned - increment to cover
>> +        // all the cards spanned by the object
>> +        end_idx += 1;
>> +      }
>> +
>> +      assert(end_idx <= _card_bm->size(),
>> +             err_msg("oob: end_idx=  "SIZE_FORMAT", bitmap size= 
>> "SIZE_FORMAT,
>> +                     end_idx, _card_bm->size()));
>> +      assert(start_idx < _card_bm->size(),
>> +             err_msg("oob: start_idx=  "SIZE_FORMAT", bitmap size= 
>> "SIZE_FORMAT,
>> +                     start_idx, _card_bm->size()));
>> +
>> +      _cm->set_card_bitmap_range(_card_bm, start_idx, end_idx, true 
>> /* is_par */);
>>      }
>>
>>      // Set the bit for the region if it contains live data
>>
>> did not apply cleanly. These had to be fixed up by hand. The original 
>> webrev for 7200261 can be found at: 
>> http://cr.openjdk.java.net/~johnc/7200261/webrev.1 and was originally 
>> reviewed by Bengt, Jesper, and Jon Masamitsu.
>>
>> Thanks,
>>
>> JohnC
>