RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC

[Kharbas, Kishor]

Hi Stefan,
Thanks for the review.

I agree that handling evacuation failures by handing out extra regions from nv-dimm will reduce the complexity. Will work on this, and provide update soon.

Thanks
Kishor

> -----Original Message-----
> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> Sent: Thursday, November 29, 2018 5:50 AM
> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> sangheon.kim 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,
> 
> 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.00_to_01
> >
> 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->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.
> 
> 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