RFR (M): 8224741: Optimize the scan area during the Scan Heap Roots phase

[Thomas Schatzl]

Hi Kim,

  thanks for your review.

On Mon, 2019-07-01 at 18:31 -0400, Kim Barrett wrote:
> > On May 24, 2019, at 11:31 AM, Thomas Schatzl <
> > thomas.schatzl at oracle.com> wrote:
> > 
> > Hi all,
> > 
> >  can I have reviews for this small change based on JDK-8213108 (out
> > for review right now) that optimizes the scanning of blocks within
> > a chunk (see comment of g1RemSet.cpp introduced in JDK-8213108) a
> > bit?
> > 
> > […]
> > The only reason this is a separate CR is because I thought that the
> > change in JDK-8213108 is large enough already as is, and easier to
> > understand without.
> 
> Thank you for that :)
> 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8224741
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8224741/webrev/
> > Testing:
> > hs-tier1-5, local jtreg gc and gc/g1 testing, perf testing
> > 
> > Thanks,
> >  Thomas
> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/heapRegion.hpp
>  296   // Returns the address after the last actually scanned.
> 
> This doesn't mention the possible NULL result when the iteration
> couldn't be done (only in the concurrent case).  Of course, the
> previous comment didn't describe the return value at all.

Fixed.

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1RemSet.cpp
>  923     assert(scanned_to >= mr.end(), "Scanned to " PTR_FORMAT "
> less than range " PTR_FORMAT, p2i(scanned_to), p2i(mr.end()));
> 
> I think this assertion isn't correct.
> 
> Suppose we have a humongous non-objarray object that fits in a single
> region.  And suppose the range of dirty cards covers the entire
> object, and beyond (because the object's size is not a multiple of
> card size).  The start of the scanned region includes (is) the start
> of the object, and the end of the scan region is beyond the end of
> the
> object.  The scan of the region will hit the clause in
> do_oops_on_card_in_humongous that (with this change) returns obj end,
> which is less than mr.end(), tripping the assertion.
> 
> But see next comment.
> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1RemSet.cpp
>  296       return (HeapWord*)obj + size;
> 
> Maybe return the greator of end of object and end of region?
> Similarly to the dead case, there can't be other objects in this
> region, and we've scanned the entire object.
> 
> Doing so would also address the assert issue above.

Fixed it the suggested way.

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1RemSet.cpp 
>  260 HeapWord* HeapRegion::do_oops_on_card_in_humongous(MemRegion mr,
> ...
>  281   if (!g1h->is_obj_dead(obj, sr)) {
> ...
>  298   }
>  299   // The object is dead. There can be no other object in this
> region, so return
>  300   // the end of that region.
>  301   return end();
> 
> The structure here has become somewhat confusing with the return
> value change.  It used to be that we always returned true at the end,
> once we reached the is-dead check.  But now we return different
> values in every clause.  I think it would be easier to read if the
> control structure were flattened out to something like
> 
>   if (g1h->is_obj_dead(obj, sr)) {
>     ...
>     return ...;
>   } else if (obj->is_objArray() || (sr->bottom() < mr.start())) {
>     ...
>     return ...;
>   } else {
>     ...
>     return ...;
>   }

Done.

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1RemSet.cpp 
>  349     bool is_imprecise = false;
> 
> I think the variable name is backward, and should be is_precise.

:(

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1RemSet.cpp 
>  367       return is_imprecise ? mr.end() : cur;
> 
> s/mr.end()/end/
> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/heapRegion.hpp
>  648   // Iterate over the objects overlapping part of a card,
> applying cl
> 
> [pre-existing]
> 
> Before JDK-8213108, oops_on_card_seq_iterate_careful and its helpers
> were applied to a MemRegion that covered no more than a single card.
> 8213108 changed that, now possibly applying the iteration to a
> MemRegion intersecting multiple cards.  But comments for the iterate
> functions haven't been updated.
> 

Fixed.

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


testing:
hs-tier1-3 almost done without issues

Thanks,
  Thomas