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
>
More information about the hotspot-runtime-dev
mailing list