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

[coleen.phillimore at oracle.com]

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.

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