RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Kharbas, Kishor
kishor.kharbas at intel.com
Fri Nov 30 00:25:06 UTC 2018
Thanks Jiangli,
I am able to avoid the 2 errors with -nativepath option.
-Kishor
> -----Original Message-----
> From: Jiangli Zhou [mailto:jiangli.zhou at oracle.com]
> Sent: Thursday, November 29, 2018 1:05 PM
> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> sangheon.kim at oracle.com; Stefan Johansson
> <stefan.johansson at oracle.com>
> Cc: Viswanathan, Sandhya <sandhya.viswanathan 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>
> Subject: Re: RFR(M): 8211425: Allocation of old generation of java heap on
> alternate memory devices - G1 GC
>
> Hi Kishor,
>
> Please see some comments below about AppCDS test.
>
>
> 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/
> >
> > 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".
>
> The EmptyClassInBootClassPath.java failure is a known issue:
> https://bugs.openjdk.java.net/browse/JDK-8213299
>
> The two tests with error require -nativepath. You can build the test native
> image with 'make test-image', then set -nativepath to
> images/test/hotspot/jtreg/native in your build.
>
> Thanks,
> Jiangli
> >
> >> - 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