RFR: 8224815: Remove non-GC uses of CollectedHeap::is_in_reserved()
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Aug 29 18:07:21 UTC 2019
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
More information about the hotspot-dev
mailing list