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

Thu Nov 1 20:42:55 UTC 2012

Hi John,

Thanks for the extra explanation on this! I think it sounds fine.

Ship it!
Bengt

On 2012-11-01 18:07, John Cuthbertson wrote:
> 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
>>
>