RFR (7xS): 8177707: Specialize G1RemSet::refine_card for concurrent/during safepoint refinement

[Thomas Schatzl]

Hi Kim,

  thanks for your review.

On Thu, 2017-04-20 at 23:29 -0400, Kim Barrett wrote:
> > 
> > On Apr 11, 2017, at 7:30 AM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> 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.
> > 
> > […]
> > 8177707: Specialize G1RemSet::refine_card for concurrent/during
> > safepoint refinement
> > 
> > CR: https://bugs.openjdk.java.net/browse/JDK-8177707
> > Webrev: http://cr.openjdk.java.net/~tschatzl/8177707/webrev/
> > 
> > This change temporarily duplicates the code in
> > G1RemSet::refine_card
> > for during refinement/gc to allow (initial) specialization of these
> > methods for their purpose.
> -------------------------------------------------------------------
> ----------- 
> src/share/vm/gc/g1/g1RemSet.cpp
>  588   // While we are processing RSet buffers during the collection,
> we
> ...
>  598   assert(!r->in_collection_set(), "Concurrent refinement should
> not encounter cards in the collection set.");
> 
> The comment starting at line 588 seems like it needs updating to
> account for line 598 being changed from an "if" to an "assert".
> 
> However, I'm not sure this change change to an assert is correct,
> because of the possibility of stale cards.

Fixed. Thanks for pointing this out, you are right.

> 
> -------------------------------------------------------------------
> ----------- 
> src/share/vm/gc/g1/g1RemSet.cpp 
>  636   HeapWord* scan_limit = r->top();
> 
> I think the commentary about this initialization is important and
> ought not be deleted.
> 
> Similarly for the use of scan_top() in the gc_active case.

Fixed.

> 
> -------------------------------------------------------------------
> -----------
> src/share/vm/gc/g1/g1RemSet.cpp 
>  688 bool G1RemSet::refine_card_during_gc(jbyte* card_ptr,
> 
> I sure hope a lot of the near duplication with the concurrent version
> is going to go away in one of the later changes in this stack.

Actually the next change in this stack cleans this up to a large degree
(8177044: Remove _scan_top from HeapRegion), but also others are going
to reduce the code and code duplication in refine_card_during_gc().

> -------------------------------------------------------------------
> -----------  
> src/share/vm/gc/g1/g1RemSet.cpp 
>  777   _conc_refine_cards++;
> 
> Since this is not in "concurrent" refinement, is it still correct to
> include it in this count?  Maybe the variable name is not right?  If
> so, I'd be okay with a variable rename in a later change if that
> makes management of this stack of changes easier.

The variable name has actually never been "right". I created JDK-
8179677 to avoid disturbing the change stack. I will post an RFR for
that later.

> 
> -------------------------------------------------------------------
> -----------  
> src/share/vm/gc/g1/g1OopClosures.inline.hpp
>  114 inline static void check_obj_during_refinement(T* p, oop const
> obj) {
> 
> Just "inline" is sufficient, no "static" needed.
> 
> -------------------------------------------------------------------
> ----------- 

Fixed.

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

Thanks,
  Thomas