RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices (original) (raw)

Stefan Johansson stefan.johansson at oracle.com
Thu Nov 29 13:50:22 UTC 2018

Hi Kishor,

Thanks for updated webrev, I have some more comments.

A general comment is that you should make sure that your includes are sorted alphabetically, Sangheon mentions that as well but I wanted to point it out. Please also see the comment regarding premature_old at bottom of the mail.

On 2018-11-20 03:20, Kharbas, Kishor wrote: > Hi Sangheon, >> Here is the incremental webrev - http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00to01 >src/hotspot/share/gc/g1/g1CollectedHeap.cpp

1493 _is_hetero_heap(AllocateOldGenAt != NULL),

Does G1CollectedHeap really need to know this, can't we always just as the G1CollectorPolicy? As a matter of fact, I think most places where we check is_hetro_heap() a better solution is to use virtual calls or other abstractions to do the right thing.

1786 // Now we know the target length of young list. So adjust the heap to provision that many regions on dram. 1787 if (is_hetero_heap()) { 1788 static_cast<HeterogeneousHeapRegionManager*>(hrm())->adjust_dram_regions((uint)g1_policy()->young_list_target_length(), workers()); 1789 }

Here we have an example of what I mentioned above. Instead of having the check and the cast, is there a way we can make this happen every time we update the young_list_target_length(). I see that we currently only do this at initialization and after full collections, why don't we have to do this after each GC?

Same comment for lines 2532-2535.

src/hotspot/share/gc/g1/heapRegionManager.cpp

71 HeapRegionManager* HeapRegionManager::create_manager(G1CollectedHeap* heap, CollectorPolicy* policy) { 72 if (heap->is_hetero_heap()) { 73 return new HeterogeneousHeapRegionManager((uint)(policy->max_heap_byte_size() / HeapRegion::GrainBytes) /heap size as num of regions/); 74 } 75 return new HeapRegionManager(); 76 }

As I mentioned above, I think the is_hetero check should be done on the policy instead. Otherwise this is a place where this check is really needed.

src/hotspot/share/gc/g1/heapRegionSet.hpp

198 uint num_of_regions_in_range(uint start, uint end) const;

Was looking at the usage of this method and it is only used to get the length of the dram free-list. Looking at the HeteroHeapRegionManager wouldn't it be possible to add a second free-list, so we have one for dram and one for nvdimm.

src/hotspot/share/runtime/arguments.cpp

1625 if (AllocateOldGenAt != NULL) { 1626 FLAG_SET_ERGO(bool, UseCompressedOops, false); 1627 return; 1628 }

Any reason this can't be done in GCArguments::initialize()?

src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp

85 // Empty constructor, we'll initialize it with the initialize() method. 86 HeterogeneousHeapRegionManager(uint num_regions) : _max_regions(num_regions), _max_dram_regions(0), 87 _max_nvdimm_regions(0), _start_index_of_nvdimm(0) 88 {}

The member _total_commited_before_full_gc is not initialized.

98 // Adjust dram_set to provision 'expected_num_regions' regions. 99 void adjust_dram_regions(uint expected_num_regions, WorkGang* pretouch_workers);

As mentioned I would turn this into a virtual method if it can't be abstracted in another way. The WorkGang can be obtained by calling G1CollectedHeap::heap()->workers() so you don't need to pass it in. This way we get a cleaner interface I think.

test/hotspot/jtreg/gc/8202286/*.java

I would suggest moving all tests in gc/8202286 to gc/nvdimm or something like that, because we don't use JBS numbers for tests in GC.

I have also filed a CSR at https://bugs.openjdk.java.net/browse/JDK-8214081 Reviewed it.

Thank you, Kishor

[...] 3925 if(g1h->isheteroheap()) { 3926 if(!r->isold()) { .... 3929 r->setprematureold(); 3930 } 3931 } else { 3932 r->setold(); 3933 }

So if I understand this correctly, prematureold 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.

I would really like to avoid adding the complexity around premature old and I think "going over" Xmx in this case is no problem, because after the GC we will be under again (this we must of course make sure). Also, as you say, the memory reservation on nv-dimm is already done, so in reality this will not increase the memory usage.

So I would like to see a solution where we instead hand out old regions instead of get an evacuation failure when running with nvdimm.

Thanks, Stefan

More information about the hotspot-gc-dev mailing list