<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<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;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 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;
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;}
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;
color:black;}
span.PlainTextChar
{mso-style-name:"Plain Text Char";
mso-style-priority:99;
mso-style-link:"Plain Text";
font-family:"Calibri",sans-serif;}
span.EmailStyle19
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle20
{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;}
--></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]-->
</head>
<body bgcolor="white" lang="EN-US" link="#0563C1" vlink="#954F72">
<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. Here is an updated webrev-<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><a href="http://cr.openjdk.java.net/~kkharbas/8190308/webrev.12">http://cr.openjdk.java.net/~kkharbas/8190308/webrev.12</a><o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><a href="http://cr.openjdk.java.net/~kkharbas/8190308/webrev.12_to_11">http://cr.openjdk.java.net/~kkharbas/8190308/webrev.12_to_11</a><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 addition to your suggested corrections, I added code to set Linux core dump filter ensuring Heap is dumped correctly when this feature is used. This is follow-up to Jini George’s comment (http://openjdk.5641.n7.nabble.com/RFR-M-8171181-Supporting-heap-allocation-on-alternative-memory-devices-td300109.html#a300450).<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">(some reply is inline)<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<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Kishor<o:p></o:p></span></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"><a name="_____replyseparator"></a><b><span style="color:windowtext">From:</span></b><span style="color:windowtext"> sangheon.kim [mailto:sangheon.kim@oracle.com]
<br>
<b>Sent:</b> Wednesday, November 1, 2017 10:53 PM<br>
<b>To:</b> Kharbas, Kishor <kishor.kharbas@intel.com>; 'hotspot-gc-dev@openjdk.java.net' <hotspot-gc-dev@openjdk.java.net>; hotspot-runtime-dev@openjdk.java.net<br>
<b>Cc:</b> Thomas Schatzl <thomas.schatzl@oracle.com><br>
<b>Subject:</b> Re: RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review<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 10/31/2017 04:53 PM, Kharbas, Kishor wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Greetings,<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">I am continuing the earlier discussion [1] with a revised issue number representing the implementation of the JEP [2].<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">I have addressed the remaining unaddressed comments (copied below) in these webrevs -<o:p></o:p></p>
<p class="MsoNormal"><a href="http://cr.openjdk.java.net/%7Ekkharbas/8190308/webrev.11/">http://cr.openjdk.java.net/~kkharbas/8190308/webrev.11/</a><o:p></o:p></p>
<p class="MsoNormal"><a href="http://cr.openjdk.java.net/%7Ekkharbas/8190308/webrev.11_to_10/">http://cr.openjdk.java.net/~kkharbas/8190308/webrev.11_to_10/</a><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Also, I would appreciate a review of the CSR for the new flag at
<a href="https://bugs.openjdk.java.net/browse/JDK-8190309">https://bugs.openjdk.java.net/browse/JDK-8190309</a>.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman",serif">CSR: Reviewed.<br>
<br>
<br>
<br>
<o:p></o:p></span></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoPlainText">> > - in that same thread there has also been the question about the<o:p></o:p></p>
<p class="MsoPlainText">> > need <o:p></o:p></p>
<p class="MsoPlainText">> > for blocking the signals for the process that has gone unanswered.<o:p></o:p></p>
<p class="MsoPlainText">> > <o:p></o:p></p>
<p class="MsoPlainText"> <o:p></o:p></p>
<p class="MsoPlainText">Removed the blocking of signals.<o:p></o:p></p>
<p class="MsoPlainText"> <o:p></o:p></p>
<p class="MsoPlainText">> > - Arguments::finalize_vm_init_args: maybe at the place where the <o:p></o:p></p>
<p class="MsoPlainText">> > change adds the warning/info message about NUMA support being<o:p></o:p></p>
<p class="MsoPlainText">> > specific <o:p></o:p></p>
<p class="MsoPlainText">> > to the file system; also add the same warning about UseLargePages<o:p></o:p></p>
<p class="MsoPlainText">> > that <o:p></o:p></p>
<p class="MsoPlainText">> > is located elsewhere.<o:p></o:p></p>
<p class="MsoPlainText">> > <o:p></o:p></p>
<p class="MsoPlainText">> > Like "UseXXXX may not be supported in some specific file system <o:p></o:p></p>
<p class="MsoPlainText">> > implementations." - or just ignore as Andrew said in the other<o:p></o:p></p>
<p class="MsoPlainText">> > email thread.<o:p></o:p></p>
<p class="MsoPlainText">> > <o:p></o:p></p>
<p class="MsoPlainText">> > Note that I am not sure that the selected place is the correct<o:p></o:p></p>
<p class="MsoPlainText">> > place <o:p></o:p></p>
<p class="MsoPlainText">> > for such warning about incompatible flags (and maybe <o:p></o:p></p>
<p class="MsoPlainText">> > UseNUMA/UseLargePages should be forcibly disabled here? But then <o:p></o:p></p>
<p class="MsoPlainText">> > again, it does not hurt just having it enabled?).<o:p></o:p></p>
<p class="MsoPlainText">> > <o:p></o:p></p>
<p class="MsoPlainText">> > I will let the runtime team comment as a lot of that argument <o:p></o:p></p>
<p class="MsoPlainText">> > processing changed in jdk9.<o:p></o:p></p>
<p class="MsoPlainText">> > <o:p></o:p></p>
<p class="MsoPlainText">> > Maybe Arguments::check_vm_args_consistency() is better.<o:p></o:p></p>
<p class="MsoPlainText">> > <o:p></o:p></p>
<p class="MsoPlainText">> > There is similar code in Arguments::adjust_after_os()...<o:p></o:p></p>
<p class="MsoPlainText">> > <o:p></o:p></p>
<p class="MsoPlainText"> <o:p></o:p></p>
<p class="MsoPlainText">1. Moved the check from finalize_vm_init_args() <span style="font-size:9.5pt;font-family:Consolas">
to </span>check_vm_args_consistency()<o:p></o:p></p>
<p class="MsoPlainText">2. When UseNUMA flag is true, adaptive logical group chunk resizing is used for Numa aware collectors such as ParallelOld. Just like UseNUMA is disabled in os::inti_2() in Linux when UseLargePages is set, it will be a good idea to disable
UseNUMA when the new option is used.<o:p></o:p></p>
<p class="MsoPlainText">3. I think its ok to leave UseNUMAInterleaving ON as it can be used for other memory areas.<o:p></o:p></p>
<p class="MsoPlainText">4. For the same reason I also do not disable UseLargePages.<o:p></o:p></p>
<p class="MsoPlainText">5. About handling UseLargePages, I thought of handling it the same way as when reserve_memory_special() fails to allocate large pages, i.e. generating a log_debug message.
<o:p></o:p></p>
<p class="MsoPlainText" style="margin-left:1.0in"> <i><span style="font-family:"Courier New"">// failed; try to reserve regular memory below</span></i><o:p></o:p></p>
<p class="MsoPlainText" style="margin-left:1.0in"><i><span style="font-family:"Courier New""> if (UseLargePages && (!FLAG_IS_DEFAULT(UseLargePages) ||</span></i><o:p></o:p></p>
<p class="MsoPlainText" style="margin-left:1.0in"><i><span style="font-family:"Courier New""> !FLAG_IS_DEFAULT(LargePageSizeInBytes))) {</span></i><o:p></o:p></p>
<p class="MsoPlainText" style="margin-left:1.0in"><i><span style="font-family:"Courier New""> log_debug(gc, heap, coops)("Reserve regular memory without large pages");</span></i><o:p></o:p></p>
<p class="MsoPlainText"> <o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoPlainText">> > - here I may probably be speaking wrongly on behalf of the<o:p></o:p></p>
<p class="MsoPlainText">> > runtime <o:p></o:p></p>
<p class="MsoPlainText">> > team, but os.hpp, as an abstraction of the OS should probably not<o:p></o:p></p>
<p class="MsoPlainText">> > have <o:p></o:p></p>
<p class="MsoPlainText">> > platform specific ifdefs in it.<o:p></o:p></p>
<p class="MsoPlainText">> > <o:p></o:p></p>
<p class="MsoNormal">and <o:p></o:p></p>
<p class="MsoPlainText">> > > - You removed os::attempt_reserve_memory_at() from os.cpp and<o:p></o:p></p>
<p class="MsoPlainText">> > > split <o:p></o:p></p>
<p class="MsoPlainText">> > > into os_posix.cpp and os_windows.cpp.<o:p></o:p></p>
<p class="MsoPlainText">> > > But I think you should remain os::attempt_reserve_memory_at()<o:p></o:p></p>
<p class="MsoPlainText">> > > at <o:p></o:p></p>
<p class="MsoPlainText">> > > os.cpp and implement os specific stuffs at each os_xxx.cpp files<o:p></o:p></p>
<p class="MsoPlainText">> > > for <o:p></o:p></p>
<p class="MsoPlainText">> > > pd_xxx. Of cource move MemTracker function call as well.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoPlainText">In the updated webrev, I move the implementation from os_posix.cpp and os_window.cpp to respective pd_xxx functions. No AIX specific ifdef is required now.<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman",serif">Thank you for all changes.<br>
<br>
I have minor nits now:<br>
==============================================<br>
- os***.cpp has some function names which include *dax*. I would prefer not to include it. As you know, Thomas also pointed it.<br>
<br>
</span><span style="font-size:12.0pt;font-family:"Times New Roman",serif;color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><b><i><span style="color:#1F497D">[Kharbas, Kishor] Done.<o:p></o:p></span></i></b></p>
<p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman",serif"><br>
==============================================<br>
src/hotspot/share/runtime/arguments.cpp<br>
2537 if (!FLAG_IS_DEFAULT(UseNUMAInterleaving) || !FLAG_IS_DEFAULT(UseNUMA)) {<br>
- Don't we need to check these 2 flags' value to be true? i.e. if user sets to false, below message will be printed.<br>
<br>
</span><span style="font-size:12.0pt;font-family:"Times New Roman",serif;color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><b><i><span style="color:#1F497D">[Kharbas, Kishor] That’s correct. I changed it such that you check the flag is true and it’s not default. In future if these flags become ‘true’ by default, we may not want to print warning.<o:p></o:p></span></i></b></p>
<p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman",serif"><br>
==============================================<br>
test/hotspot/jtreg/gc/TestAllocateHeapAt.java<br>
- On other discussion, I mentioned to test only for Windows and Linux as the JEP described only about those 2. But without *dax* function names, it seems like not filtering OS seems okay too.<br>
<br>
2 * Copyright (c) 2017, xxx Oracle and/or its affiliates. All rights reserved.<br>
- Please remove 'xxx '.<br>
<br>
47 Collections.addAll(vmOpts, new String[] {"-XX:AllocateHeapAt="+test_dir,<br>
- Add spaces. '+test_dir' -> ' + test_dir'<br>
<br>
49 "-Xmx50m",<br>
50 "-Xms50m",<br>
- You said there were no special reason for 200m(heap size of webrev.10) on other discussion. I would recommend 32m.<br>
<br>
</span><span style="font-size:12.0pt;font-family:"Times New Roman",serif;color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><b><i><span style="color:#1F497D">[Kharbas, Kishor] All done.<o:p></o:p></span></i></b></p>
<p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman",serif"><br>
FYI, I ran hs-tier1 and hs-tier2 with your patch and the result is good. i.e. no new failures.<br>
<br>
</span><span style="font-size:12.0pt;font-family:"Times New Roman",serif;color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><b><i><span style="color:#1F497D">[Kharbas, Kishor] That’s great. Thanks!<o:p></o:p></span></i></b></p>
<p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman",serif"><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="MsoPlainText"> <o:p></o:p></p>
<p class="MsoPlainText"> <o:p></o:p></p>
<p class="MsoPlainText">Thank you and looking forward for your feedback.<o:p></o:p></p>
<p class="MsoPlainText">- Kishor<o:p></o:p></p>
<p class="MsoPlainText"> <o:p></o:p></p>
<p class="MsoNormal">[1] <a href="http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-October/020682.html">
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-October/020682.html</a><o:p></o:p></p>
<p class="MsoNormal">[2] <a href="https://bugs.openjdk.java.net/browse/JDK-8171181">
https://bugs.openjdk.java.net/browse/JDK-8171181</a><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:12.0pt;font-family:"Times New Roman",serif"><o:p> </o:p></span></p>
</div>
</div>
</body>
</html>