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

[Thomas Schatzl]

Hi Kim,

  thanks for your review.

On Wed, 2017-04-19 at 20:06 -0400, Kim Barrett wrote:
> > 
> > On Apr 11, 2017, at 7:30 AM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> 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.
> Only this changeset.  I’ll be sending out comments on others later,
> separate messages for each.
> 
> -------------------------------------------------------------------
> ----------- 
> src/share/vm/gc/g1/heapRegion.inline.hpp
>  149   assert(ClassUnloadingWithConcurrentMark,
> 
> Is this assertion really correct?  There is a similar assertion in
> block_size, but it is protected by a call to block_is_obj.  I'm not
> seeing similar protection for this one.  Maybe I'm missing someting?

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.

> -------------------------------------------------------------------
> -----------
> src/share/vm/gc/g1/g1RemSet.cpp
>  728   }
> 
> Close brace is now mis-indented.

Fixed.

> -------------------------------------------------------------------
> ----------- 
> src/share/vm/gc/g1/heapRegion.hpp
>  329   template <class Closure, bool is_gc_active>
>  330   inline bool do_oops_on_card_in_humongous(MemRegion mr,
> 
> and
> 
>  682   template <class Closure, bool is_gc_active>
>  683   inline bool oops_on_card_seq_iterate_careful(MemRegion mr,
> Closure* cl);
> 
> It would be better to have the is_gc_active template parameter first.
> That would allow the Closure parameter to be deduced in calls, e.g.
> instead of
> 
>     card_processed = r-
> >oops_on_card_seq_iterate_careful<G1UpdateRSOrPushRefOopClosure,
> true>(dirty_region, &update_rs_oop_cl);
> 
> one would use
> 
>     card_processed = r-
> >oops_on_card_seq_iterate_careful<true>(dirty_region,
> &update_rs_oop_cl);
> 
> with Closure being deduced from the type of the argument in the call.

Fixed.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8071280/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8071280/webrev.1/ (full)

Thanks,
  Thomas