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

[Coleen Phillimore]

Erik.  It’s fine to split out these changes.  Please file an RFE and copy me. 
Thanks
Coleen

> On Aug 30, 2019, at 8:31 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Coleen,
> 
>> On 2019-08-29 20:07, 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.
> 
> You are right that we are sometimes overly conservative about checking if we have valid oops. However, If you are okay with it, I would prefer to split such changes out to a new RFE. Because this patch touches a lot of code, and I would like the patch to come with as few surprises as possible, and be as mechanical as possible. Are you okay with splitting out removal of unnecessary check to a separate RFE?
> 
>> 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*.
> 
> The check_oop_location function checks that the address looks like it could be inside of the heap, by looking at the address range, as well as alignment. But it is okay with there not being an oop there content wise. This is useful for example when you are forwarding an oop to a new location, where there is not an oop yet, but you want to make sure it is a valid looking oop location, ignoring the contents.
> 
> The is_in check, looks if the address is inside of heap memory that has been handed out to mutators. From now on, you never want is_in_reserved in shared code, which is easy to remember! Because the function is removed from CollectedHeap.
> 
>> 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 
> 
> That might be true. However, I would still like to defer such decisions to a follow-up RFE if you are okay with that, to reduce the number of surprises in this patch.
> 
>> - 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?
> 
> And that right there, is exactly what it checks. It's a sanity check that an oop shouldn't have a _klass pointing into the Java heap, because that couldn't possibly be a valid oop then. Arguably, it should instead check that the _klass points into metaspace, instead of checking that it doesn't point
> 
>> 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.
> 
> Sure. Again, I hope it is okay to defer removal of such asserts to a new RFE.
> 
>> 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?
> 
> Sure.
> 
>> I only glazed over the GC parts.  This looks good to me.
> 
> Thank you for the review Coleen. I will await if you are okay with splitting out of some of your proposed changes to the asserts before producing a new webrev.
> 
> Thanks,
> /Erik
> 
>> 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