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

[Jesper Wilhelmsson]

Hi John,

Looks good to me.
/Jesper


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: jesper_wilhelmsson.vcf
Type: text/x-vcard
Size: 263 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20121030/40b16d8f/jesper_wilhelmsson.vcf>