<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Kishor,<br>
<br>
<div class="moz-cite-prefix">On 10/19/2017 08:00 AM, Kharbas, Kishor
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3901C8AB3@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:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@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;}
@font-face
{font-family:"Courier New \;color\:black";
panose-1:0 0 0 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
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;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:"Courier New";}
span.EmailStyle20
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle21
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle23
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:140586095;
mso-list-type:hybrid;
mso-list-template-ids:1740825630 1003399146 67698691 67698693 67698689 67698691 67698693 67698689 67698691 67698693;}
@list l0:level1
{mso-level-start-at:137;
mso-level-number-format:bullet;
mso-level-text:-;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:.75in;
text-indent:-.25in;
font-family:"Times New Roman",serif;
mso-fareast-font-family:"Times New Roman";}
@list l0:level2
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:1.25in;
text-indent:-.25in;
font-family:"Courier New";}
@list l0:level3
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:1.75in;
text-indent:-.25in;
font-family:Wingdings;}
@list l0:level4
{mso-level-number-format:bullet;
mso-level-text:\F0B7;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:2.25in;
text-indent:-.25in;
font-family:Symbol;}
@list l0:level5
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:2.75in;
text-indent:-.25in;
font-family:"Courier New";}
@list l0:level6
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:3.25in;
text-indent:-.25in;
font-family:Wingdings;}
@list l0:level7
{mso-level-number-format:bullet;
mso-level-text:\F0B7;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:3.75in;
text-indent:-.25in;
font-family:Symbol;}
@list l0:level8
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:4.25in;
text-indent:-.25in;
font-family:"Courier New";}
@list l0:level9
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:4.75in;
text-indent:-.25in;
font-family:Wingdings;}
@list l1
{mso-list-id:1688947326;
mso-list-type:hybrid;
mso-list-template-ids:-1134396328 67698703 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l1:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l1:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l1:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l2
{mso-list-id:2028287743;
mso-list-type:hybrid;
mso-list-template-ids:-1340298278 67698703 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l2:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l2:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l2:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></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"><span style="color:#1F497D">Hi Sangheon,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Thanks for the
review and comments.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">I created two
webrevs:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">webrev.07 : Is
the rebase of webrev.06 on jdk10 (</span><span
style="color:#1F497D"><a
href="http://cr.openjdk.java.net/%7Ekkharbas/8171181/webrev.07/"
moz-do-not-send="true">http://cr.openjdk.java.net/~kkharbas/8171181/webrev.07/</a>)</span><span
style="color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">webrev.08 : Is
incremental webrev over 07. (</span><span
style="color:#1F497D"><a
href="http://cr.openjdk.java.net/%7Ekkharbas/8171181/webrev.08/"
moz-do-not-send="true">http://cr.openjdk.java.net/~kkharbas/8171181/webrev.08/</a>)</span></p>
</div>
</blockquote>
1. I couldn't apply webrev.07 patch cleanly. Could you test? <br>
Just minor nit: 07 seems not only rebase of webrev.06. i.e.
there seems to have some changes from my comment of alignments.<br>
2. The description in the JEP uses 'AllocateHeapAt' while the patch
uses 'HeapDir'.<br>
<br>
-----------------------------<br>
src/os/posix/vm/os_posix.cpp<br>
159 (void)strcpy(fullname, dir);<br>
- Please use strncpy instead.<br>
<br>
160 (void)strcat(fullname, name_template);<br>
- Please use strncat instead.<br>
<br>
180 ::free(fullname);<br>
- os::free().<br>
<br>
196 ::free(fullname);<br>
- os::free().<br>
<br>
-----------------------------<br>
src/os/windows/vm/os_windows.cpp<br>
2885 char *fullname = (char*)_alloca(strlen(dir) +
sizeof(name_template));<br>
- Missing allocation failure handling.<br>
- Please use _malloca instead.<br>
* This function is deprecated because a more secure version is
available; see _malloca.<br>
<a class="moz-txt-link-freetext" href="https://msdn.microsoft.com/en-us/library/wb1s57t5.aspx">https://msdn.microsoft.com/en-us/library/wb1s57t5.aspx</a><br>
<br>
2886 (void)strcpy(fullname, dir);<br>
- Please use strncpy instead.<br>
<br>
2887 (void)strcat(fullname, name_template);<br>
- Please use strncat instead.<br>
<br>
-----------------------------<br>
src/share/vm/runtime/arguments.cpp<br>
3726 }<br>
<br>
<br>
3727 }<br>
- My previous comment was to add explicit explanation of disabling
UseNUMA and UseNUMAInterleaving.<br>
<br>
<br>
4636 if(!FLAG_IS_DEFAULT(HeapDir)) {<br>
- Previsouly we checked UseNUMA and UseNUMAInterleaving and then
disabled those. IMHO, I think previous one seems better with more
explanation that we are going to disable those options. i.e. Similar
to os_linux.cpp:4869, a warning message with disabling codes.<br>
<br>
4638 }<br>
4639 else if (UseParallelGC || UseParallelOldGC) {<br>
- One line please if this change is still needed. <br>
-> } else if {<br>
<br>
-----------------------------<br>
src/share/vm/runtime/os.cpp<br>
1678<br>
- Extra line.<br>
<br>
1688 }<br>
1689 else {<br>
-> } else {<br>
<br>
My answers are inlined.<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3901C8AB3@ORSMSX116.amr.corp.intel.com">
<div class="WordSection1">
<p class="MsoNormal"><span style="color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">In webrev08 I
have addressed all the comments, except for ones below:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">---------------------------------------<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">src/os/aix/vm/os_aix.cpp<br>
<br>
2514 char* os::pd_attempt_reserve_memory_at(size_t bytes,
char* requested_addr, bool use_SHM) {<br>
- Question. Why os_aix has additional parameter at
pd_attemp_reserve_memory_at()? Probably only AIX has shmated
memory implementation?<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"> A: Yes that’s correct.</span></p>
</div>
</blockquote>
Okay.<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3901C8AB3@ORSMSX116.amr.corp.intel.com">
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p></o:p></span></p>
<p class="MsoNormal" style="margin-left:5.25pt"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">--------------------------------------<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">137 log_debug(gc, heap,
coops)("UseLargePages can't be set with HeapDir option.");<br>
- Is it enough to print log message instead of warning
message? i.e. Don't we need something at arguments.cpp:3656,
similar to NUMA flags?<br>
A: If don’t disable UseLargePages like UseNUMA
because large pages can be used for other allocation such as
CodeCache.</span></p>
</div>
</blockquote>
I meant to changing log_debug() to log_warning().<br>
If UseNUMA is enabled, we print warning at arguments.cpp:3725 while
UseLargePages is enabled, we print debug message.<br>
In addition, is this enough? Don't we need to force disabling
UseLargePages?<br>
What happens if both options are enabled?<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3901C8AB3@ORSMSX116.amr.corp.intel.com">
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">------------------------------------<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">603
ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
alignment, bool large, const char* backing_fs_for_heap)<br>
- ReservedSpace has '_backing_fd' but the constructor
doesn't take it as a parameter and only ReservedHeapSpace
uses it. This seems not ideal, couldn't make it better? I
know actual logic is at ReservedSpace so it is not
convenient to add _backing_fs_for_heap at ReservedHeapSapce.<br>
A: ReservedHeapSpace depends on few functions in
ReservedSpace such as initialize(), release(). So instead of
passing it as argument, I set it as a propert of
ReservedSpace.</span></p>
</div>
</blockquote>
Okay.<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3901C8AB3@ORSMSX116.amr.corp.intel.com">
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">-----------------------------------<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">1663<br>
- You removed os::attempt_reserve_memory_at() from os.cpp
and split into os_posix.cpp and os_windows.cpp.<br>
But I think you should remain
os::attempt_reserve_memory_at() at os.cpp and implement os
specific stuffs at each os_xxx.cpp files for pd_xxx. Of
cource move MemTracker function call as well.<br>
</span><span style="color:#1F497D"> A: I do it this way
to reduce the redundant code, If I implement in OS specific
files in pd_xxx(), the code to replace reserved mapping with
file mapping
(replace_existing_mapping_with_dax_file_mapping()) will be
redundant.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">
Still if you feel I will do the change and see how it looks.</span></p>
</div>
</blockquote>
I would prefer to have pd_xxx() even though there's some redundant
code.<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3901C8AB3@ORSMSX116.amr.corp.intel.com">
<div class="WordSection1">
<p class="MsoNormal"><span style="color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Regards<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Kishor<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"
moz-do-not-send="true"><span style="color:#1F497D"><o:p> </o:p></span></a></p>
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="color:windowtext">From:</span></b><span
style="color:windowtext"> sangheon.kim [<a
class="moz-txt-link-freetext"
href="mailto:sangheon.kim@oracle.com">mailto:sangheon.kim@oracle.com</a>]
<br>
<b>Sent:</b> Tuesday, September 26, 2017 3:18 PM<br>
<b>To:</b> Kharbas, Kishor <a
class="moz-txt-link-rfc2396E"
href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>;
'<a class="moz-txt-link-abbreviated"
href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>'
<a class="moz-txt-link-rfc2396E"
href="mailto:hotspot-gc-dev@openjdk.java.net"><hotspot-gc-dev@openjdk.java.net></a><br>
<b>Subject:</b> Re: RFR(M): 8171181: Supporting heap
allocation on alternative memory devices<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal" style="margin-bottom:12.0pt">Hi Kishor,<span
style="font-size:12.0pt"><o:p></o:p></span></p>
<div>
<p class="MsoNormal">On 07/20/2017 06:34 PM, Kharbas, Kishor
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="color:#1F497D">I have a new
version of this patch at <a
href="http://cr.openjdk.java.net/%7Ekkharbas/8171181/webrev.06/"
moz-do-not-send="true">http://cr.openjdk.java.net/~kkharbas/8171181/webrev.06/</a></span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">This version
has been tested on Windows, Linux, Solaris and Mac OS. I
could not get access to AIX for testing.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">I used tmpfs
to test the functionality. Cases that were tested were.</span><o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l2 level1 lfo2"><!--[if !supportLists]--><span
style="mso-list:Ignore">1.<span style="font:7.0pt
"Times New Roman""> </span></span><!--[endif]--><span
style="color:#1F497D">Allocation of heap using file
mapping when –XX:HeapDir= option is used.</span><o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l2 level1 lfo2"><!--[if !supportLists]--><span
style="mso-list:Ignore">2.<span style="font:7.0pt
"Times New Roman""> </span></span><!--[endif]--><span
style="color:#1F497D">Creation of nameless temporary file
for Heap allocation which prevents access to file using
its name.</span><o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l2 level1 lfo2"><!--[if !supportLists]--><span
style="mso-list:Ignore">3.<span style="font:7.0pt
"Times New Roman""> </span></span><!--[endif]--><span
style="color:#1F497D">Correct deletion and freeing up of
space allocated for file under different exit conditions.</span><o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l2 level1 lfo2"><!--[if !supportLists]--><span
style="mso-list:Ignore">4.<span style="font:7.0pt
"Times New Roman""> </span></span><!--[endif]--><span
style="color:#1F497D">Error handling when path specified
is not present, heap size is more than size of file
system, etc.</span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">Overall seems good.<br>
I tried to list some missing part.<br>
<br>
1. Please rebase with consolidated repository. (jdk10/hs)<br>
2. Build failure on Solaris.<br>
(Sorry no build error log file, as I tested a few weeks
ago, it is deleted)<br>
3. Have you tested with various heap reserve path using
HeapBaseMinAddress flag? i.e. to test code path of
ReservedHeapSpace::try_reserve_heap() and
try_reserve_range().<br>
4. How about adding HeapDir allocation success message? e.g.
gc+heap+coops=info<br>
5. Adding JTREG test. Probably we would need to verify this
allocation is succeeded via #4 added allocation success
message.<br>
6. CSR (Compatibility & Specification Review). At some
point, you need to file another CR of 'CSR' type to add a
new flag of 'HeapDir'.<br>
7. It will be much appreciated if you provide incremental
webrev. I think 06(this version) vs. 07(rebase version)
would be hard to get. Probably from 08 version.<br>
<br>
Here's my comments.<br>
-----------------------------<br>
src/os/aix/vm/os_aix.cpp<br>
<br>
2514 char* os::pd_attempt_reserve_memory_at(size_t bytes,
char* requested_addr, bool use_SHM) {<br>
- Question. Why os_aix has additional parameter at
pd_attemp_reserve_memory_at()? Probably only AIX has shmated
memory implementation?<br>
<br>
-----------------------------<br>
src/os/posix/vm/os_posix.cpp<br>
<br>
148 char *fullname = (char*)::malloc(strlen(dir) +
sizeof(name_template));<br>
- Use os::malloc()<br>
<br>
196 int flags;<br>
197 <br>
198 flags = MAP_PRIVATE | MAP_NORESERVE | MAP_ANONYMOUS;<br>
- Combining 196 and 198 seems better to me.<br>
<br>
200 assert((uintptr_t)requested_addr %
os::Linux::page_size() == 0, "unaligned address");<br>
- Linux dependency on posix file which makes build error on
Solaris. Probably os::vm_page_size().<br>
<br>
207 addr = (char*)::mmap(requested_addr, bytes, PROT_NONE,<br>
208 flags, -1, 0);<br>
- Missing some spaces? Alignment seems odd to me.<br>
<br>
226 if (ret == -1)<br>
- Probably you wanted to add handling code? If not, just
return ret.<br>
<br>
252 if (addr == MAP_FAILED || (base != NULL &&
addr != base)) {<br>
253 if (addr != MAP_FAILED) {<br>
254 if (!os::release_memory(addr, size)) {<br>
255 warning("Could not release memory on
unsuccessful file mapping");<br>
256 }<br>
257 }<br>
258 return NULL;<br>
259 }<br>
- Splitting MAP_FAILED case and another gives better
readability to me. But this is your call.<br>
<br>
269 <br>
- Extra line.<br>
<br>
284 if (result != NULL && file_desc != -1) {<br>
285 if
(replace_existing_mapping_with_dax_file_mapping(result,
bytes, file_desc) == NULL) {<br>
286 vm_exit_during_initialization(err_msg("Error in
mapping Java heap at the given filesystem directory"));<br>
287 }<br>
288
MemTracker::record_virtual_memory_reserve_and_commit((address)result,
bytes, CALLER_PC);<br>
289 return result;<br>
290 }<br>
291 if (result != NULL) {<br>
292
MemTracker::record_virtual_memory_reserve((address)result,
bytes, CALLER_PC);<br>
293 }<br>
- Combining line 284 and 291 seems better to me.<br>
284 if (result != NULL) {<br>
if (file_desc != -1) {<br>
if
(replace_existing_mapping_with_dax_file_mapping(result,
bytes, file_desc) == NULL) {<br>
vm_exit_during_initialization(err_msg("Error in
mapping Java heap at the given filesystem directory"));<br>
}<br>
MemTracker::record_virtual_memory_reserve_and_commit((address)result,
bytes, CALLER_PC);<br>
} else {<br>
MemTracker::record_virtual_memory_reserve((address)result,
bytes, CALLER_PC);<br>
}<br>
}<br>
return result;<br>
<br>
-----------------------------<br>
src/os/windows/vm/os_windows.cpp<br>
3141 // if 'base' is not NULL, function will return NULL if
it cannot get 'base'<br>
- Start with uppercase.<br>
<br>
3142 //<br>
- This line seems redundant.<br>
<br>
3151 vm_exit_during_initialization(err_msg("Could not
allocate sufficient disk space for heap"));<br>
- heap -> Java heap (same as line 3153)<br>
<br>
3168 assert(base != NULL, "base cannot be NULL");<br>
- 'base' -> 'Base' or 'Base address'.<br>
<br>
3172 <br>
- Redundant line.<br>
<br>
3230 }<br>
3231 else {<br>
-> } else {<br>
<br>
3278 return reserve_memory(bytes, requested_addr, 0);<br>
- Is it correct to use '0' not '-1'? It would be better to
explain why we use hard-coded value here.<br>
<br>
-----------------------------<br>
src/share/vm/memory/universe.cpp<br>
- No comments<br>
<br>
-----------------------------<br>
src/share/vm/memory/virtualspace.cpp<br>
- copyright update<br>
<br>
74 const size_t
size, bool special, bool is_file_mapped= false)<br>
- Need space between 'is_file_mapped' and '='.<br>
<br>
92 fatal("os::release_memory failed");<br>
- Typo, 'os::unmap_memory failed'.<br>
<br>
129 // If there is a backing file directory for this
VirtualSpace then whether<br>
- This is not VirtualSpace. Probably just 'space'.<br>
<br>
130 // large pages are allocated is upto the filesystem
the dir resides in.<br>
- 'dir'? Probably 'backing file for Java heap'.<br>
<br>
137 log_debug(gc, heap, coops)("UseLargePages can't be
set with HeapDir option.");<br>
- Is it enough to print log message instead of warning
message? i.e. Don't we need something at arguments.cpp:3656,
similar to NUMA flags?<br>
<br>
191 // unmap_memory will do extra work esp. in
Windows<br>
- esp. -> especially<br>
<br>
282 }<br>
283 else {<br>
-> } else {<br>
<br>
346 // If there is a backing file directory for this
VirtualSpace then whether<br>
- Again this is not VirtualSpace, so just 'space'.<br>
<br>
352 if (UseLargePages &&
(!FLAG_IS_DEFAULT(UseLargePages) ||<br>
353 !FLAG_IS_DEFAULT(LargePageSizeInBytes))) {<br>
- Wrong alignment at line 353. Consider to make same as line
380.<br>
<br>
603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
alignment, bool large, const char* backing_fs_for_heap)<br>
- ReservedSpace has '_backing_fd' but the constructor
doesn't take it as a parameter and only ReservedHeapSpace
uses it. This seems not ideal, couldn't make it better? I
know actual logic is at ReservedSpace so it is not
convenient to add _backing_fs_for_heap at ReservedHeapSapce.<br>
<br>
-----------------------------<br>
src/share/vm/memory/virtualspace.hpp<br>
40 int _backing_fd;<br>
- I would prefer to have better name to describe. <br>
e.g. as command-line option name is 'HeapDir', _heap_fd or
_fd_for_heap(similar to below)?<br>
<br>
115 ReservedHeapSpace(size_t size, size_t
forced_base_alignment, bool large, const char*
backingFSforHeap = NULL);<br>
- Snake case. How about 'fs_for_heap' or 'heap_fs'?<br>
<br>
-----------------------------<br>
src/share/vm/runtime/arguments.cpp<br>
3655 FLAG_SET_CMDLINE(bool, UseNUMA, false);<br>
- (questions) Don't need to add a warning message for
UseLargePagesSame=true as commented virtualspace.cpp:137?<br>
<br>
-----------------------------<br>
src/share/vm/runtime/globals.hpp<br>
- No comments<br>
<br>
-----------------------------<br>
src/share/vm/runtime/os.cpp<br>
<br>
1632 <br>
- Extra line.<br>
<br>
1642 }<br>
1643 else {<br>
-> } else {<br>
<br>
1663<br>
- You removed os::attempt_reserve_memory_at() from os.cpp
and split into os_posix.cpp and os_windows.cpp.<br>
But I think you should remain
os::attempt_reserve_memory_at() at os.cpp and implement os
specific stuffs at each os_xxx.cpp files for pd_xxx. Of
cource move MemTracker function call as well.<br>
<br>
-----------------------------<br>
src/share/vm/runtime/os.hpp<br>
<br>
349 // replace existing reserved memory with file mapping<br>
- Start with uppercase letter.<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<br>
<o:p></o:p></span></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">- Kishor</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<div style="border:none;border-left:solid blue
1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> Kharbas, Kishor <br>
<b>Sent:</b> Tuesday, July 11, 2017 6:40 PM<br>
<b>To:</b> '<a
href="mailto:hotspot-gc-dev@openjdk.java.net"
moz-do-not-send="true">hotspot-gc-dev@openjdk.java.net</a>'
<a href="mailto:hotspot-gc-dev@openjdk.java.net"
moz-do-not-send="true"><hotspot-gc-dev@openjdk.java.net></a><br>
<b>Cc:</b> Kharbas, Kishor <a
href="mailto:kishor.kharbas@intel.com"
moz-do-not-send="true"><kishor.kharbas@intel.com></a><br>
<b>Subject:</b> RFR(M): 8171181: Supporting heap
allocation on alternative memory devices<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Greetings,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">I have an
updated patch for JEP <a
href="https://bugs.openjdk.java.net/browse/JDK-8171181"
moz-do-not-send="true">
https://bugs.openjdk.java.net/browse/JDK-8171181</a>
at <a
href="http://cr.openjdk.java.net/%7Ekkharbas/8171181/webrev.05"
moz-do-not-send="true">
http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05</a></span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">This patch
fixes the bugs pointed earlier and other suggestions to
make the code less intrusive.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">I have also
sent this to ‘hotspot-runtime-dev’ mailing list
(included below).</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">I would
appreciate comments and feedback.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Thanks</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Kishor</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> Kharbas, Kishor <br>
<b>Sent:</b> Monday, July 10, 2017 1:53 PM<br>
<b>To:</b> <a
href="mailto:hotspot-runtime-dev@openjdk.java.net"
moz-do-not-send="true">hotspot-runtime-dev@openjdk.java.net</a><br>
<b>Cc:</b> Kharbas, Kishor <<a
href="mailto:kishor.kharbas@intel.com"
moz-do-not-send="true">kishor.kharbas@intel.com</a>><br>
<b>Subject:</b> RFR(M): 8171181: Supporting heap
allocation on alternative memory devices<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Courier New
;color:black",serif">Hello all!</span><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<pre>I have an updated patch for <a href="https://bugs.openjdk.java.net/browse/JDK-8171181" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8171181</a> at <a href="http://cr.openjdk.java.net/%7Ekkharbas/8171181/webrev.05" moz-do-not-send="true">http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05</a><o:p></o:p></pre>
<p class="MsoNormal">I have lost the old email chain so had
to start a fresh one. The archived conversation can be
found at - <a
href="http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-March/022733.html"
moz-do-not-send="true">
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-March/022733.html</a><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l1 level1 lfo4"><!--[if !supportLists]--><span
style="mso-list:Ignore">1.<span style="font:7.0pt
"Times New Roman""> </span></span><!--[endif]-->I
have worked on all the comments and fixed the bugs. Mainly
bugs fixed are related to sigprocmask() and changed the
implementation such that ‘fd’ is not passed all the way
down the call stack. Thus minimizing function signature
changes.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l1 level1 lfo4"><!--[if !supportLists]--><span
style="mso-list:Ignore">2.<span style="font:7.0pt
"Times New Roman""> </span></span><!--[endif]-->Patch
supports all OS’es. Consolidated all Posix compliant OS’s
implementation in os_posix.cpp.<o:p></o:p></p>
<p class="MsoListParagraph"> <o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l1 level1 lfo4"><!--[if !supportLists]--><span
style="mso-list:Ignore">3.<span style="font:7.0pt
"Times New Roman""> </span></span><!--[endif]-->The
patch is tested on Windows and Linux. Working on testing
it on other OS’es.<o:p></o:p></p>
<p class="MsoListParagraph"> <o:p></o:p></p>
<p class="MsoNormal">Let me know if this version looks clean
and correct.<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>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in
0in 0in 4.0pt">
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
</div>
</div>
</blockquote>
<br>
</body>
</html>