RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Kharbas, Kishor
kishor.kharbas at intel.com
Sat Nov 17 02:35:55 UTC 2018
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