<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Kishor,<br>
<br>
Sorry for replying late.<br>
<br>
<div class="moz-cite-prefix">On 10/3/18 7:08 PM, Kharbas, Kishor
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC4893E@ORSMSX116.amr.corp.intel.com">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
p.MsoPlainText, li.MsoPlainText, div.MsoPlainText
{mso-style-priority:99;
mso-style-link:"Plain Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle17
{mso-style-type:personal-compose;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.PlainTextChar
{mso-style-name:"Plain Text Char";
mso-style-priority:99;
mso-style-link:"Plain Text";
font-family:"Calibri",sans-serif;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoNormal">Hi all,<o:p></o:p></p>
<p class="MsoNormal">Requesting review of the patch for
allocating old generation of g1 gc on alternate memory devices
such nv-dimm.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">The design of this implementation is
explained on the bug page -
<a href="https://bugs.openjdk.java.net/browse/JDK-8211425"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8211425</a><o:p></o:p></p>
<p class="MsoNormal">Please follow the parent issue here : <a
href="https://bugs.openjdk.java.net/browse/JDK-8202286"
moz-do-not-send="true">
https://bugs.openjdk.java.net/browse/JDK-8202286</a>.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Webrev: <a
href="http://cr.openjdk.java.net/%7Ekkharbas/8211425/webrev.00"
moz-do-not-send="true">
http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00</a><o:p></o:p></p>
</div>
</blockquote>
General comments.<br>
<br>
1. Please file CSR for newly added options.<br>
2. Please run jtreg tests with and without your patch.<br>
3. Did you test with AppCds as well? <br>
<br>
You can skip style comments. :)<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/g1CollectedHeap.cpp<br>
<br>
1489 _is_hetero_heap(false),<br>
- _is_hetero_heap(AllocateOldGenAt != NULL) at initialization list.<br>
<br>
1644 max_byte_size *= 2;<br>
- It would be better to explain why multiple by 2?<br>
<br>
1668 G1RegionToSpaceMapper::create_heap_mapper(g1_rs,<br>
1669 g1_rs.size(),<br>
1670 page_size,<br>
1671
HeapRegion::GrainBytes,<br>
1672 1,<br>
1673 mtJavaHeap);<br>
- 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.<br>
<br>
1704 if (is_hetero_heap()) {<br>
1705 _hrm = new
HeapRegionManagerForHeteroHeap((uint)((max_byte_size / 2) /
HeapRegion::GrainBytes /*heap size as num of regions*/));<br>
1706 }<br>
1707 else {<br>
1708 _hrm = new HeapRegionManager();<br>
1709 }<br>
- If we want to minimize adding the check for hetero heap, these
codes can be moved line 1644.<br>
<br>
<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/g1FullCollector.cpp<br>
<br>
169 if (_heap->is_hetero_heap()) {<br>
170 static_cast
<HeapRegionManagerForHeteroHeap*>(_heap->_hrm)->prepare_for_full_collection_start();<br>
<br>
186 static_cast
<HeapRegionManagerForHeteroHeap*>(_heap->_hrm)->prepare_for_full_collection_end();<br>
187 }<br>
- Stype. hrm() instead of _hrm?<br>
<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/g1Policy.cpp<br>
<br>
222 // Resize dram regions set if called after full collection
end.<br>
223 if(_g1h->is_hetero_heap() &&
(Thread::current()->is_VM_thread() ||
Heap_lock->owned_by_self())) {<br>
224 static_cast
<HeapRegionManagerForHeteroHeap*>(_g1h->hrm())->resize_dram_regions(_young_list_target_length,
_g1h->workers());<br>
225 }<br>
- Instead of checking vmthread or heap lock, couldn't move somewhere
else?<br>
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.<br>
<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp<br>
206 /* Toggle HeteroHeap<br>
207 // We map first part of size Xmx to DRAM.<br>
208 ReservedSpace rs_dram = rs.first_part(MaxHeapSize);<br>
209 // Second half of reserved memory is mapped to NV-DIMM.<br>
210 ReservedSpace rs_nvdimm = rs.last_part(MaxHeapSize);*/<br>
<br>
236 /* Toggle HeteroHeap<br>
237 _start_index_of_nvdimm = (uint)(rs_dram.size() /
alloc_granularity);<br>
238 _start_index_of_dram = 0; */<br>
<br>
246 /* Toggle HeteroHeap<br>
247 uint num_nvdimm = end_idx >= _start_index_of_nvdimm ?
MIN2((end_idx - _start_index_of_nvdimm + 1), (uint)num_regions) : 0;<br>
248 uint num_dram = (uint)num_regions - num_nvdimm;*/<br>
<br>
257 /* Toggle HeteroHeap<br>
258 _dram_mapper->commit_regions(start_idx, num_dram,
pretouch_gang); */<br>
<br>
266 /* Toggle HeterHeap<br>
267 uint num_nvdimm = end_idx >= _start_index_of_nvdimm ?
MIN2((end_idx - _start_index_of_nvdimm + 1), (uint)num_regions) : 0;<br>
268 uint num_dram = (uint)num_regions - num_nvdimm;*/<br>
- There are some commentted out blocks, please remove them.<br>
<br>
173 void
G1RegionToHeteroSpaceMapper::map_nvdimm_space(ReservedSpace rs) {<br>
- 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.<br>
- 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'.<br>
<br>
177 vm_exit_during_initialization(<br>
- 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.<br>
<br>
178 err_msg("Could not create file for Old generation at
location %s", AllocateOldGenAt));<br>
179 }<br>
- Include "utilities/formatBuffer.hpp" to avoid compile error in
Solaris because of 'err_msg()'.<br>
<br>
182 //char* ret =
os::replace_existing_mapping_with_file_mapping(rs.base(), rs.size(),
_backing_fd);<br>
- Please remove this line.<br>
<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/g1RegionToSpaceMapper.hpp<br>
114 uint num_committed_dram();<br>
115 uint num_committed_nvdimm();<br>
- Add 'const'?<br>
<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/heapRegionManager.hpp<br>
<br>
<br>
74 protected:<br>
75 G1HeapRegionTable _regions;<br>
<br>
76 G1RegionToSpaceMapper* _heap_mapper;<br>
77 private:<br>
78 G1RegionToSpaceMapper* _prev_bitmap_mapper;<br>
79 G1RegionToSpaceMapper* _next_bitmap_mapper;<br>
80 G1RegionToSpaceMapper* _bot_mapper;<br>
81 G1RegionToSpaceMapper* _cardtable_mapper;<br>
82 G1RegionToSpaceMapper* _card_counts_mapper;<br>
83 <br>
84 protected:<br>
85 FreeRegionList _free_list;<br>
86 private:<br>
87 <br>
88 // Each bit in this bitmap indicates that the corresponding
region is available<br>
89 // for allocation.<br>
90 CHeapBitMap _available_map;<br>
91 <br>
92 // The number of regions committed in the heap.<br>
93 uint _num_committed;<br>
94 <br>
95 // Internal only. The highest heap region +1 we allocated a
HeapRegion instance for.<br>
96 uint _allocated_heapregions_length;<br>
97 <br>
98 HeapWord* heap_bottom() const { return
_regions.bottom_address_mapped(); }<br>
99 HeapWord* heap_end() const {return
_regions.end_address_mapped(); }<br>
100 <br>
101 protected:<br>
102 void make_regions_available(uint index, uint num_regions = 1,
WorkGang* pretouch_gang = NULL);<br>
103 void uncommit_regions(uint index, size_t num_regions = 1);<br>
104 private:<br>
- How about regrouping instead of adding bunch of access specifiers?<br>
<br>
128 #else<br>
129 // Returns whether the given region is available for
allocation.<br>
130 protected:<br>
131 bool is_available(uint region) const;<br>
132 #endif<br>
- This seems weird. Why is_available(uint) const and 'protected:'
have a new condition of !ASSERT?<br>
<br>
208 // Return maximum number of regions that heap can expand to.<br>
209 virtual uint max_expandable_length() const { return
(uint)_regions.length(); }<br>
- It would be better to explain the difference with max_capacity().<br>
<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/heapRegionType.hpp<br>
<br>
79 PreMatureOldTag = PreMatureOldMask,<br>
80 // Archive regions are regions with immutable content (i.e.
not reclaimed, and<br>
- Please add a new line between 79 and 80.<br>
<br>
<br>
========================================================<br>
src/hotspot/share/runtime/globals.hpp<br>
<br>
2612 experimental(uintx, G1YoungExpansionBufferPerc,
10, \<br>
2613 "When heterogenous heap is enabled by
AllocateOldGenAt " \<br>
2614 "option, after every GC, yg gen is re-sized
which involves " \<br>
2615 "system calls to commit/uncommit memory. To
reduce these " \<br>
2616 "calls, we keep a buffer of extra regions to
absorb small " \<br>
2617 "changes in yg gen length. This flag takes the
buffer " \<br>
2618 "size as an percentage of young gen length")<br>
- Add 'range(0, 100)'<br>
- yg gen -> young gen<br>
- This G1 specific option should be moved to g1_globals.hpp.<br>
<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.cpp<br>
<br>
34 // expand_by() is called to grow the heap. We grow into nvdimm
now.<br>
35 // Dram regions are committed later as needed during mutator
region allocation or <br>
36 // when young list target length is determined after gc cycle.<br>
37 uint HeapRegionManagerForHeteroHeap::expand_by(uint
num_regions, WorkGang* pretouch_workers) {<br>
38 uint num_expanded = expand_nvdimm(MIN2(num_regions,
max_expandable_length() - total_regions_committed()),
pretouch_workers);<br>
39 assert(total_regions_committed() <=
max_expandable_length(), "must be");<br>
40 return num_expanded;<br>
41 }<br>
- 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)<br>
<br>
50 num_regions = MIN2(num_regions, max_expandable_length() -
total_regions_committed());<br>
- Style. I would prefer to have a new variable instead of reusing.<br>
<br>
68 #ifdef ASSERT<br>
69 uint total_committed_before = total_regions_committed();<br>
70 #endif<br>
71 uint can_be_made_available =
shrink_nvdimm(to_be_made_available);<br>
- There is one more space at line 69.<br>
<br>
225 while (cur >= start_idx && (is_available(cur)
&& at(cur)->is_empty())) {<br>
- Redundant parenthesis set before is_available(cur).<br>
<br>
239 HeapRegion*
HeapRegionManagerForHeteroHeap::allocate_free_region(bool is_old) {<br>
- 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().<br>
- 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.<br>
<br>
<br>
========================================================<br>
src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.hpp<br>
<br>
40 uint _max_regions;<br>
- Add 'const'?<br>
<br>
111 // Override. Expand in nv-dimm.<br>
- Style comment. I would prefer adding 'virtual' instead of override
comment. There are more if you agree to change.<br>
<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC4893E@ORSMSX116.amr.corp.intel.com">
<div class="WordSection1">
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoPlainText">Testing : Passed tier1_gc and
tier1_runtime jtret tests.<o:p></o:p></p>
<p class="MsoPlainText"> 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.</p>
</div>
</blockquote>
As mentioned at the beginning, please test without the new option as
well.<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC4893E@ORSMSX116.amr.corp.intel.com">
<div class="WordSection1">
<p class="MsoPlainText"><o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks<o:p></o:p></p>
<p class="MsoNormal">Kishor<o:p></o:p></p>
</div>
</blockquote>
<br>
</body>
</html>