RFR (7xS): 8071280: Specialize HeapRegion::oops_on_card_seq_iterate_careful() for use during concurrent refinement and updating the rset

[Thomas Schatzl]

Hi Kim, Sangheon,

On Fri, 2017-05-05 at 10:05 -0700, sangheon wrote:
> Hi Thomas,
> 
> On 05/05/2017 05:03 AM, Thomas Schatzl wrote:
[...]
> > > > 8071280: Specialize
> > > > HeapRegion::oops_on_card_seq_iterate_careful()
> > > > for use during concurrent refinement and updating the rset
> > > > 
> > > > CR: https://bugs.openjdk.java.net/browse/JDK-8071280
> > > > Webrev: http://cr.openjdk.java.net/~tschatzl/8071280/webrev
> > > > 
> > > > As the title indicates, this change allows specializations of
> > > > HeapRegion::oops_on_card_seq_iterate_careful() according to
> > > > current GC phase (concurrent or during gc) to allow to remove
> > > > some checks.
> > > > This is mostly a preparatory change for the next ones.
> > > 8071280, seems good to me.
> > > 
> > > src/share/vm/gc/g1/heapRegion.inline.hpp
> > >    149   assert(ClassUnloadingWithConcurrentMark,
> > > 
> > > I have same question as Kim about this assertion.
> > > In addition, block_size_during_gc() and block_size() are very
> > > similar except block_size() could return earlier.
> > > Could you give me some explanation for this?
> > As mentioned in Kim's reply: "If ClassUnloadingWithConcurrentMark
> > is false, this method will not be called, and this is what the
> > assert checks. HeapRegion::is_obj_dead_with_size() then always uses
> > the obj-size() path."
> > 
[...]
> > 
> > Looking at it now (it has been a long way to get to this version),
> > maybe block_size() could just call block_size_during_gc() at the
> > end?
> Doesn't hurt your goal with above idea?
> Current version seems good to me but if you are considering a change 
> adding inline function with common stuffs could be one of idea.

  I revisited this again, and factored out the common parts into a new
method HeapRegion::block_size_using_bitmap().

While doing so I also did reruns of tier1-5 testing with and without
ClassUnloadingWithConcurrentRemark and found that my suggestion that
block_size_during_gc() is never called is wrong, so some asserts are
failing, although the method is safe to call, just unnecessarily slow.

Instead of just removing the asserts, and penalizing the case when
class unloading is disabled (as likely or unlikely it is), I did some
tests with protecting the call to block_size_during_gc() with an
additional ClassUnloadingWithConcurrentMark clause. Perf testing showed
no difference (with class unloading enabled), so I kept it.

e.g. in HeapRegion::is_obj_dead_with_size():


   assert(!is_archive(), "Archive regions should not have references
into interesting regions.");
   assert(!is_humongous(), "Humongous objects not handled here");
   bool result =
     !obj_allocated_since_prev_marking(obj) &&
     !bitmap->isMarked((HeapWord*)obj);
-  if (result) {
+  if (ClassUnloadingWithConcurrentMark && result) {
     *size = is_gc_active ? block_size_during_gc((HeapWord*)obj,
bitmap)
                          : block_size((HeapWord*)obj);
   } else {
+    assert(block_is_obj((HeapWord*)obj), "must be");
     *size = obj->size();
   }
   return result;
 }

I also fixed some comments.

Webrevs:
http://cr.openjdk.java.net/~tschatzl/8071280/webrev.1_to_2/ (diff)
http://cr.openjdk.java.net/~tschatzl/8071280/webrev.2/ (full)

Testing:
jprt, gc/common tier 1-5 tests with and without
ClassUnloadingWithConcurrentMark

Thanks,
  Thomas