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

[Kim Barrett]

On Jan 28, 2015, at 8:00 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> 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.

------------------------------------------------------------------------------
src/share/vm/gc_implementation/g1/heapRegion.cpp 
 456       if (!obj->is_objArray() || (((HeapWord*)obj) >= start && cur < end)) {

Should "cur < end" be "cur <= end"?

Sorry I missed this the first time around.

------------------------------------------------------------------------------
src/share/vm/gc_implementation/g1/heapRegion.cpp 
 438   assert(obj->klass_or_null() != NULL, "Loop invariant");

"invariant" => "post condition" would be more accurate, and would also 
revert the change for this line entirely.

OTOH, I agree it is superfluous, given the preceding loop returns if
that asserted condition is false. But arguably so is the assert on the
previous line; it's not very hard to prove it must be true based on
the preceding code.   I don't think I care one way or the other about 
either of them, though others might.

------------------------------------------------------------------------------ 
src/share/vm/gc_implementation/g1/heapRegion.cpp 
 418   assert(cur <= start, "Postcondition");
 419 
 420   oop obj;
 421 
 422   HeapWord* next = cur;
 423   while (next <= start) {
 424     cur = next;
 425     obj = oop(cur);
 426     if (obj->klass_or_null() == NULL) {
 427       // Ran into an unparseable point.
 428       return cur;
 429     }
 430     // Otherwise...
 431     next = cur + block_size(cur);
 432   }

This code wasn't changed, but perhaps it should be.

I think it would be clearer changed to a do/while with the same
termination condition.  On the first iteration we know the test at 422
is true, because of the assert at line 418.

This would, among other things, make it more obvious that the
reference to "obj" in the assert at line 438 is valid.  Note that I
don't think this changes my mind about whether in the later loop the
"obj oop(cur)" should be at the begining of the loop rather than at
the end.

Sorry I didn't see and comment on this the first time around.

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