RFR(M): 8190828: Test plan: JEP 8171181: Support heap allocation on alternative memory devices

Sat Nov 18 00:52:33 UTC 2017

Hi Sagheon,

I have new webrev at - http://cr.openjdk.java.net/~kkharbas/8190980/webrev.03
and my reply inline.

Thank you!
From: sangheon.kim [mailto:sangheon.kim at oracle.com]
Sent: Friday, November 17, 2017 3:41 PM
To: Kharbas, Kishor <kishor.kharbas at intel.com>; hotspot-gc-dev at openjdk.java.net
Subject: Re: RFR(M): 8190828: Test plan: JEP 8171181: Support heap allocation on alternative memory devices

Hi Kishor,

On 11/17/2017 02:59 PM, Kharbas, Kishor wrote:
Hi Sangheon!
Thanks for the reply. Please find my reply inline.

From: sangheon.kim [mailto:sangheon.kim at oracle.com]
Sent: Thursday, November 16, 2017 10:28 PM
To: Kharbas, Kishor <kishor.kharbas at intel.com><mailto:kishor.kharbas at intel.com>; hotspot-gc-dev at openjdk.java.net<mailto:hotspot-gc-dev at openjdk.java.net>
Subject: Re: RFR(M): 8190828: Test plan: JEP 8171181: Support heap allocation on alternative memory devices

Hi Kishor,
On 11/13/2017 03:51 PM, Kharbas, Kishor wrote:
Hi!

I have developed a test plan for the implementation of 8171181.
I would appreciate a review and further guidance from the gc-dev members. I am hoping to get everything done well before 18.3 code freeze (have a vacation planned during that time).

Test plan: https://bugs.openjdk.java.net/browse/JDK-8190828
Test webrev: http://cr.openjdk.java.net/~kkharbas/8190980/webrev.01/<http://cr.openjdk.java.net/%7Ekkharbas/8190980/webrev.01/>
Looking at the comment at 8190980, webrev.2 seems the latest one, so my comments are for the webrev.2.

--------------------------------------
test/hotspot/jtreg/gc/TestAllocateHeapAtMultiple.java

  51     String[] extraOptsList = new String[] {
  52       "-Xmx32m -Xms32m -XX:+UseCompressedOops",     // 1. With compressedoops enabled.
  53       "-Xmx32m -Xms32m -XX:-UseCompressedOops",     // 2. With compressedoops disabled.
  54       "-Xmx32m -Xms32m -XX:HeapBaseMinAddress=3g",  // 3. With user specified HeapBaseMinAddress.
  55       "-Xmx4g -Xms4g",                              // 4. With larger heap size (UnscaledNarrowOop not possible).
  56       "-Xmx4g -Xms4g -XX:+UseLargePages",           // 5. Set UseLargePages.
  57       "-Xmx4g -Xms4g -XX:+UseNUMA"                  // 6. Set UseNUMA.
  58     };
- I think we do differently to run sub-tests. Maybe SQE folks would give better comment on this.
e.g. TestAllocationInEden.java
 * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions
...
 *                   TestAllocationInEden 10m 9 EDEN
 * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions
...
 *                   TestAllocationInEden 10m 47 EDEN
 * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions

[Kharbas, Kishor] In my tests I use stdout to check for log generated by -Xlog:heap+gc=info to test correct allocation of Heap. That's why I need to spawn a process from within this test to get a handle on stdout, which (as per my limited knowledge) is not possible if I use @run. Please correct me if I am wrong.
Please keep as is.
I think handling stdout seems okay with @run approach but @run doesn't allow to add more vm options. In this test, we need 'test_dir', so we can't use @run.

[Kharbas, Kishor] Ok. Thanks

52       "-Xmx32m -Xms32m -XX:+UseCompressedOops",     // 1. With compressedoops enabled.
54       "-Xmx32m -Xms32m -XX:HeapBaseMinAddress=3g",  // 3. With user specified HeapBaseMinAddress.
55       "-Xmx4g -Xms4g",                              // 4. With larger heap size (UnscaledNarrowOop not possible).
- I think these 3 sub-tests are testing different compressed oop modes. I would recommend to include other 1 type(non-zero based) as well. In addition, adding the comment also would help increase the readability.

[Kharbas, Kishor] Yes I can do that. I need to specify heap size close to 32 GB right? There should be that much disk space available in the test environment, do you think that would be problem?
No need so huge heap. Please refer UseComporessedOops.java for an example of each cases.
universe.hpp:378 also has good explanation.
e.g. -Xmx32m -XX:HeapBaseMinAddress=0x800000008 also uses non-zero based.

[Kharbas, Kishor] I added this sub-test, used the options from UseCompressedOops.java for same non-zero base, unscaled case.

--------------------------------------
test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithAllocateHeapAt.java

1) This seems identical to TestGCBasherWithG1.java, how about just adding another '@run'?
i.e.  adding "* @run main/othervm/timeout=500 -Xlog:gc*=info -Xmx256m -server -XX:+UseG1GC  -XX:AllocateHeapAt=. TestGCBasherWithG1 120000"

2) Don't we need testing for other GC types as well? i.e. Serial, Parallel and CMS.

[Kharbas, Kishor] The idea was to exercise the Java heap (allocated on a file) by some stress test. I thought one GC option would suffice, do you think we need more?

Let's leave as is.

[Kharbas, Kishor] Ok Thanks

2  * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
- Is this intended to start from 2016 as this seems to be copied from TestGCBasherWithXXX.java?

[Kharbas, Kishor] I will change this.
OK.
But, if you agree to go 1) option(adding @run, instead of making TestGCBasherWithAllocateHeapAt.java, this comment can be ignored.

[Kharbas, Kishor] I feel it's better to keep this separate so as to not give an impression that AllocateHeapAt is tied to G1GC. Change the copyright.

34  * @run main/othervm/timeout=500 -Xlog:gc*=info -Xmx256m -server -XX:+UseG1GC -XX:AllocateHeapAt=. TestGCBasherWithAllocateHeapAt 120000
- Are there any reason to use timeout of 500? TestGCBasherWithG1 is using 200ms.
[Kharbas, Kishor] I increased the timeout since heap is mapped to a file on disk (either HDD or SSD depending on test environment) which makes the test run slower.
Make sense to have longer timeout, not sure additional 300ms is good enough though.

[Kharbas, Kishor] I my environment even 200s did not fail, but I observed it taking longer. Whole or part of file is paged in memory so run time is not very bad.

At least when I ran your 8190308(webrev.15) with this webrev.2, all tier1~tier5 tests were okay. There were some failures but those are known issues.

Thanks,
Sangheon

Thanks,
Sangheon

JEP: https://bugs.openjdk.java.net/browse/JDK-8171181
Implementation webrev : http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15/<http://cr.openjdk.java.net/%7Ekkharbas/8190308/webrev.15/>

Thank you!
Kishor

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20171118/4ef06486/attachment.htm>