RFR (S): 8069760: When iterating over a card, G1 often iterates over much more references than are contained in the card

[Thomas Schatzl]

Hi Kim,

  thanks for the review and thanks for finding these issues.

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

It passed another jprt run.

More comments below.

On Tue, 2015-01-27 at 18:27 -0500, Kim Barrett wrote:
> On Jan 27, 2015, at 8:21 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8069760
> > 
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8069760/webrev/
> > 
> > Testing:
> > nosql, dev-submit performance testing (specj*), vm.quick.testlist, jprt.
> 
> ------------------------------------------------------------------------------ 
> src/share/vm/gc_implementation/g1/heapRegion.cpp 
>  443     assert(obj == oop(cur), "Loop invariant");
> 
> This and later references to "obj" in the first iteration depend on
> the assignment of "obj" in the loop back at
>  425     obj = oop(cur);
> 
> It required some careful study to conclude that "obj" is guaranteed to
> be initialized and have the proper value at line 443.  I think it
> would be better to move the update of "obj" from the end of the loop
> (line 463) to the head of the loop (after line 440).  Of course, then
> eliminate the unnecessary assert(obj == oop(cur), ...).

Fixed.

> 
> ------------------------------------------------------------------------------ 
> src/share/vm/gc_implementation/g1/heapRegion.cpp 
>  453       // Non-object arrays are sometimes marked imprecise at the object start. We
> 
> I think "Non-object arrays" would be clearer as "Non-objArrays".  At
> least, I think that's the intent.  As written I read it as arrays of
> non-objects, e.g. primitive arrays such as int[].
> 

Fixed.

> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/g1/heapRegion.cpp 
>  440   do {
>  441     assert(obj->klass_or_null() != NULL, "Loop invariant");
> ...
>  444     if (obj->klass_or_null() == NULL) {
> 
> Line 444 can never be true if the assertion at line 441 is correct.
> 
> The assertion block from line 441-443 used to be before the loop, and
> has now been hoisted into the loop.  I'm not sure that's correct, and
> this inconsistency is a large part of what makes that move look
> suspect.  Or maybe it should be moved after the
>   if (obj->klass_or_null() == NULL)
> check?  But then we might be missing the first iteration assertion
> that the klass is != NULL.

Moved the assert outside of the loop, although I think it is
superfluous.

> 
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/g1/heapRegion.cpp 
>  678 //      gclog_or_tty->print_cr("Verifying address " PTR_FORMAT, p2i(p));
> 
> Leftover debug print statement.
> 

Fixed.

Thanks,
  Thomas