RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Kharbas, Kishor
kishor.kharbas at intel.com
Tue Nov 20 02:20:58 UTC 2018
Hi Sangheon,
Here is the incremental webrev - http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00_to_01
I have also filed a CSR at https://bugs.openjdk.java.net/browse/JDK-8214081
Thank you,
Kishor
> -----Original Message-----
> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> Sent: Sunday, November 18, 2018 11:14 AM
> 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,
>
> 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