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

[Kim Barrett]

> 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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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 ...;
  }

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------