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

sangheon.kim at oracle.com sangheon.kim at oracle.com
Tue Oct 23 21:53:34 UTC 2018


Hi Kishor,

Sorry for replying late.

On 10/3/18 7:08 PM, 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 
> <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>
>
General comments.

1. Please file CSR for newly added options.
2. Please run jtreg tests with and without your patch.
3. Did you test with AppCds as well?

You can skip style comments. :)

========================================================
src/hotspot/share/gc/g1/g1CollectedHeap.cpp

1489   _is_hetero_heap(false),
- _is_hetero_heap(AllocateOldGenAt != NULL) at initialization list.

1644     max_byte_size *= 2;
- It would be better to explain why multiple by 2?

1668     G1RegionToSpaceMapper::create_heap_mapper(g1_rs,
1669                                          g1_rs.size(),
1670                                          page_size,
1671 HeapRegion::GrainBytes,
1672                                          1,
1673                                          mtJavaHeap);
- If we change 'create_heap_mapper' to return NULL in case of failing to 
creating mapping file etc.., we can exit with error code. And add NULL 
check around line 1674.

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   }
- If we want to minimize adding the check for hetero heap, these codes 
can be moved line 1644.



========================================================
src/hotspot/share/gc/g1/g1FullCollector.cpp

  169   if (_heap->is_hetero_heap()) {
  170     static_cast 
<HeapRegionManagerForHeteroHeap*>(_heap->_hrm)->prepare_for_full_collection_start();

  186     static_cast 
<HeapRegionManagerForHeteroHeap*>(_heap->_hrm)->prepare_for_full_collection_end();
  187   }
- Stype. hrm() instead of _hrm?


========================================================
src/hotspot/share/gc/g1/g1Policy.cpp

  222   // Resize dram regions set if called after full collection end.
  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   }
- Instead of checking vmthread or heap lock, couldn't move somewhere else?
e.g. G1Policy::update_young_list_max_and_target_length() would be good 
fit if your concern is not being called from rset sampling thread.


========================================================
src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
  206   /* Toggle HeteroHeap
  207   // We map first part of size Xmx to DRAM.
  208   ReservedSpace rs_dram = rs.first_part(MaxHeapSize);
  209   // Second half of reserved memory is mapped to NV-DIMM.
  210   ReservedSpace rs_nvdimm = rs.last_part(MaxHeapSize);*/

  236 /* Toggle HeteroHeap
  237   _start_index_of_nvdimm = (uint)(rs_dram.size() / alloc_granularity);
  238   _start_index_of_dram = 0; */

  246   /* Toggle HeteroHeap
  247   uint num_nvdimm = end_idx >= _start_index_of_nvdimm ? 
MIN2((end_idx - _start_index_of_nvdimm + 1), (uint)num_regions) : 0;
  248   uint num_dram = (uint)num_regions - num_nvdimm;*/

  257     /* Toggle HeteroHeap
  258      _dram_mapper->commit_regions(start_idx, num_dram, 
pretouch_gang); */

  266   /* Toggle HeterHeap
  267   uint num_nvdimm = end_idx >= _start_index_of_nvdimm ? 
MIN2((end_idx - _start_index_of_nvdimm + 1), (uint)num_regions) : 0;
  268   uint num_dram = (uint)num_regions - num_nvdimm;*/
