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,


On Thu, 2015-01-29 at 17:17 -0500, Kim Barrett wrote: 
> 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)
> 
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/g1/heapRegion.cpp 
>  456       if (!obj->is_objArray() || (((HeapWord*)obj) >= start && cur < end)) {
> 
> Should "cur < end" be "cur <= end"?
> 
> ------------------------------------------------------------------------------
> 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.
> [...]
> 
> ------------------------------------------------------------------------------ 
> 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.

  all fixed. I kept the asserts.

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

Testing: jprt

Thanks for your extensive review.

Thomas