<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 27, 2019, at 6:54 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 25/05/2019 2:02 am, Bob Vandette wrote:<br class=""><blockquote type="cite" class="">Here’s the updated webrev.<br class=""><a href="http://cr.openjdk.java.net/~bobv/8222252/webrev.03" class="">http://cr.openjdk.java.net/~bobv/8222252/webrev.03</a><br class=""> I’ve also updated the CSR (https://bugs.openjdk.java.net/browse/JDK-8223957).<br class=""></blockquote><br class="">Sorry but I'm having some trouble reconciling the code with the general description. I would have expected to see something like this:<br class=""><br class=""> bool override_coop_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=""><br class="">where override_coop_limit might also be called use_physical_memory, or ignore_maxram, if I understand correctly. Then things would simplify to:<br class=""><br class=""> if (override_coop_limit) { // use physical, ignore MaxRAM<br class=""> phys_mem = os::physical_memory();<br class=""> FLAG_SET_ERGO(MaxRAM, (uint64_t)phys_mem);<br class=""> } else { // use MaxRAM<br class=""> phys_mem = FLAG_IS_DEFAULT(MaxRAM) ? MIN2(os::physical_memory(), (julong)MaxRAM)<br class=""> : (julong)MaxRAM;<br class=""> }<br class=""></div></div></blockquote><div><br class=""></div><div>The problem with your implementation is that if the user specifies a MaxRAM value that’s higher than the coop_limit,</div><div>the heap size will be reduced in favor of UseCompressedOops.</div><div><br class=""></div><div>My CSR states "<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><br class=""></div><div><font color="#172b4d" face="DejaVu Sans, sans-serif" class="">My rationale is that if someone goes to the effort of specifying a large MaxRAM value, they</font></div><div><font color="#172b4d" face="DejaVu Sans, sans-serif" class="">don’t want to end up with a different value without knowing it’s happening.</font></div><div><br class=""></div><div><font color="#172b4d" face="DejaVu Sans, sans-serif" class="">If this is not an acceptable behavior, I can simplify my changes to match yours.</font></div><div class=""><font color="#172b4d" face="DejaVu Sans, sans-serif" class=""><br class=""></font></div><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">I’ve filed a bug to create tests for this change (<a href="https://bugs.openjdk.java.net/browse/JDK-8224764" class="">https://bugs.openjdk.java.net/browse/JDK-8224764</a>) which<br class="">I will start working on in parallel with the integration of this change.<br class=""></blockquote><br class="">Given the change is taking somewhat longer than you may have hoped, I hope the test has now progressed. I think it would be good to get everything in together rather than break it up.<br class=""></div></div></blockquote><div><br class=""></div>I split out the test since it can be integrated later in the process. I’m out of the office next week and might not have time to get </div><div>that done before I return.</div><div><br class=""></div><div>Bob.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">Thanks,<br class="">David<br class=""><br class=""><blockquote type="cite" class="">Here are the latest results showing the old and new MaxHeapSize and UseCompressedOops values before<br class="">and after this change.<br class="">./java -XX:MaxRAMPercentage=75 -XX:+PrintFlagsFinal -version | grep MaxHeapSize<br class=""> size_t MaxHeapSize [OLD] = 32178700288 {product} {ergonomic}<br class=""> size_t MaxHeapSize [NEW] = 113883742208 {product} {ergonomic}<br class="">/java -XX:MaxRAMPercentage=75 -XX:+PrintFlagsFinal -version | grep UseCompressedOops<br class=""> bool UseCompressedOops [OLD] = true {lp64_product} {ergonomic}<br class=""> bool UseCompressedOops [NEW] = false {lp64_product} {ergonomic}<br class="">./java -XX:MaxRAM=192g -XX:+PrintFlagsFinal -version | grep MaxHeapSize<br class=""> size_t MaxHeapSize [OLD] = 32178700288 {product} {ergonomic}<br class=""> size_t MaxHeapSize [NEW] = 51539607552 {product} {ergonomic}<br class="">./java -XX:MaxRAM=192g -XX:+PrintFlagsFinal -version | grep UseCompressedOops<br class=""> bool UseCompressedOops [OLD] = true {lp64_product} {ergonomic}<br class=""> bool UseCompressedOops [NEW] = false {lp64_product} {ergonomic}<br class="">./java -XX:MaxRAM=192g -XX:MaxRAMPercentage=75 -XX:+PrintFlagsFinal -version | grep MaxHeapSize<br class=""> size_t MaxHeapSize [OLD] = 32178700288 {product} {ergonomic}<br class=""> size_t MaxHeapSize [NEW] = 154618822656 {product} {ergonomic}<br class="">./java -XX:MaxRAM=192g -XX:MaxRAMPercentage=75 -XX:+PrintFlagsFinal -version | grep UseCompressedOops<br class=""> bool UseCompressedOops [OLD] = true {lp64_product} {ergonomic}<br class=""> bool UseCompressedOops [NEW] = false {lp64_product} {ergonomic}<br class="">./java -XX:MaxRAMPercentage=10 -XX:+PrintFlagsFinal -version | grep MaxHeapSize<br class=""> size_t MaxHeapSize [OLD] = 13744734208 {product} {ergonomic}<br class=""> size_t MaxHeapSize [NEW] = 15183380480 {product} {ergonomic}<br class="">./java -XX:MaxRAMPercentage=10 -XX:+PrintFlagsFinal -version | grep UseCompressedOops<br class=""> bool UseCompressedOops = true {lp64_product} {ergonomic}<br class="">./java -XX:+PrintFlagsFinal -version | grep MaxHeapSize<br class=""> size_t MaxHeapSize = 32178700288 {product} {ergonomic}<br class="">./java -XX:+PrintFlagsFinal -version | grep UseCompressedOops<br class=""> bool UseCompressedOops = true {lp64_product} {ergonomic}<br class="">./java -XX:-UseCompressedOops -XX:+PrintFlagsFinal -version | grep MaxHeapSize<br class=""> size_t MaxHeapSize = 34359738368 {product} {ergonomic}<br class="">./java -XX:-UseCompressedOops -XX:+PrintFlagsFinal -version | grep UseCompressedOops<br class=""> bool UseCompressedOops = false {lp64_product} {command line}<br class="">./java -XX:+UseCompressedOops -XX:MaxRAM=128g -XX:+PrintFlagsFinal -version | grep MaxHeapSize<br class=""> size_t MaxHeapSize = 32178700288 {product} {ergonomic}<br class="">./java -XX:+UseCompressedOops -XX:MaxRAM=128g -XX:+PrintFlagsFinal -version | grep UseCompressedOops<br class=""> bool UseCompressedOops = true {lp64_product} {command line}<br class="">./java -XX:+UseCompressedOops -XX:MaxRAMPercentage=75 -XX:+PrintFlagsFinal -version | grep MaxHeapSize<br class=""> size_t MaxHeapSize = 32178700288 {product} {ergonomic}<br class="">./java -XX:+UseCompressedOops -XX:MaxRAMPercentage=75 -XX:+PrintFlagsFinal -version | grep UseCompressedOops<br class=""> bool UseCompressedOops = true {lp64_product} {command line}<br class="">Bob.<br class=""><blockquote type="cite" class="">On May 23, 2019, at 9:15 AM, Bob Vandette <<a href="mailto:bob.vandette@oracle.com" class="">bob.vandette@oracle.com</a> <<a href="mailto:bob.vandette@oracle.com" class="">mailto:bob.vandette@oracle.com</a>>> wrote:<br class=""><br class=""><br class=""><blockquote type="cite" class="">On May 16, 2019, at 11:34 PM, David Holmes <<a href="mailto:David.Holmes@oracle.com" class="">David.Holmes@oracle.com</a> <<a href="mailto:David.Holmes@oracle.com" class="">mailto:David.Holmes@oracle.com</a>>> wrote:<br class=""><br 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=""></blockquote><br class="">Yes, you are correct. I’ll add MaxRAM to the use_os_mem_limit check. This is the behavior<br class="">I documented in the CSR.<br class=""><br 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.”<br class=""><br class="">I’ll update the patch provided in the CSR as well.<br class=""><br class=""><br class="">Can you take a look and review the CSR for this change?<br class=""><br class="">Thanks,<br class="">Bob.<br class=""><br class=""><br class=""><blockquote type="cite" 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></blockquote><br class=""></blockquote></blockquote></div></div></blockquote></div><br class=""></body></html>