- There are some commentted out blocks, please remove them.

  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'.

  177     vm_exit_during_initialization(
- As the caller 'jint G1CollectedHeap::initialize()' is returning its 
status, I would prefer to return its status. In addition, it would be 
better not remapping nvdimm in ctor of G1RegionToHeteroSpaceMapper. i.e. 
Split initialization part(including map_nvdimm_space(), 
os::attempt_reserve_memory_at() etc) into other method. And call the 
newly added method after creating G1RegionToHeteroSpaceMapper.

  178       err_msg("Could not create file for Old generation at 
location %s", AllocateOldGenAt));
  179   }
- Include "utilities/formatBuffer.hpp" to avoid compile error in Solaris 
because of 'err_msg()'.

  182   //char* ret = 
os::replace_existing_mapping_with_file_mapping(rs.base(), rs.size(), 
_backing_fd);
- Please remove this line.


========================================================
src/hotspot/share/gc/g1/g1RegionToSpaceMapper.hpp
  114   uint num_committed_dram();
  115   uint num_committed_nvdimm();
- Add 'const'?


========================================================
src/hotspot/share/gc/g1/heapRegionManager.hpp


   74   protected:
   75   G1HeapRegionTable _regions;

   76   G1RegionToSpaceMapper* _heap_mapper;
   77   private:
   78   G1RegionToSpaceMapper* _prev_bitmap_mapper;
   79   G1RegionToSpaceMapper* _next_bitmap_mapper;
   80   G1RegionToSpaceMapper* _bot_mapper;
   81   G1RegionToSpaceMapper* _cardtable_mapper;
   82   G1RegionToSpaceMapper* _card_counts_mapper;
   83
   84   protected:
   85   FreeRegionList _free_list;
   86   private:
   87
   88   // Each bit in this bitmap indicates that the corresponding 
region is available
   89   // for allocation.
   90   CHeapBitMap _available_map;
   91
   92    // The number of regions committed in the heap.
   93   uint _num_committed;
   94
   95   // Internal only. The highest heap region +1 we allocated a 
HeapRegion instance for.
   96   uint _allocated_heapregions_length;
   97
   98   HeapWord* heap_bottom() const { return 
_regions.bottom_address_mapped(); }
   99   HeapWord* heap_end() const {return _regions.end_address_mapped(); }
  100
  101   protected:
  102   void make_regions_available(uint index, uint num_regions = 1, 
WorkGang* pretouch_gang = NULL);
  103   void uncommit_regions(uint index, size_t num_regions = 1);
  104   private:
- How about regrouping instead of adding bunch of access specifiers?

  128 #else
  129   // Returns whether the given region is available for allocation.
  130 protected:
  131   bool is_available(uint region) const;
  132 #endif
- This seems weird. Why is_available(uint) const and 'protected:' have a 
new condition of !ASSERT?

  208   // Return maximum number of regions that heap can expand to.
  209   virtual uint max_expandable_length() const { return 
(uint)_regions.length(); }
- It would be better to explain the difference with max_capacity().


========================================================
src/hotspot/share/gc/g1/heapRegionType.hpp

   79     PreMatureOldTag       = PreMatureOldMask,
   80     // Archive regions are regions with immutable content (i.e. 
not reclaimed, and
- Please add a new line between 79 and 80.


========================================================
src/hotspot/share/runtime/globals.hpp

2612   experimental(uintx, G1YoungExpansionBufferPerc, 
10,                       \
2613                "When heterogenous heap is enabled by 
AllocateOldGenAt "     \
2614                "option, after every GC, yg gen is re-sized which 
involves " \
2615                "system calls to commit/uncommit memory. To reduce 
these "   \
2616                "calls, we keep a buffer of extra regions to absorb 
small "  \
2617                "changes in yg gen length. This flag takes the 
buffer "      \
2618                "size as an percentage of young gen length")
- Add 'range(0, 100)'
- yg gen -> young gen
- This G1 specific option should be moved to g1_globals.hpp.


========================================================
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)

   50   num_regions = MIN2(num_regions, max_expandable_length() - 
total_regions_committed());
- Style. I would prefer to have a new variable instead of reusing.

   68 #ifdef ASSERT
   69      uint total_committed_before = total_regions_committed();
   70 #endif
   71     uint can_be_made_available = shrink_nvdimm(to_be_made_available);
- There is one more space at line 69.

  225   while (cur >= start_idx && (is_available(cur) && 
at(cur)->is_empty())) {
- Redundant parenthesis set before is_available(cur).

  239 HeapRegion* 
HeapRegionManagerForHeteroHeap::allocate_free_region(bool is_old) {
- If is_old=true from G1CollectedHeap::new_region(), your patch allows 
to expand 2 times for nvdimm. One from this method(new behavior), 1 from 
G1CollectedHeap::expand().
- If is_old=false in above case, there's no big difference w.r.t 
expanding POV. But the location of expanding is different. For now, I 
don't see other than mentioned difference, but would like to hear 
others' opinions.


========================================================
src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.hpp

   40   uint _max_regions;
- Add 'const'?

  111   // Override. Expand in nv-dimm.
- Style comment. I would prefer adding 'virtual' instead of override 
comment. There are more if you agree to change.


> 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.
>
As mentioned at the beginning, please test without the new option as well.

Thanks,
Sangheon


> Thanks
>
> Kishor
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20181023/88568068/attachment-0001.html>


More information about the hotspot-gc-dev mailing list