Re: RFR: 8204173: Lower the minimum number of heap memory pools in MemoryTest.java
On 5/31/18 11:14 AM, Stefan Karlsson wrote: Right. The ZGC patch has a @requires vm.gc != "Z" row in MemoryTest.java. Ah. This change is fine then. Mandy
Re: RFR: 8204173: Lower the minimum number of heap memory pools in MemoryTest.java
On 2018-05-31 20:09, mandy chung wrote: On 5/31/18 10:36 AM, Stefan Karlsson wrote: Hi Mandy, On 2018-05-31 18:19, mandy chung wrote: Hi Stefan, This change looks okay. Can you add a comment to describe the expected memory pools for ZGC so that it explains why the min number of heap memory pools is 1. I think this test and possibly other memory pool tests should be re-examined and determine what proper verification should be done, for example, expected heap memory pools for a specific GC. It may be worth to file a RFE to follow up. What if I change the code like this: http://cr.openjdk.java.net/~stefank/8204173/webrev.02/ and then later add a new MemoryTestZGC.sh test, that calls MemoryTest 1 1 and explains that ZGC has one memory manager and one heap memory pool HotSpot nightly testing used to rotate among different GC configuration. MemoryTest.java would still fail if the test is running with ZGC when the VM option is specified via jtreg -vmoption configured for automated testing. Do you need to concern that case? If so, this version won't work. Right. The ZGC patch has a @requires vm.gc != "Z" row in MemoryTest.java. MemoryTestAllGC.sh is already filtered with @requires vm.gc=="Parallel" | vm.gc=="null". It's okay with me to follow up this in a separate issue. Maybe we have to dynamically set up the expected values if there is a way to determine which collector is running. OK. Thanks, StefanK Mandy
Re: RFR: 8204173: Lower the minimum number of heap memory pools in MemoryTest.java
On 5/31/18 10:36 AM, Stefan Karlsson wrote: Hi Mandy, On 2018-05-31 18:19, mandy chung wrote: Hi Stefan, This change looks okay. Can you add a comment to describe the expected memory pools for ZGC so that it explains why the min number of heap memory pools is 1. I think this test and possibly other memory pool tests should be re-examined and determine what proper verification should be done, for example, expected heap memory pools for a specific GC. It may be worth to file a RFE to follow up. What if I change the code like this: http://cr.openjdk.java.net/~stefank/8204173/webrev.02/ and then later add a new MemoryTestZGC.sh test, that calls MemoryTest 1 1 and explains that ZGC has one memory manager and one heap memory pool HotSpot nightly testing used to rotate among different GC configuration. MemoryTest.java would still fail if the test is running with ZGC when the VM option is specified via jtreg -vmoption configured for automated testing. Do you need to concern that case? If so, this version won't work. It's okay with me to follow up this in a separate issue. Maybe we have to dynamically set up the expected values if there is a way to determine which collector is running. Mandy
Re: RFR: 8204173: Lower the minimum number of heap memory pools in MemoryTest.java
Hi Mandy, On 2018-05-31 18:19, mandy chung wrote: Hi Stefan, This change looks okay. Can you add a comment to describe the expected memory pools for ZGC so that it explains why the min number of heap memory pools is 1. I think this test and possibly other memory pool tests should be re-examined and determine what proper verification should be done, for example, expected heap memory pools for a specific GC. It may be worth to file a RFE to follow up. What if I change the code like this: http://cr.openjdk.java.net/~stefank/8204173/webrev.02/ and then later add a new MemoryTestZGC.sh test, that calls MemoryTest 1 1 and explains that ZGC has one memory manager and one heap memory pool? Thanks, StefanK Mandy On 5/31/18 6:53 AM, Stefan Karlsson wrote: Hi all, Please review this patch to lower the minimum number of heap memory pools in MemoryTest.java. http://cr.openjdk.java.net/~stefank/8204173/webrev.01/ https://bugs.openjdk.java.net/browse/JDK-8204173 Just like the comment in the test says: * NOTE: This expected result is hardcoded in this test and this test * will be affected if the heap memory layout is changed in * the future implementation. */ we need to update this test to support ZGC, which only has one heap memory pool. Thanks, StefanK
Re: RFR: 8204173: Lower the minimum number of heap memory pools in MemoryTest.java
Hi Stefan, This change looks okay. Can you add a comment to describe the expected memory pools for ZGC so that it explains why the min number of heap memory pools is 1. I think this test and possibly other memory pool tests should be re-examined and determine what proper verification should be done, for example, expected heap memory pools for a specific GC. It may be worth to file a RFE to follow up. Mandy On 5/31/18 6:53 AM, Stefan Karlsson wrote: Hi all, Please review this patch to lower the minimum number of heap memory pools in MemoryTest.java. http://cr.openjdk.java.net/~stefank/8204173/webrev.01/ https://bugs.openjdk.java.net/browse/JDK-8204173 Just like the comment in the test says: * NOTE: This expected result is hardcoded in this test and this test * will be affected if the heap memory layout is changed in * the future implementation. */ we need to update this test to support ZGC, which only has one heap memory pool. Thanks, StefanK
Re: RFR: 8204173: Lower the minimum number of heap memory pools in MemoryTest.java
Thanks, Erik. StefanK On 2018-05-31 16:26, Erik Österlund wrote: Hi Stefan, Looks good. Thanks, /Erik On 2018-05-31 15:53, Stefan Karlsson wrote: Hi all, Please review this patch to lower the minimum number of heap memory pools in MemoryTest.java. http://cr.openjdk.java.net/~stefank/8204173/webrev.01/ https://bugs.openjdk.java.net/browse/JDK-8204173 Just like the comment in the test says: * NOTE: This expected result is hardcoded in this test and this test * will be affected if the heap memory layout is changed in * the future implementation. */ we need to update this test to support ZGC, which only has one heap memory pool. Thanks, StefanK
Re: RFR: 8204173: Lower the minimum number of heap memory pools in MemoryTest.java
Hi Stefan, Looks good. Thanks, /Erik On 2018-05-31 15:53, Stefan Karlsson wrote: Hi all, Please review this patch to lower the minimum number of heap memory pools in MemoryTest.java. http://cr.openjdk.java.net/~stefank/8204173/webrev.01/ https://bugs.openjdk.java.net/browse/JDK-8204173 Just like the comment in the test says: * NOTE: This expected result is hardcoded in this test and this test * will be affected if the heap memory layout is changed in * the future implementation. */ we need to update this test to support ZGC, which only has one heap memory pool. Thanks, StefanK
