RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Jiangli Zhou
jiangli.zhou at oracle.com
Thu Nov 29 21:04:50 UTC 2018
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