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

[Stefan Johansson]

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