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

[Stefan Karlsson]

On 2014-02-20 11:13, Thomas Schatzl wrote:
> 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 for the changes.

Would it be possible to shrink the the comment and make it a one-liner. 
Just to make it less conspicuous?

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   }

I'm fine with the changes if you don't want to.

thanks,
StefanK

>
> Thanks,
> Thomas
>