RFR(S/M): 8001985: G1: Backport fix for 7200261 to hsx24
Bengt Rutisson
bengt.rutisson at oracle.com
Wed Oct 31 08:38:07 UTC 2012
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
More information about the hotspot-gc-dev
mailing list