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
Sun Nov 18 19:14:26 UTC 2018
Hi Kishor,
Could you give us incremental webrev? It will help a lot for reviewers.
Thanks,
Sangheon
On 11/16/18 6:35 PM, Kharbas, Kishor wrote:
> Hi,
>
> I have an update to the webrev at : http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01
> 1. Most of the comments from Sangheon and Stefan have been addressed. For ease of reading, I am copy-pasting below the comments not fixed or for which I have follow up.
> 2. I also ran jtreg tests, with and without flag using test group shown below (removed tests incompatible with this flag. Also, for testing, I made -XX:+AllocateOldGenAt a no-op instead of error since many tests have sub-tests for all different GCs)
> I see same number of failures (12) for original, patch without flag and with flag (no new failures)
> 3. Tasks in progress are :
> - AppCDS tests.
> - CSR filing.
>
> tier1_old_nvdimm = \
> serviceability/ \
> :tier1_gc \
> :tier1_runtime \
> -gc/serial \
> -gc/parallel \
> -gc/cms \
> -gc/epsilon \
> -gc/TestAllocateHeapAt.java \
> -gc/TestAllocateHeapAtError.java \
> -gc/TestAllocateHeapAtMultiple.java \
> -gc/g1/TestFromCardCacheIndex.java \
> -gc/metaspace/TestMetaspacePerfCounters.java \
> -gc/metaspace/TestPerfCountersAndMemoryPools.java \
> -runtime/Metaspace/PrintMetaspaceDcmd.java
>
> ========================================================
> Comments from Sangheon:
> ========================================================
> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
>
> 173 void G1RegionToHeteroSpaceMapper::map_nvdimm_space(ReservedSpace rs) {
> - Your previous change of adding AllocateHeapAt created nv file inside of ReservedHeapSpace but this-AllocateOldGenAt create inside of mapper. Couldn't old gen also create its file near there? I feel like creating here seems un-natural.
> - This is just an idea. Instead of adding AllocateOldGenAt, couldn't re-use AllocateHeapAt and add type option? e.g. 'all' or 'old' or 'off'.
>
>>> If I do this in ReservedHeapSpace, all of the reserved memory will be mapped to nv-dimm. Since here I want part of reserved memory in nv-dimm, I can't use this directly, or easily modify it for our need.
>>> I like the idea of re-using AllocateHeapAt. I however not sure how can a '-XX option handle extra option.( I am thinking of something similar to -Xbootclasspath/a(p))
> ========================================================
> src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.cpp
> 34 // expand_by() is called to grow the heap. We grow into nvdimm now.
> 35 // Dram regions are committed later as needed during mutator region allocation or
> 36 // when young list target length is determined after gc cycle.
> 37 uint HeapRegionManagerForHeteroHeap::expand_by(uint num_regions, WorkGang* pretouch_workers) {
> 38 uint num_expanded = expand_nvdimm(MIN2(num_regions, max_expandable_length() - total_regions_committed()), pretouch_workers);
> 39 assert(total_regions_committed() <= max_expandable_length(), "must be");
> 40 return num_expanded;
> 41 }
> - I guess the only reason of not adjusting dram here is that we don't have information of 'is_old'? Looks a bit weired but I don't have any suggestion. (see more below at allocate_free_region)
>
>>> Yes, expand_by() is used to grow the heap at initialization or after full GC. Since young generation target length is decided at a later point, I expand in nv-dimm space here.
>>> Later when young generation target length is determined, adjust_dram_regions() is called to provision the target number of regions.
> ========================================================
> Comments from Stefan
> ========================================================
> 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.
>
>>> I abstracted out this in G1HeteroCollectorPolicy.
> 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);
>
>>> Since the mapper needs to map the reserved memory, Since create_heap_mapper() does not need anything from collector_policy, I chose to keep the call unchanged.
> 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.
>
>>> Yes! I had given this a thought. We can always procure a free region in nv-dimm. However if we do this, during this GC phase there could be more regions committed in memory than Xmx.
>>> This would mean heap memory usage increases to more than 'Xmx' during some gc phases. Will this be ok?
>>> But again in current implementation memory usage is more than Xmx anyway, since I allocate Xmx memory in nv-dimm at start. I do this because allocating in nv-dimm involves mapping to a file system,
>>> which becomes complicated if you expand/shrink file whenever you commit/uncommit regions. But I chose to keep this only in G1regionToSpaceMapper and not design rest of the code based on this.
>>> I had to make an exception for Full GC where I increase the committed regions in nv-dimm before start of GC, since we need to move all objects to old generation.
>>> Let me know what you think, I like your idea since we don't need to add more complexity for handling premature old regions.
> Thanks and regards,
> Kishor
>
>> -----Original Message-----
>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>> Sent: Monday, November 5, 2018 9:39 PM
>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
>> <stefan.johansson at oracle.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,
>>
>> 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