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

[Thomas Schatzl]

Hi Sangheon,

  thanks for your review.

On Tue, 2017-05-02 at 15:07 -0700, sangheon wrote:
> Hi Thomas,
> 
> On 04/11/2017 04:30 AM, Thomas Schatzl wrote:
> > 
> > Hi all,
> > 
> >    I would like you to ask for reviews of a set of related changes
> > that optimize the way G1 updates/scans/refines cards of the
> > remembered set.
> > 
> > 
> > [...]
> > Here we go:
> > 
> > 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."

Block_size_during_gc() is a very aggressive specialization of
block_size(), so of course they ultimately do the same thing. The
difference is that during GC a lot of cases that block_size() needs to
check for always evaluate to a particular value.

The other important optimization that block_size_during_gc() does is
that we pass the bitmap to look at, as the triple indirection (load
G1CollectedHeap*, load G1ConcurrentMark*, load G1CMBitMapR0*) to find
it has shown to be very expensive.

The compiler is unable to see all this.

The reason why the two methods look similar is that
block_size_during_gc() changes all ifs into asserts checking that it is
called only in the right circumstances. I.e. that all assumptions that
we can make during GC are actually true.

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?

Thanks,
  Thomas