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

[Thomas Schatzl]

Hi Stefan,

  thanks for the review.

On Thu, 2014-02-20 at 10:00 +0100, Stefan Karlsson wrote:
> Hi Thomas,
> 
> On 2014-02-19 16:12, Thomas Schatzl wrote:
> > Hi all,
> >
> >    can I get reviews for the following change that lifts the assumption
> > that the oop* passed to G1CollectedHeap::in_cset_fast_test is non-null,
> > allowing manual optimization of code.
> >
> > Almost all uses of G1CollectedHeap::in_cset_fast_test() already made
> > sure that non-null references are passed to it. The only remaining one,
> > in G1ParCopyClosure::do_oop_work() actually benefits from lifting this
> > restriction by allowing removal of some additional checks.
> >
> > Also removes another superfluous check in the same method.
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8035326
> >
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8035326/webrev/
> 
> Looks good to me.
> 
> 4794   if (!oopDesc::is_null(heap_oop)) {
> 
> Did you consider doing an early return instead of adding an extra 
> indentation level to the do_oop_work function?

I did consider an early return, and actually it would be preferable to
me. I looked around in other code and got the impression that this is
not the way it is generally done in the code.

I will happily change this.

> 
> 4828       assert(obj != NULL, "must be");
> 
> I don't think we need this assert:

Okay. I also removed the assert before the call to mark_object.
fast_is_in_cset() already checks that the object reference points to
committed space, which is more strict than checking whether the
reference is in reserved space.
Trying to avoid duplicate asserts - fastdebug is slow enough already.

New webrev at http://cr.openjdk.java.net/~tschatzl/8035326/webrev.1 that
addresses your comments, and adds a comment to the assert in
in_cset_fast_test() as noticed by Jon.

Thanks,
Thomas