RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Kharbas, Kishor
kishor.kharbas at intel.com
Thu Nov 29 20:19:01 UTC 2018
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/
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
4. I moved the CSR to finalized. Thanks for your review. We will wait to hear from CSR committee.
5. I am working on Stefan's latest feedback on handling of evacuation failure.
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