<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On May 16, 2019, at 11:34 PM, David Holmes <<a href="mailto:David.Holmes@oracle.com" class="">David.Holmes@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Hi Bob,<br class=""><br class="">On 16/05/2019 12:52 am, Bob Vandette wrote:<br class=""><blockquote type="cite" class="">I’ve updated the webrev based on your comments.<br class=""><a href="http://cr.openjdk.java.net/~bobv/8222252/webrev.02/" class="">http://cr.openjdk.java.net/~bobv/8222252/webrev.02/</a><br class="">Changes:<br class="">1. MaxRAM is updated if fractional flags cause us to use OS memory size for heap selection.<br class="">2. If MaxRAM is specified with the other fractional based flags, we will use MaxRAM as<br class=""> upper limit instead of OS memory size.<br class=""></blockquote><br class="">That seems fine, but here:<br class=""><br class="">1799 if (FLAG_IS_ERGO(UseCompressedOops) && use_os_mem_limit) {<br class=""><br class="">Shouldn't this also be checking for FLAG_IS_DEFAULT(MaxRAM) ? Or better that check would fold into the use_os_mem_limit determination:<br class=""><br class=""> bool use_os_mem_limit = (!FLAG_IS_DEFAULT(MaxRAMPercentage) ||<br class=""> !FLAG_IS_DEFAULT(MaxRAMFraction) ||<br class=""> !FLAG_IS_DEFAULT(MinRAMPercentage) ||<br class=""> !FLAG_IS_DEFAULT(MinRAMFraction) ||<br class=""> !FLAG_IS_DEFAULT(InitialRAMPercentage) ||<br class=""> !FLAG_IS_DEFAULT(InitialRAMFraction)) &&<br class=""> FLAG_IS_DEFAULT(MaxRAM);<br class=""> if (use_os_mem_limit) {<br class=""> phys_mem = os::physical_memory();<br class=""> FLAG_SET_ERGO(uint64_t, MaxRAM, (uint64_t)phys_mem);<br class=""> } else {<br class=""> phys_mem = FLAG_IS_DEFAULT(MaxRAM) ? MIN2(os::physical_memory(), (julong)MaxRAM)<br class=""> : (julong)MaxRAM;<br class=""> }<br class=""><br class=""></div></div></blockquote><div><br class=""></div>Yes, you are correct. I’ll add MaxRAM to the use_os_mem_limit check. This is the behavior</div><div>I documented in the CSR. </div><div><br class=""></div><div> "<span style="color: rgb(23, 43, 77); font-family: 'DejaVu Sans', sans-serif; background-color: rgb(255, 255, 255);" class="">Also, If any of these flags including MaxRAM are specified on the command line, the selected Java heap size will not be limited by the compressed oop limit.</span><font color="#172b4d" face="DejaVu Sans, sans-serif" class="">”</font></div><div><span style="color: rgb(23, 43, 77); font-family: 'DejaVu Sans', sans-serif; background-color: rgb(255, 255, 255);" class=""><br class=""></span></div><div><div>I’ll update the patch provided in the CSR as well.</div><div class=""><br class=""></div></div><div><br class=""></div><div>Can you take a look and review the CSR for this change?</div><div><br class=""></div><div>Thanks,</div><div>Bob.</div><div><br class=""></div><div><br class=""><blockquote type="cite" class=""><div class=""><div class="">?<br class=""><br class="">Thanks,<br class="">David<br class="">-----<br class=""><br class=""><blockquote type="cite" class="">Here’s a proposed CSR for this behavioral change.<br class=""><a href="https://bugs.openjdk.java.net/browse/JDK-8223957" class="">https://bugs.openjdk.java.net/browse/JDK-8223957</a><br class="">Bob.<br class=""><blockquote type="cite" class="">On May 14, 2019, at 9:26 AM, Thomas Schatzl <thomas.schatzl@oracle.com <mailto:thomas.schatzl@oracle.com>> wrote:<br class=""><br class="">Hi,<br class=""><br class="">On Mon, 2019-05-13 at 10:03 -0400, Bob Vandette wrote:<br class=""><blockquote type="cite" class="">Please review this suggested fix for correcting Heap size selection<br class="">when -XX:{Max,Min,Initial)RAMPercentage and their Factional<br class="">equivalent options are used.<br class=""><br class="">There are two bugs filed that are related to this issue (8222252 and<br class="">8213175)<br class=""><br class="">SUMMARY of FIX:<br class="">The fix corrects what I believe is an incorrect implementation of the<br class="">*Percentage/*Fraction options. These options should not have caused<br class="">the heap size to be fractionally based on MaxRAM or limited by<br class="">MaxRAM. They instead should be based on the host memory. This is<br class="">what I assume was meant by “real memory”.<br class=""><br class="">product(double, MaxRAMPercentage,<br class="">25.0, \<br class=""> "Maximum percentage of real memory used for maximum heap<br class="">size") \<br class=""> range(0.0, 100.0)<br class=""><br class="">When these options are selected, they should take precedence over<br class="">UseCompressedOops unless UseCompressedOops is also specified on the<br class="">command line.<br class=""><br class=""><br class="">WEBREV:<br class="">http://cr.openjdk.java.net/~bobv/8222252/webrev.01<br class=""></blockquote><br class="">- this is a style preference: I would assign the result of the<br class="">condition in the if (arguments.cpp:1721+) to the initialization of<br class="">use_os_mem_limit directly instead of first setting it to false, and<br class="">then to true again if needed. Feel free to ignore.<br class=""><br class="">- as David pointed out, this change needs a CSR.<br class=""><br class="">- the interaction between MaxRAM and MaxRAMPercentage is a bit unclear;<br class="">if they are independent as implemented now, then the update of one<br class="">should probably update the other. Or the VM should fail if they<br class="">disagree?<br class=""><br class="">If they are dependent, if David suggests (i.e. MaxRAMPercentage uses<br class="">MaxRAM as maximum real memory if set; this is what I would tend to<br class="">prefer), then not of course.<br class=""><br class="">- there should be regressions tests verifying this in cases where<br class="">possible.<br class=""><br class="">I.e. it should be doable to get current os::physical_memory using e.g.<br class="">whitebox and then run a few tests verifying the results of MaxHeapSize<br class="">and coop mode.<br class=""><br class="">Particularly the interaction between MaxRAM, MaxRAMPercentage and coops<br class="">mode would be interesting.<br class=""><br class="">Thanks,<br class=""> Thomas<br class=""><br class=""><br class=""></blockquote></blockquote></div></div></blockquote></div><br class=""></body></html>