RFR: 8224815: Remove non-GC uses of CollectedHeap::is_in_reserved()

[coleen.phillimore at oracle.com]

On 8/29/19 2:07 PM, coleen.phillimore at oracle.com wrote:
>
> http://cr.openjdk.java.net/~eosterlund/8224815/webrev.00/src/hotspot/share/ci/ciObjectFactory.cpp.udiff.html 
>
>
> I can't believe how many files have 
> assert(Universe::heap()->is_in_reserved(obj)), which seem like they 
> should have failed somewhere else first, since the parameter is "oop".
>
>   ciObject* ciObjectFactory::get(oop key) {
>     ASSERT_IN_VM;
>   - assert(Universe::heap()->is_in_reserved(key), "must be");
> + assert(Universe::heap()->is_in(key), "must be");
>       NonPermObject* &bucket = find_non_perm(key);
>
>
> It looks like these were added during permgen elimination and really 
> aren't needed in this file.  We already know these are oops.
>
> http://cr.openjdk.java.net/~eosterlund/8224815/webrev.00/src/hotspot/share/gc/serial/markSweep.cpp.udiff.html 
>
>
> - assert(!Universe::heap()->is_in_reserved(p),
> + assert(!GenCollectedHeap::heap()->is_in_reserved(p),
>
> I'm confused when you want Universe::heap()->is_in() vs when you want 
> SomeMoreSpecific->is_in_reserved().  or check_oop_location().  It 
> looks like check_oop_location is chosen from inside GC where the type 
> is HeapWord*.
>
> It seems like all of the runtime asserts should just be 
> assert(OopDesc::is_oop(obj), "is oop"); and that calls 
> Universe::heap()->is_oop() and that calls is_in_reserved and the 
> alignment.
>
> http://cr.openjdk.java.net/~eosterlund/8224815/webrev.00/src/hotspot/share/gc/shared/collectedHeap.cpp.udiff.html 
>
>
> - if (is_in_reserved(object->klass_or_null())) {
> + if (is_in(object->klass_or_null())) {
>       return false;
>     }
>
> How can this be true?  _klass is in metaspace?
>
>
> http://cr.openjdk.java.net/~eosterlund/8224815/webrev.00/src/hotspot/share/interpreter/interpreterRuntime.cpp.udiff.html 
>
>
> All of these can be deleted.  The Handleizing will check is_oop() 
> which checks that the oop is in reserved space, eventually.
>
> http://cr.openjdk.java.net/~eosterlund/8224815/webrev.00/src/hotspot/share/oops/compressedOops.cpp.udiff.html 
>
>
> Can you put an assert(UseCompressedOops, "should only be called for 
> compressed oops");
>
> since you now assume that the caller checks this first?
>
> I only glazed over the GC parts.  This looks good to me.

If you make the suggested deletions, I don't need to see another webrev.
Thanks,
Coleen

>
> Thanks,
> Coleen
>
> On 8/29/19 9:20 AM, Erik Österlund wrote:
>> Hi Stefan,
>>
>> Thanks for having an in-depth look at this patch.
>>
>> On 2019-08-29 15:07, Stefan Karlsson wrote:
>>> Hi Erik,
>>>
>>> On 2019-08-09 09:43, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> The CollectedHeap::is_in_reserved() function assumes the GC has a 
>>>> contiguous heap. This might not be the case, and hence the shared 
>>>> code should not make assumptions about that. It should use 
>>>> CollectedHeap::is_in() instead.
>>>>
>>>> However, CompressedOops inherently assumes the heap is contiguous, 
>>>> so I let it know about the reserved region.
>>>>
>>>> This patch depends on 8229278 and 8229189, which are both out for 
>>>> review right now.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8224815
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8224815/webrev.00/
>>>
>>>
>>> I think this looks good, but have a few cleanups that I'd like to 
>>> suggest:
>>>
>>> http://cr.openjdk.java.net/~stefank/8224815/webrev.01.review.00/
>>>
>>> 1) Changed some parameters to accept void* to get rid of some casts
>>>
>>> 2) Replaced address with HeapWord* in GC code.
>>>
>>> 3) Moved the code that was moved to initialize_reserved_region and 
>>> moved it into Universe::reserve_heap. I could therefore restore the 
>>> location of the initialize_reserved_region calls. Fixed a test that 
>>> relied on the logging order.
>>>
>>> 4) Moved the heap address range MemRegion out of NarrowPtrStruct 
>>> into CompressedOops
>>>
>>> 5) Replaced some DEBUG_ONLY with NOT_DEBUG_RETURN.
>>>
>>> 6) Minor cleanups
>>>
>>> I ran this through tier1.
>>
>> Thank you for the bits. I think your improvements did make the code 
>> nicer, and I agree with all suggestions.
>>
>>> It's a bit annoying that we still have CollectedHeap::_reserved, but 
>>> I understand why you left it for now. However, I see that the 
>>> Serviceability Agent uses that field. Did you think about if that 
>>> affects ZGC support in SA in any way?
>>
>> Yeah it annoyed me a bit too. And no I haven't looked into what the 
>> SA does differently if you don't specify a reserve region. I will try 
>> to look into that.
>>
>> Thanks,
>> /Erik
>>
>>> Thanks,
>>> StefanK
>>>
>>>>
>>>> Thanks,
>>>> /Erik
>