RFR (S): 8035326: Assume non-NULL references in G1CollectedHeap::in_cset_fast_test

[Mikael Gerdin]

On Thursday 20 February 2014 14.58.01 Thomas Schatzl wrote:
> Hi,
> 
> On Thu, 2014-02-20 at 13:06 +0100, Mikael Gerdin wrote:
> > Thomas,
> > 
> > On Thursday 20 February 2014 11.17.39 Thomas Schatzl wrote:
> > > Hi Jon,
> > > 
> > >   thanks for the review.
> > > 
> > > On Wed, 2014-02-19 at 17:18 -0800, Jon Masamitsu wrote:
> > > > Thomas,
> > > > 
> > > > Changes look good.
> > > > 
> > > > Is there really nothing you would put into the assert string?
> > > > 
> > > > http://cr.openjdk.java.net/~tschatzl/8035326/webrev/src/share/vm/gc_im
> > > > plem
> > > > entation/g1/g1CollectedHeap.hpp.udiff.html
> > > > 
> > > > +    assert(_g1_committed.contains((HeapWord*) obj), "");
> > > 
> > > Fixed in http://cr.openjdk.java.net/~tschatzl/8035326/webrev.1 along
> > > with other suggestions from Stefan.
> > 
> > in g1CollectedHeap.cpp
> > 
> > 4792   T heap_oop = oopDesc::load_heap_oop(p);
> > 4793
> > 4794   // Filter out all NULL references up front avoiding checking this
> > again 4795   // over and over.
> > 4796   if (oopDesc::is_null(heap_oop)) {
> > 4797     return;
> > 4798   }
> > 4799
> > 4800   oop obj = oopDesc::load_decode_heap_oop_not_null(p);
> > 
> > Why do you dereference p again after the null check? You could just have:
> > oop obj = oopDesc::decode_heap_oop_not_null(heap_oop)
> 
> Thanks for catching this. Bug from splitting.
> 
> Also removed the comment as per suggestion from Stefan.
> 
> New webrev: http://cr.openjdk.java.net/~tschatzl/8035326/webrev.2

Ship it!
/Mikael

> 
> Thanks,
>   Thomas