RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
sangheon.kim at oracle.com
sangheon.kim at oracle.com
Tue Nov 6 05:39:01 UTC 2018
Hi Kishor,
On 11/2/18 2:48 PM, Kharbas, Kishor wrote:
> Thanks Stefan and Sangheon for your comments.
>
> As suggested, I ran jtreg for serviceability tests. I see failures even without using the new flag.
> Looks like things are breaking in mirror classes (jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.*) because of new fields and/or method changes in JVM classes.
> Is there a bkm for changing these mirror Java classes when we change JVM classes?
Sorry I'm not aware of any better known method than fixing one-by-one.
>
> I am working on addressing your comments on the patch (had been dragged into another project, so I am behind on this).
Thanks for your update.
Thanks,
Sangheon
>
> Thanks
> Kishor
>
>> -----Original Message-----
>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>> Sent: Friday, October 26, 2018 7:32 AM
>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; 'hotspot-gc-
>> dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev
>> runtime' <hotspot-runtime-dev at openjdk.java.net>
>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java heap on
>> alternate memory devices - G1 GC
>>
>> Hi Kishor,
>>
>> I'll start with a few general observations and then there are some specific
>> code comments below.
>>
>> I think the general design looks promising, not having to set a fixed limit for
>> what can end up on NV-dimm is great. Having a separate
>> HeapRegionManager looks like a good abstraction and when adding larger
>> features like this that is really important.
>>
>> I also agree with Sangheon that you should do more testing, both with and
>> without the featured turned on. I also recommend you to build with pre-
>> compiled headers disabled, to find places where includes have been
>> forgotten. To configure such build add --disable-precompiled-headers to
>> your configure call.
>>
>> On 2018-10-04 04:08, Kharbas, Kishor wrote:
>>> Hi all,
>>>
>>> Requesting review of the patch for allocating old generation of g1 gc
>>> on alternate memory devices such nv-dimm.
>>>
>>> The design of this implementation is explained on the bug page -
>>> https://bugs.openjdk.java.net/browse/JDK-8211425
>>>
>>> Please follow the parent issue here :
>>> https://bugs.openjdk.java.net/browse/JDK-8202286.
>>>
>>> Webrev: http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00
>>> <http://cr.openjdk.java.net/%7Ekkharbas/8211425/webrev.00>
>> src/hotspot/share/gc/g1/g1Allocator.inline.hpp
>> ----------------------------------------------
>> 100 size_t length = static_cast
>> <G1CollectedHeap*>(Universe::heap())->max_reserved_capacity();
>>
>> You can avoid the cast by using G1CollectedHeap::heap() instead of
>> Universe::heap().
>> ---
>>
>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>> -------------------------------------------
>> 1642 if (AllocateOldGenAt != NULL) {
>> 1643 _is_hetero_heap = true;
>> 1644 max_byte_size *= 2;
>> 1645 }
>>
>> I would like this to be abstracted out of G1CollectedHeap, not entirely sure
>> how but I'm thinking something like adding a G1HeteroCollectorPolicy which
>> answers correctly on how much memory needs to be reserved and how big
>> the heap actually is.
>>
>> 1668 G1RegionToSpaceMapper::create_heap_mapper(g1_rs,
>> 1669 g1_rs.size(),
>> 1670 page_size,
>> 1671 HeapRegion::GrainBytes,
>> 1672 1,
>> 1673 mtJavaHeap);
>>
>> This function could then also make use of the new policy, something like:
>> create_heap_mapper(g1_rs, _collector_policy, page_size);
>>
>> 1704 if (is_hetero_heap()) {
>> 1705 _hrm = new
>> HeapRegionManagerForHeteroHeap((uint)((max_byte_size
>> / 2) / HeapRegion::GrainBytes /*heap size as num of regions*/));
>> 1706 }
>> 1707 else {
>> 1708 _hrm = new HeapRegionManager();
>> 1709 }
>>
>> This code could then become something like:
>> _hrm = HeapRegionManager::create_manager(_collector_policy)
>>
>> 3925 if(g1h->is_hetero_heap()) {
>> 3926 if(!r->is_old()) {
>> ....
>> 3929 r->set_premature_old();
>> 3930 }
>> 3931 } else {
>> 3932 r->set_old();
>> 3933 }
>>
>> So if I understand this correctly, premature_old is used when we get evac
>> failures and we use it to force these regions to be included in the next Mixed
>> collection. I'm not sure this is needed, in fact I think one of the cool things
>> with nv-dimm is that we can avoid evac failures. G1 normally needs to stop
>> handing out regions to promote to when there are no regions left, but when
>> using nv-dimm the old regions come from another pool and we should be
>> able to avoid this case.
>> ---
>>
>> src/hotspot/share/gc/g1/g1FullCollector.cpp
>> -------------------------------------------
>> 169 if (_heap->is_hetero_heap()) {
>> 170 static_cast
>> <HeapRegionManagerForHeteroHeap*>(_heap->_hrm)-
>>> prepare_for_full_collection_start();
>> 171 }
>>
>> Move this to:
>> G1CollectedHeap::prepare_heap_for_full_collection()
>>
>> 185 if (_heap->is_hetero_heap()) {
>> 186 static_cast
>> <HeapRegionManagerForHeteroHeap*>(_heap->_hrm)-
>>> prepare_for_full_collection_end();
>> 187 }
>>
>> Move this to:
>> G1CollectedHeap::prepare_heap_for_mutators()
>>
>> And if you make the prepare-functions virtual and not doing anything on
>> HeapRegionManager we can avoid the checks.
>> ---
>>
>> src/hotspot/share/gc/g1/g1Policy.cpp
>> ------------------------------------
>> 223 if(_g1h->is_hetero_heap() && (Thread::current()->is_VM_thread()
>> || Heap_lock->owned_by_self())) {
>> 224 static_cast
>> <HeapRegionManagerForHeteroHeap*>(_g1h->hrm())-
>>> resize_dram_regions(_young_list_target_length,
>> _g1h->workers());
>> 225 }
>>
>> Feels like this code should be part of the
>> prepare_for_full_collection_end() above, but the _young_list_target_length
>> isn't updated until right after
>> prepare_heap_for_mutators() so you might need to restructure the code a
>> bit more to make it fit.
>> ---
>>
>> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
>> -------------------------------------------------
>> Had to add these two includes to make it compile.
>> #include "runtime/java.hpp"
>> #include "utilities/formatBuffer.hpp"
>>
>> Please also clean out all code commented out.
>> ---
>>
>> src/hotspot/share/gc/g1/heapRegionManager.hpp
>> ---------------------------------------------
>> I agree with Sangheon, please group the members that should be protected
>> and remember to update the init-list for the constructor.
>>
>> Also agree that the #else on #ifdef ASSERT should be removed.
>> ---
>>
>> src/hotspot/share/gc/g1/heapRegionSet.cpp
>> -----------------------------------------
>> 240 bool started = false;
>> 241 while (cur != NULL && cur->hrm_index() <= end) {
>> 242 if (!started && cur->hrm_index() >= start) {
>> 243 started = true;
>> 244 }
>> 245 if(started) {
>> 246 num++;
>> 247 }
>> 248 cur = cur->next();
>> 249 }
>>
>> To me this looks more complex than it is because of the multiple conditions,
>> what do you think about:
>> while (cur != NULL) {
>> uint index = cur->hrm_index();
>> if (index > end) {
>> break;
>> } else if (index >= start) {
>> num++;
>> }
>> cur = cur->next();
>> }
>> ---
>>
>> src/hotspot/share/runtime/arguments.cpp
>> ---------------------------------------
>> 2072 if(!FLAG_IS_DEFAULT(AllocateHeapAt) &&
>> !FLAG_IS_DEFAULT(AllocateOldGenAt)) {
>> 2073 jio_fprintf(defaultStream::error_stream(),
>> 2074 "AllocateHeapAt and AllocateOldGenAt cannot be used
>> together.\n");
>> 2075 status = false;
>> 2076 }
>> 2077
>> 2078 if (!FLAG_IS_DEFAULT(AllocateOldGenAt) && (UseSerialGC ||
>> UseConcMarkSweepGC || UseEpsilonGC || UseZGC)) {
>> 2079 jio_fprintf(defaultStream::error_stream(),
>> 2080 "AllocateOldGenAt not supported for selected GC.\n");
>> 2081 status = false;
>> 2082 }
>>
>> This code would fit much better in GCArguments. There is currently no
>> method handling this case but please add check_args_consistency(). You can
>> look at CompilerConfig::check_args_consistency for inspiration.
>> ---
>>
>> src/hotspot/share/runtime/globals.hpp
>> -------------------------------------
>> 2612 experimental(uintx, G1YoungExpansionBufferPerc, 10,
>>
>> Move to g1_globals.hpp and call it G1YoungExpansionBufferPercent.
>> ---
>>
>> src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.*pp
>> ----------------------------------------------------------
>>
>> Haven't reviewed this code in detail yet but will do in a later review.
>> One thought I have already is about the name, what do you think about:
>> HeterogeneousHeapRegionManager
>> ---
>>
>> Thanks,
>> Stefan
>>
>>> Testing : Passed tier1_gc and tier1_runtime jtret tests.
>>>
>>> I added extra options
>>> "-XX:+UnlockExperimentalVMOptions -
>> XX:AllocateOldGenAt=/home/Kishor"
>>> to each test. There are some tests which I had to exclude since they
>>> won't work with this flag. Example: tests in ConcMarkSweepGC which
>>> does not support this flag, tests requiring CompressedOops to be enabled,
>> etc.
>>> Thanks
>>>
>>> Kishor
>>>
More information about the hotspot-gc-dev
mailing list