RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Kharbas, Kishor
kishor.kharbas at intel.com
Wed Dec 5 08:51:18 UTC 2018
Hi Stefan, Sangheon,
I have another webrev update -
http://cr.openjdk.java.net/~kkharbas/8211425/webrev.06
http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05_to_06
Changes are:
1. While testing the new evacuation handling mechanism, I identified some issues and fixed them. The implementation has been tested by tracing the code path through the debugger.
Based on the fix, here is an explanation for evacuation failure handling -
1) The implementation is based on the axiom that for every region in dram where is a unavailable (not 'committed') region in nv-dimm. By borrowing such regions, we can avoid evacuation failure for young objects and thus avoid young->old region conversion.
2) We cannot use borrowed regions for old objects. Since in worst case for evacuating 'n' young and 'm' old regions we need 'n + m' destination regions. And we may need to borrow 'n+m' from nv-dimm. But we can only guarantee 'n' borrowed regions.
So we use borrowed region only for copying young objects, else we cannot avoid young->old conversion and will need a special region type as in current implementation anyway.
3) When we cannot allocate a new region for OldGCAllocRegion, we borrow a region from hrm() to use as allocation region. We release (un-commit) that many number of regions in adjust_dram_regions() which is called at end of GC.
4) So we may be in situation where there is valid old plab and OldGCAllocRegion but we can only use them when source is young. So we add another clause to the if() statement at the beginning of copy_to_survivor_space() to check for this condition.
For ease of review I extracted evacuation failure handling changes here - http://cr.openjdk.java.net/~kkharbas/8211425/webrev_EvacuationFailure
2. There is a fix for incorrect handling of humongous allocations under certain situations.
3. My jtreg tests are running, will provide the results when they finish.
Let me know your feedback.
Thank you.
Kishor
> -----Original Message-----
> From: Kharbas, Kishor
> Sent: Monday, December 3, 2018 11:48 PM
> To: sangheon.kim at oracle.com; Stefan Johansson
> <stefan.johansson at oracle.com>
> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>;
> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Kharbas, Kishor
> <kishor.kharbas at intel.com>
> Subject: RE: RFR(M): 8211425: Allocation of old generation of java heap on
> alternate memory devices - G1 GC
>
> Hi Sangheon and Stefan,
>
> I have updated webrev for this feature,
>
> 1. Webrev with Stefan's comments on Parallel GC (abstraction related
> comments) : http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04
>
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03_to_04
>
> 2. Webrev with evacuation failure handling :
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05
>
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04_to_05
> Testing underway.
>
> 3. Explanation of evacuation failure handling:
> 1) The implementation is based on the axiom that for every region in
> dram where is a unavailable (not 'committed') region in nv-dimm. By
> borrowing such regions, we can avoid evacuation failure for young objects
> and thus avoid young->old region conversion.
> 2) We cannot use borrowed regions for old objects. Since in worst
> case for evacuating 'n' young and 'm' old regions we need 'n + m' destination
> regions. And we may need to borrow 'n+m' from nv-dimm. But we can only
> guarantee 'n' borrowed regions.
> So we use borrowed region only for copying young objects, else we
> cannot avoid young->old conversion and will need a special region type as in
> current implementation anyway.
> 3) When we cannot allocate a new region for OldGCAllocRegion, we
> borrow a region from hrm() to use as allocation region. We sort of release
> that many number of regions in adjust_dram_regions() which is called at end
> of GC.
> 4) When G1ParScanThreadState: _old_gen_is_full is true, we only let
> young objects be allocated from the borrowed region. There might be a race
> condition here, when one thread enters plab_allocate() while other thread
> determines and set _old_gen_is_full.
> But we can tolerate this race condition.
>
> 4. I am testing all tier1 tests and appCDS tests with the combined patch. Will
> provide an update soon.
>
> Thanks,
> Kishor
>
> > -----Original Message-----
> > From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> > Sent: Sunday, December 2, 2018 2:49 PM
> > To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
> > <stefan.johansson at oracle.com>
> > Cc: 'hotspot-gc-dev at openjdk.java.net'
> > <hotspot-gc-dev at openjdk.java.net>;
> > 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
> > 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/29/18 12:19 PM, Kharbas, Kishor wrote:
> > > Hi Sangheon,
> > >
> > > Thank you for reviewing the webrev.
> > > 1. Based on your feedback I have newer webrev. There are some
> > comments inline about AppCDS (top of previous email).
> > >
> > > 2. Webrev with your feedback on G1 GC patch :
> > http://cr.openjdk.java.net/~kkharbas/8211425/webrev.02/
> > >
> > > http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01_to_02/
> > Webrev.02 looks good.
> >
> > >
> > > 3. I merged the Parallel GC patch with your latest comment,
> > > consolidated jtreg tests and combined webrev is :
> > > http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03
> > I meant to work based on Parallel GC patch so that your G1 patch
> > doesn't need to comment out Paralell GC test case at newly added tests.
> >
> > I only checked commented out parts of Parallel GC are enabled. This
> > looks good too.
> > I would expect that you completed running whole your patches(G1 and
> > Parallel GC) together without any failures.
> >
> > >
> > > 4. I moved the CSR to finalized. Thanks for your review. We will
> > > wait to
> > hear from CSR committee.
> > Okay, I saw it is approved now.
> >
> > >
> > > 5. I am working on Stefan's latest feedback on handling of
> > > evacuation
> > failure.
> > OK.
> >
> > Thanks,
> > Sangheon
> >
> >
> > >
> > > Regards,
> > > Kishor
> > >
> > >> -----Original Message-----
> > >> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> > >> Sent: Tuesday, November 27, 2018 9:25 PM
> > >> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
> > >> <stefan.johansson at oracle.com>
> > >> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-
> > dev at openjdk.java.net>;
> > >> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
> > >> 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/19/18 6:20 PM, Kharbas, Kishor wrote:
> > >>> Hi Sangheon,
> > >>>
> > >>> Here is the incremental webrev -
> > >>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00_to_01
> > >> Thanks for the incremental webrev.
> > >> Here are some comments for webrev.01.
> > >>
> > >> 1. General comments:
> > >> - Any update with AppCDS tests? (You mentioned this on your
> > >> previous
> > email)
> > > [Kharbas, Kishor] I completed testing of AppCDS with : 1) original
> > > code. 2)
> > patch applied, but flag not used. 3) patch applied, flag set.
> > > Test results at :
> > http://cr.openjdk.java.net/~kkharbas/8211425/JT_AppCDS
> > > 1) and 2) both show "Test results:
> > > passed: 120; failed: 1;
> > error: 2"
> > > 3) No tests are run, all are
> > > skipped. Do you know if
> > appCDS does not work with certain flags. I use jtreg flag
> > -javaoptions:"- XX:+UnlockExperimentalVMOptions -
> XX:AllocateOldGenAt=/home/kishor".
> > >
> > >> - Please test with ParallelGC patch added. i.e. Applied both
> > >> ParallelGC and
> > G1
> > >> parts, so that you don't need to comment out ParallelGC on new tests.
> > >>
> > > [Kharbas, Kishor] The tests pass with merged patch, testing both
> > > ParallelGC
> > and G1.
> > >
> > >> 2. New tests related general comments:
> > >> - As AllocateOldGenAt feature will not be tested regularly, new
> > >> tests
> > should
> > >> be excluded by default.
> > > [Kharbas, Kishor] How do I do this? Should I remove these test from
> > TEST.groups?
> > >
> > >> - New tests need filters for other unsupported GCs.
> > >> e.g. @requires vm.gc=="null" | vm.gc.G1 | vm.gc.Parallel
> > > [Kharbas, Kishor] Done.
> > >> - Are there any reason for printing stdout in new tests? i.e. looks
> > >> like we don't need 'System.out.println(output.getStdout());'
> > >>
> > > [Kharbas, Kishor] not really, I checked that output is dumped to
> > > stdout of
> > jtreg anyway. So I removed this.
> > >> ----------------------
> > >> src/hotspot/share/gc/g1/g1CollectedHeapHeap.hpp
> > >> 45 #include "gc/g1/heapRegionManager.hpp"
> > >> 46 #include "gc/g1/heterogeneousHeapRegionManager.hpp"
> > >> 47 #include "gc/g1/heapRegionSet.hpp"
> > >> - Line 46 should be below line 47.
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/gc/g1/g1CollectorPolicy.hpp
> > >> 41 virtual size_t heap_reservation_size_bytes();
> > >> - const?
> > >>
> > >> 45 size_t _heap_reservation_size_bytes;
> > >> - Do we really need this variable? Just returning
> > >> 2*_max_heap_byte_size seems enough. In this case, adding virtual
> > >> size_t
> > >> heap_reservation_size_bytes() seems enough.
> > >> - Regarding naming, how about heap_reserved_size_bytes()?
> > >>
> > >> 51 size_t heap_reservation_size_bytes();
> > >> - I would prefer to have 'virtual' for readability. I saw you
> > >> reflected this
> > kind
> > >> of my comment in other places so asking here too.
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
> > >> 25 #include "logging/log.hpp"
> > >> - precompiled.hpp comes first
> > >>
> > >> 180 return false;
> > >> 181 log_error(gc, init)("Could not create file for Old
> > >> generation at location %s", AllocateOldGenAt);
> > >> - We never reach line 181 because of line 180? :)
> > >>
> > >> 196
> > >>
> >
> G1RegionToHeteroSpaceMapper::G1RegionToHeteroSpaceMapper(Reserve
> > >> dSpace rs,
> > >> - My previous comment was about splitting initialization part(which
> > >> does actual file-heap mapping) from ctor, so that we can avoid
> > >> failure during construction. After construction, call initialize
> > >> method to do file mapping and then report any failures if have.
> > >>
> > >> 196
> > >>
> >
> G1RegionToHeteroSpaceMapper::G1RegionToHeteroSpaceMapper(Reserve
> > >> dSpace rs,
> > >> 197 size_t actual_size,
> > >> 198 size_t page_size,
> > >> 199 size_t alloc_granularity,
> > >> 200 size_t commit_factor,
> > >> 201 MemoryType type) :
> > >> 202 G1RegionToSpaceMapper(rs, actual_size, page_size,
> > >> alloc_granularity, commit_factor, type),
> > >> 203 _num_committed_dram(0), _num_committed_nvdimm(0),
> > >> _success(false) {
> > >> - Looks like a bad alignment. I think parameters and initialization
> > >> list(parental class as well) should be distinguishable.
> > >>
> > >>
> > >> 295 delete mapper;
> > >> - Existing issue(no action needed): current mapper code doesn't
> > >> have a destructor.
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/gc/g1/heapRegionManager.cpp
> > >> 29 #include "gc/g1/heapRegionManager.inline.hpp"
> > >> 30 #include "gc/g1/heterogeneousHeapRegionManager.hpp"
> > >> 31 #include "gc/g1/heapRegionSet.inline.hpp"
> > >> - Move line 30 after line 31.
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/gc/g1/heapRegionManager.hpp
> > >>
> > >> 117 FreeRegionList _free_list;
> > >> 118 void make_regions_available(uint index, uint num_regions =
> > >> 1,
> > >> WorkGang* pretouch_gang = NULL);
> > >> - New line between 117 and 118 please.
> > >>
> > >> 130 static HeapRegionManager* create_manager(G1CollectedHeap*
> > >> heap,
> > >> CollectorPolicy* policy);
> > >> - I see build failure in solaris.
> > >> open/src/hotspot/share/gc/g1/heapRegionManager.hpp", line 129:
> Error:
> > >> CollectorPolicy is not defined.
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/gc/g1/heapRegionType.cpp
> > >> - Copyright update
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/gc/shared/gcArguments.cpp
> > >>
> > >> 68 "AllocateOldGenAt not supported for selected GC.\n");
> > >> - "AllocateOldGenAt *is* not supported ..."
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/prims/whitebox.cpp
> > >> 510 return (jlong)(Universe::heap()->base() + start_region
> > >> * HeapRegion::GrainBytes);
> > >> - g1h->base() is same, isn't it? There are more lines that using
> > >> Universe::heap()->base().
> > >>
> > >> 524 }
> > >> 525 else {
> > >> and
> > >> 551 }
> > >> 552 else {
> > >> - Make at one line. i.e. '} else {'
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/runtime/arguments.cpp
> > >> 1625 if (AllocateOldGenAt != NULL) {
> > >> 1626 FLAG_SET_ERGO(bool, UseCompressedOops, false);
> > >> 1627 return;
> > >> 1628 }
> > >> - Wrong alignment. Needs 2 spaces for line 1625~1628.
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/runtime/globals.hpp
> > >> 2606 experimental(ccstr, AllocateOldGenAt, NULL,
> > >> \
> > >> 2607 "Directory to use for allocating old
> > >> generation")
> > >> - How about moving AllocateOldGenAt around AllocateHeapAt option.
> > >> - Change the description similar to AllocateHeapAt as it explains better.
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
> > >> 29 #include "gc/g1/heapRegionManager.inline.hpp"
> > >> 30 #include "gc/g1/heterogeneousHeapRegionManager.hpp"
> > >> 31 #include "gc/g1/heapRegionSet.inline.hpp"
> > >> - Line 30 should be located after line 31.
> > >>
> > >>
> > >> ----------------------
> > >> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
> > >>
> > >> #ifndef SHARE_VM_GC_G1_HeterogeneousHeapRegionManager_HPP
> > >> - Should be all uppercase letters.
> > >>
> > >>
> > >> ----------------------
> > >> test/hotspot/jtreg/gc/8202286/TestAllocateOldGenAtMultiple.java
> > >> 56 "-Xmx4g -Xms4g", // 4.
> > >> With larger heap size (UnscaledNarrowOop not possible).
> > >> 57 "-Xmx4g -Xms4g -XX:+UseLargePages", // 5.
> > >> Set UseLargePages.
> > >> 58 "-Xmx4g -Xms4g -XX:+UseNUMA" // 6. Set UseNUMA.
> > >> - It would be better to use smaller heap for test stability(even
> > >> though this will not be tested regularly) unless you have any good
> reason for it.
> > >>
> > >>
> > >>> I have also filed a CSR at
> > >>> https://bugs.openjdk.java.net/browse/JDK-
> > >> 8214081
> > >> I reviewed this CSR as well.
> > >>
> > >> Thanks,
> > >> Sangheon
> > >>
> > >>
> > >>> 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