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

[Stefan Karlsson]

On 2014-02-20 14:58, 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_implem
>>>> 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

Looks good.

StefanK

>
> Thanks,
>    Thomas
>
>