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

[Erik Österlund]

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
>