[PATCH] test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java failed in JITed code
Hi all, My name is Jie Fu, a JVM developer at Loongson working on the MIPS port of OpenJDK. I hope this is the right place to discuss the issue. test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java failed with "java -Xcomp RuntimeThreadInheritanceLeak". The failure was caused by the lost of an OopMap item for "loaderRef" which was optimized out by the JIT with the liveness analysis optimization. For more details, see [1]. It seems that RuntimeThreadInheritanceLeak.java is only suitable for testing interpreters which is not adapted to JITs at all. Different behaviors performed by the interpreter and JITs with the same test case really confused us a lot. And I think it's worth making the test also suitable for JITs. A tiny change can fix this issue --------------------------------- diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java --- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Thu Jan 10 20:49:22 2019 +0800 @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while (loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + } System.err.println( "waiting to be notified of loader being weakly reachable..."); --------------------------------- Could you please review it and tell me if it's OK to file a bug on JBS. Thanks. [1] https://github.com/DamonFool/MyBlog/blob/master/JVM/RuntimeThreadInheritance... Best Regards, Jie
Hi Jie, On 11/01/2019 12:44 am, Jie Fu wrote:
Hi all,
My name is Jie Fu, a JVM developer at Loongson working on the MIPS port of OpenJDK. I hope this is the right place to discuss the issue.
test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java failed with "java -Xcomp RuntimeThreadInheritanceLeak".
The failure was caused by the lost of an OopMap item for "loaderRef" which was optimized out by the JIT with the liveness analysis optimization. For more details, see [1].
It seems that RuntimeThreadInheritanceLeak.java is only suitable for testing interpreters which is not adapted to JITs at all. Different behaviors performed by the interpreter and JITs with the same test case really confused us a lot. And I think it's worth making the test also suitable for JITs.
A tiny change can fix this issue --------------------------------- diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Thu Jan 10 20:49:22 2019 +0800 @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while (loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + }
System.err.println( "waiting to be notified of loader being weakly reachable..."); ---------------------------------
Could you please review it and tell me if it's OK to file a bug on JBS. Thanks.
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp. But by all means file a bug and fix it. Cheers, David
[1] https://github.com/DamonFool/MyBlog/blob/master/JVM/RuntimeThreadInheritance...
Best Regards, Jie
Hi David, Thanks and apologies. This issue was discovered by a broad -Xcomp testing with jtreg on Loongson CPUs (MIPS compatible processors). It was intended to test our MIPS port of OpenJDK. We've found and fixed quite a lot of JIT bugs for our MIPS implementation by this approach. I'll ask Ao Qi to file a bug on JBS and post a webrev soon. Thanks again. Best regards, Jie
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp.
But by all means file a bug and fix it.
Cheers, David
Hi Jie, On 11/01/2019 11:58 am, Jie Fu wrote:
Hi David,
Thanks and apologies.
No apology needed :)
This issue was discovered by a broad -Xcomp testing with jtreg on Loongson CPUs (MIPS compatible processors). It was intended to test our MIPS port of OpenJDK. We've found and fixed quite a lot of JIT bugs for our MIPS implementation by this approach.
Okay, you may well be testing more tests under Xcomp than what we regularly do, so that may well expose a number of tests that may not work at all, or which fail intermittently. I'm trying to find out if there is a relatively easy way to enumerate the tests we do run under -Xcomp. Cheers, David
I'll ask Ao Qi to file a bug on JBS and post a webrev soon. Thanks again.
Best regards, Jie
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp.
But by all means file a bug and fix it.
Cheers, David
Thanks David. Hope more cases are suitable for both interpreter and JIT tests. On 2019/1/11 上午10:13, David Holmes wrote:
Hi Jie,
On 11/01/2019 11:58 am, Jie Fu wrote:
Hi David,
Thanks and apologies.
No apology needed :)
This issue was discovered by a broad -Xcomp testing with jtreg on Loongson CPUs (MIPS compatible processors). It was intended to test our MIPS port of OpenJDK. We've found and fixed quite a lot of JIT bugs for our MIPS implementation by this approach.
Okay, you may well be testing more tests under Xcomp than what we regularly do, so that may well expose a number of tests that may not work at all, or which fail intermittently. I'm trying to find out if there is a relatively easy way to enumerate the tests we do run under -Xcomp.
Cheers, David
I'll ask Ao Qi to file a bug on JBS and post a webrev soon. Thanks again.
Best regards, Jie
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp.
But by all means file a bug and fix it.
Cheers, David
Hi Fu Jie: I filed it here: https://bugs.openjdk.java.net/browse/JDK-8216528 Cheers, Ao Qi On Fri, Jan 11, 2019 at 10:22 AM Jie Fu <fujie@loongson.cn> wrote:
Thanks David. Hope more cases are suitable for both interpreter and JIT tests.
On 2019/1/11 上午10:13, David Holmes wrote:
Hi Jie,
On 11/01/2019 11:58 am, Jie Fu wrote:
Hi David,
Thanks and apologies.
No apology needed :)
This issue was discovered by a broad -Xcomp testing with jtreg on Loongson CPUs (MIPS compatible processors). It was intended to test our MIPS port of OpenJDK. We've found and fixed quite a lot of JIT bugs for our MIPS implementation by this approach.
Okay, you may well be testing more tests under Xcomp than what we regularly do, so that may well expose a number of tests that may not work at all, or which fail intermittently. I'm trying to find out if there is a relatively easy way to enumerate the tests we do run under -Xcomp.
Cheers, David
I'll ask Ao Qi to file a bug on JBS and post a webrev soon. Thanks again.
Best regards, Jie
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp.
But by all means file a bug and fix it.
Cheers, David
Hi, Please review this patch for JDK-8216528: --------------------------------- diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java --- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Thu Jan 10 20:49:22 2019 +0800 @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while (loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + } System.err.println( "waiting to be notified of loader being weakly reachable..."); --------------------------------- The failure was caused by the lost of an OopMap item for "loaderRef" which was optimized out by the JIT with the liveness analysis optimization. Key idea for this patch: 1) "loaderRef" is referenced after "System.gc()" to keep it alive, which prevents its corresponding OopMap being optimized out by JITs. 2) The "System.gc()"+sleep+test pattern is used, which seems more reliable to trigger a GC. Could you please review it and give me some advice? Thanks. Best regards, Jie On 2019/1/11 上午10:40, Ao Qi wrote:
Hi Fu Jie:
I filed it here: https://bugs.openjdk.java.net/browse/JDK-8216528
Cheers, Ao Qi
On Fri, Jan 11, 2019 at 10:22 AM Jie Fu <fujie@loongson.cn> wrote:
Thanks David. Hope more cases are suitable for both interpreter and JIT tests.
On 2019/1/11 上午10:13, David Holmes wrote:
Hi Jie,
On 11/01/2019 11:58 am, Jie Fu wrote:
Hi David,
Thanks and apologies. No apology needed :)
This issue was discovered by a broad -Xcomp testing with jtreg on Loongson CPUs (MIPS compatible processors). It was intended to test our MIPS port of OpenJDK. We've found and fixed quite a lot of JIT bugs for our MIPS implementation by this approach. Okay, you may well be testing more tests under Xcomp than what we regularly do, so that may well expose a number of tests that may not work at all, or which fail intermittently. I'm trying to find out if there is a relatively easy way to enumerate the tests we do run under -Xcomp.
Cheers, David
I'll ask Ao Qi to file a bug on JBS and post a webrev soon. Thanks again.
Best regards, Jie
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp.
But by all means file a bug and fix it.
Cheers, David
I'm sorry to miss the JBS link. JBS: https://bugs.openjdk.java.net/browse/JDK-8216528 Could you please review it and give me some advice? Thanks. -------- Forwarded Message -------- Subject: RFR: JDK-8216528: test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java failing with Xcomp Date: Fri, 11 Jan 2019 11:16:22 +0800 From: Jie Fu <fujie@loongson.cn> To: David Holmes <david.holmes@oracle.com>, core-libs-dev@openjdk.java.net Hi, Please review this patch for JDK-8216528: --------------------------------- diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java --- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Thu Jan 10 20:49:22 2019 +0800 @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while(loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + } System.err.println( "waiting to be notified of loader being weakly reachable..."); --------------------------------- The failure was caused by the lost of an OopMap item for "loaderRef" which was optimized out by the JIT with the liveness analysis optimization. Key idea for this patch: 1) "loaderRef" is referenced after "System.gc()" to keep it alive, which prevents its corresponding OopMap being optimized out by JITs. 2) The "System.gc()"+sleep+test pattern is used, which seems more reliable to trigger a GC. Could you please review it and give me some advice? Thanks. Best regards, Jie On 2019/1/11 上午10:40, Ao Qi wrote:
Hi Fu Jie:
I filed it here: https://bugs.openjdk.java.net/browse/JDK-8216528
Cheers, Ao Qi
On Fri, Jan 11, 2019 at 10:22 AM Jie Fu <fujie@loongson.cn> wrote:
Thanks David. Hope more cases are suitable for both interpreter and JIT tests.
On 2019/1/11 上午10:13, David Holmes wrote:
Hi Jie,
On 11/01/2019 11:58 am, Jie Fu wrote:
Hi David,
Thanks and apologies. No apology needed :)
This issue was discovered by a broad -Xcomp testing with jtreg on Loongson CPUs (MIPS compatible processors). It was intended to test our MIPS port of OpenJDK. We've found and fixed quite a lot of JIT bugs for our MIPS implementation by this approach. Okay, you may well be testing more tests under Xcomp than what we regularly do, so that may well expose a number of tests that may not work at all, or which fail intermittently. I'm trying to find out if there is a relatively easy way to enumerate the tests we do run under -Xcomp.
Cheers, David
I'll ask Ao Qi to file a bug on JBS and post a webrev soon. Thanks again.
Best regards, Jie
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp.
But by all means file a bug and fix it.
Cheers, David
Hi Jie, On 11/01/2019 1:24 pm, Jie Fu wrote:
I'm sorry to miss the JBS link.
JBS: https://bugs.openjdk.java.net/browse/JDK-8216528
Could you please review it and give me some advice? Thanks.
I see three choices for you here :) 1. Don't try to run all tests under Xcomp but just stick to the "core" sets of tests already tested by others. 2. Fix the given test as outlined. (I tested it on linux-x64 and it fixed the problem.) 3. Exclude the given test from Xcomp by adding: @requires vm.compMode != "Xcomp" If you chose options 2 or 3 please update the @bug line with 8216528. The core-libs folk may have more to say here and they will need to provide a sponsor for the commit. Thanks, David -----
-------- Forwarded Message -------- Subject: RFR: JDK-8216528: test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java failing with Xcomp Date: Fri, 11 Jan 2019 11:16:22 +0800 From: Jie Fu <fujie@loongson.cn> To: David Holmes <david.holmes@oracle.com>, core-libs-dev@openjdk.java.net
Hi,
Please review this patch for JDK-8216528: --------------------------------- diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java --- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Thu Jan 10 20:49:22 2019 +0800 @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while(loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + }
System.err.println( "waiting to be notified of loader being weakly reachable..."); ---------------------------------
The failure was caused by the lost of an OopMap item for "loaderRef" which was optimized out by the JIT with the liveness analysis optimization.
Key idea for this patch: 1) "loaderRef" is referenced after "System.gc()" to keep it alive, which prevents its corresponding OopMap being optimized out by JITs. 2) The "System.gc()"+sleep+test pattern is used, which seems more reliable to trigger a GC.
Could you please review it and give me some advice? Thanks.
Best regards, Jie
On 2019/1/11 上午10:40, Ao Qi wrote:
Hi Fu Jie:
I filed it here: https://bugs.openjdk.java.net/browse/JDK-8216528
Cheers, Ao Qi
On Fri, Jan 11, 2019 at 10:22 AM Jie Fu <fujie@loongson.cn> wrote:
Thanks David. Hope more cases are suitable for both interpreter and JIT tests.
On 2019/1/11 上午10:13, David Holmes wrote:
Hi Jie,
On 11/01/2019 11:58 am, Jie Fu wrote:
Hi David,
Thanks and apologies. No apology needed :)
This issue was discovered by a broad -Xcomp testing with jtreg on Loongson CPUs (MIPS compatible processors). It was intended to test our MIPS port of OpenJDK. We've found and fixed quite a lot of JIT bugs for our MIPS implementation by this approach. Okay, you may well be testing more tests under Xcomp than what we regularly do, so that may well expose a number of tests that may not work at all, or which fail intermittently. I'm trying to find out if there is a relatively easy way to enumerate the tests we do run under -Xcomp.
Cheers, David
I'll ask Ao Qi to file a bug on JBS and post a webrev soon. Thanks again.
Best regards, Jie
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp.
But by all means file a bug and fix it.
Cheers, David
Hi David, Thank you very much. I'd like to choose option 2. A test case is more valuable if it can be used for both interpreter and JIT tests. Here is the patch based on your comments. ---------------------------------------------------------------------------------- diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java --- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Fri Jan 11 12:55:38 2019 +0800 @@ -22,7 +22,7 @@ */ /* @test - * @bug 4404702 + * @bug 4404702 8216528 * @summary When the RMI runtime (lazily) spawns system threads that could * outlive the application context in which they were (happened to be) * created, such threads should not inherit (thread local) data specific to @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while (loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + } System.err.println( "waiting to be notified of loader being weakly reachable..."); ---------------------------------------------------------------------------------- Could you please review it and give me some advice? Thanks. Best regards, Jie On 2019/1/11 下午12:16, David Holmes wrote:
I see three choices for you here :)
1. Don't try to run all tests under Xcomp but just stick to the "core" sets of tests already tested by others.
2. Fix the given test as outlined. (I tested it on linux-x64 and it fixed the problem.)
3. Exclude the given test from Xcomp by adding: @requires vm.compMode != "Xcomp"
If you chose options 2 or 3 please update the @bug line with 8216528.
The core-libs folk may have more to say here and they will need to provide a sponsor for the commit.
Thanks, David -----
On 11/01/2019 3:07 pm, Jie Fu wrote:
Hi David,
Thank you very much. I'd like to choose option 2. A test case is more valuable if it can be used for both interpreter and JIT tests.
Here is the patch based on your comments. ----------------------------------------------------------------------------------
diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Fri Jan 11 12:55:38 2019 +0800 @@ -22,7 +22,7 @@ */
/* @test - * @bug 4404702 + * @bug 4404702 8216528 * @summary When the RMI runtime (lazily) spawns system threads that could * outlive the application context in which they were (happened to be) * created, such threads should not inherit (thread local) data specific to @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while (loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + }
System.err.println( "waiting to be notified of loader being weakly reachable..."); ----------------------------------------------------------------------------------
Could you please review it and give me some advice?
Not sure what "advice" you are looking for? I have reviewed it - looks fine to me (and I tested it). But I want someone from core-libs to also review it and hopefully sponsor it. Thanks, David
Thanks.
Best regards, Jie
On 2019/1/11 下午12:16, David Holmes wrote:
I see three choices for you here :)
1. Don't try to run all tests under Xcomp but just stick to the "core" sets of tests already tested by others.
2. Fix the given test as outlined. (I tested it on linux-x64 and it fixed the problem.)
3. Exclude the given test from Xcomp by adding: @requires vm.compMode != "Xcomp"
If you chose options 2 or 3 please update the @bug line with 8216528.
The core-libs folk may have more to say here and they will need to provide a sponsor for the commit.
Thanks, David -----
Thanks David. Could someone from core-libs help to review it? Thanks. On 2019/1/11 下午1:33, David Holmes wrote:
On 11/01/2019 3:07 pm, Jie Fu wrote:
Hi David,
Thank you very much. I'd like to choose option 2. A test case is more valuable if it can be used for both interpreter and JIT tests.
Here is the patch based on your comments. ----------------------------------------------------------------------------------
diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Fri Jan 11 12:55:38 2019 +0800 @@ -22,7 +22,7 @@ */
/* @test - * @bug 4404702 + * @bug 4404702 8216528 * @summary When the RMI runtime (lazily) spawns system threads that could * outlive the application context in which they were (happened to be) * created, such threads should not inherit (thread local) data specific to @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while (loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + }
System.err.println( "waiting to be notified of loader being weakly reachable..."); ----------------------------------------------------------------------------------
Could you please review it and give me some advice?
Not sure what "advice" you are looking for?
I have reviewed it - looks fine to me (and I tested it).
But I want someone from core-libs to also review it and hopefully sponsor it.
Thanks, David
Thanks.
Best regards, Jie
On 2019/1/11 下午12:16, David Holmes wrote:
I see three choices for you here :)
1. Don't try to run all tests under Xcomp but just stick to the "core" sets of tests already tested by others.
2. Fix the given test as outlined. (I tested it on linux-x64 and it fixed the problem.)
3. Exclude the given test from Xcomp by adding: @requires vm.compMode != "Xcomp"
If you chose options 2 or 3 please update the @bug line with 8216528.
The core-libs folk may have more to say here and they will need to provide a sponsor for the commit.
Thanks, David -----
Hi, The proposed patch changes the test in a way that is unintended. Adding the infinite loop of gc() and sleep, will change the timeout behavior from the existing timeout of TIMEOUT to the jtreg default timeout of the whole test. Further, it renders the check at lines 114-120 irrelevant since loaderRef.get() will have returned null and the ref will have been enqueued by then. While it is true that calling gc() only once is unreliable, a better fix is to put the code from 108-120 in a loop with a fixed number of durations and add Reachability.reachabilityFence(loaderRef) to ensure the ref is not ignored. Regards, Roger On 01/11/2019 12:07 AM, Jie Fu wrote:
Hi David,
Thank you very much. I'd like to choose option 2. A test case is more valuable if it can be used for both interpreter and JIT tests.
Here is the patch based on your comments. ----------------------------------------------------------------------------------
diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java --- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Fri Jan 11 12:55:38 2019 +0800 @@ -22,7 +22,7 @@ */
/* @test - * @bug 4404702 + * @bug 4404702 8216528 * @summary When the RMI runtime (lazily) spawns system threads that could * outlive the application context in which they were (happened to be) * created, such threads should not inherit (thread local) data specific to @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while (loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + }
System.err.println( "waiting to be notified of loader being weakly reachable..."); ----------------------------------------------------------------------------------
Could you please review it and give me some advice? Thanks.
Best regards, Jie
On 2019/1/11 下午12:16, David Holmes wrote:
I see three choices for you here :)
1. Don't try to run all tests under Xcomp but just stick to the "core" sets of tests already tested by others.
2. Fix the given test as outlined. (I tested it on linux-x64 and it fixed the problem.)
3. Exclude the given test from Xcomp by adding: @requires vm.compMode != "Xcomp"
If you chose options 2 or 3 please update the @bug line with 8216528.
The core-libs folk may have more to say here and they will need to provide a sponsor for the commit.
Thanks, David -----
Hi Roger, On 12/01/2019 2:22 am, Roger Riggs wrote:
Hi,
The proposed patch changes the test in a way that is unintended.
Adding the infinite loop of gc() and sleep, will change the timeout behavior from the existing timeout of TIMEOUT to the jtreg default timeout of the whole test.
Partially true. If the new loop gets stuck then yes the jtreg default timeout will apply - I don't see that is necessarily a bad thing. The existing timeout only applies to the refQueue.remove operation itself, you don't know how much time was spent before you got there, nor how much will be spent after in the dumpThreads() calls - so the jtreg timeout can still come into affect.
Further, it renders the check at lines 114-120 irrelevant since loaderRef.get() will have returned null and the ref will have been enqueued by then.
I wouldn't say irrelevant as it double-checks the interaction between the ref.get() and the queue.remove() - the result of one should imply the result of the other, but if enqueuing had a bug ....
While it is true that calling gc() only once is unreliable, a better fix is to put the code from 108-120 in a loop with a fixed number of durations
That would also work - say 5 loops and reduce TIMEOUT to 4000.
and add Reachability.reachabilityFence(loaderRef) to ensure the ref is not ignored.
Adding ReachabilityFence, alone, may solve the observed problem given one gc() seems to be working in practice (and because we don't actually have the leaked loaders anymore because those threads (sun.misc.GC threads) don't exist anymore). Cheers, David
Regards, Roger
On 01/11/2019 12:07 AM, Jie Fu wrote:
Hi David,
Thank you very much. I'd like to choose option 2. A test case is more valuable if it can be used for both interpreter and JIT tests.
Here is the patch based on your comments. ----------------------------------------------------------------------------------
diff -r 02e648ae46c3 test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Wed Jan 09 01:06:19 2019 +0100 +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java Fri Jan 11 12:55:38 2019 +0800 @@ -22,7 +22,7 @@ */
/* @test - * @bug 4404702 + * @bug 4404702 8216528 * @summary When the RMI runtime (lazily) spawns system threads that could * outlive the application context in which they were (happened to be) * created, such threads should not inherit (thread local) data specific to @@ -106,7 +106,10 @@ * context class loader-- by giving it a chance to pass away. */ Thread.sleep(2000); - System.gc(); + while (loaderRef.get() != null) { + System.gc(); + Thread.sleep(100); + }
System.err.println( "waiting to be notified of loader being weakly reachable..."); ----------------------------------------------------------------------------------
Could you please review it and give me some advice? Thanks.
Best regards, Jie
On 2019/1/11 下午12:16, David Holmes wrote:
I see three choices for you here :)
1. Don't try to run all tests under Xcomp but just stick to the "core" sets of tests already tested by others.
2. Fix the given test as outlined. (I tested it on linux-x64 and it fixed the problem.)
3. Exclude the given test from Xcomp by adding: @requires vm.compMode != "Xcomp"
If you chose options 2 or 3 please update the @bug line with 8216528.
The core-libs folk may have more to say here and they will need to provide a sponsor for the commit.
Thanks, David -----
Thanks David and Roger. On 2019年01月12日 06:52, David Holmes wrote:
Hi Roger,
On 12/01/2019 2:22 am, Roger Riggs wrote:
Hi,
The proposed patch changes the test in a way that is unintended.
Adding the infinite loop of gc() and sleep, will change the timeout behavior from the existing timeout of TIMEOUT to the jtreg default timeout of the whole test.
Partially true. If the new loop gets stuck then yes the jtreg default timeout will apply - I don't see that is necessarily a bad thing. The existing timeout only applies to the refQueue.remove operation itself, you don't know how much time was spent before you got there, nor how much will be spent after in the dumpThreads() calls - so the jtreg timeout can still come into affect.
Further, it renders the check at lines 114-120 irrelevant since loaderRef.get() will have returned null and the ref will have been enqueued by then.
I wouldn't say irrelevant as it double-checks the interaction between the ref.get() and the queue.remove() - the result of one should imply the result of the other, but if enqueuing had a bug ....
While it is true that calling gc() only once is unreliable, a better fix is to put the code from 108-120 in a loop with a fixed number of durations
That would also work - say 5 loops and reduce TIMEOUT to 4000.
and add Reachability.reachabilityFence(loaderRef) to ensure the ref is not ignored.
Adding ReachabilityFence, alone, may solve the observed problem given one gc() seems to be working in practice (and because we don't actually have the leaked loaders anymore because those threads (sun.misc.GC threads) don't exist anymore).
Cheers, David
Regards, Roger
Hi, The simplest fix for this failing test is to add a call to reachabilityFence to prevent the loader and loaderRef from going out of scope early. It maintains getting debug output on a failure. Offlist, Jie and I explored some alternate ways to write the test and settled on this one. Please review: diff --git a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java --- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java @@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea Reference dequeued = refQueue.remove(TIMEOUT); if (dequeued == null) { System.err.println( - "TEST FAILED: loader not deteced weakly reachable"); + "TEST FAILED: loader not detected weakly reachable"); dumpThreads(); throw new RuntimeException( "TEST FAILED: loader not detected weakly reachable"); } + Reference.reachabilityFence(loader); + Reference.reachabilityFence(loaderRef); System.err.println( "TEST PASSED: loader detected weakly reachable"); Thanks, Roger On 01/11/2019 07:25 PM, Jie Fu wrote:
Thanks David and Roger.
On 2019年01月12日 06:52, David Holmes wrote:
Hi Roger,
On 12/01/2019 2:22 am, Roger Riggs wrote:
Hi,
The proposed patch changes the test in a way that is unintended.
Adding the infinite loop of gc() and sleep, will change the timeout behavior from the existing timeout of TIMEOUT to the jtreg default timeout of the whole test.
Partially true. If the new loop gets stuck then yes the jtreg default timeout will apply - I don't see that is necessarily a bad thing. The existing timeout only applies to the refQueue.remove operation itself, you don't know how much time was spent before you got there, nor how much will be spent after in the dumpThreads() calls - so the jtreg timeout can still come into affect.
Further, it renders the check at lines 114-120 irrelevant since loaderRef.get() will have returned null and the ref will have been enqueued by then.
I wouldn't say irrelevant as it double-checks the interaction between the ref.get() and the queue.remove() - the result of one should imply the result of the other, but if enqueuing had a bug ....
While it is true that calling gc() only once is unreliable, a better fix is to put the code from 108-120 in a loop with a fixed number of durations
That would also work - say 5 loops and reduce TIMEOUT to 4000.
and add Reachability.reachabilityFence(loaderRef) to ensure the ref is not ignored.
Adding ReachabilityFence, alone, may solve the observed problem given one gc() seems to be working in practice (and because we don't actually have the leaked loaders anymore because those threads (sun.misc.GC threads) don't exist anymore).
Cheers, David
Regards, Roger
Hi, I agree that the simplest way to fix the issue is just adding the reachabilityFence. But this patch might also fail since the VM doesn't guarantee that a GC would be performed. I didn't make such patch since I've learned from Sergey and Alan that calling "System.gc()" several times is unreliable to trigger a gc[1]. So I still prefer the original one[2]. Thanks a lot. Best regards, Jie [1] https://mail.openjdk.java.net/pipermail/beans-dev/2019-January/000396.html [2] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht... On 2019/1/28 下午11:39, Roger Riggs wrote:
Hi,
The simplest fix for this failing test is to add a call to reachabilityFence to prevent the loader and loaderRef from going out of scope early. It maintains getting debug output on a failure.
Offlist, Jie and I explored some alternate ways to write the test and settled on this one.
Please review:
diff --git a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java --- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java +++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java @@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea Reference dequeued = refQueue.remove(TIMEOUT); if (dequeued == null) { System.err.println( - "TEST FAILED: loader not deteced weakly reachable"); + "TEST FAILED: loader not detected weakly reachable"); dumpThreads(); throw new RuntimeException( "TEST FAILED: loader not detected weakly reachable"); } + Reference.reachabilityFence(loader); + Reference.reachabilityFence(loaderRef);
System.err.println( "TEST PASSED: loader detected weakly reachable");
Thanks, Roger
On 01/11/2019 07:25 PM, Jie Fu wrote:
Thanks David and Roger.
On 2019年01月12日 06:52, David Holmes wrote:
Hi Roger,
On 12/01/2019 2:22 am, Roger Riggs wrote:
Hi,
The proposed patch changes the test in a way that is unintended.
Adding the infinite loop of gc() and sleep, will change the timeout behavior from the existing timeout of TIMEOUT to the jtreg default timeout of the whole test.
Partially true. If the new loop gets stuck then yes the jtreg default timeout will apply - I don't see that is necessarily a bad thing. The existing timeout only applies to the refQueue.remove operation itself, you don't know how much time was spent before you got there, nor how much will be spent after in the dumpThreads() calls - so the jtreg timeout can still come into affect.
Further, it renders the check at lines 114-120 irrelevant since loaderRef.get() will have returned null and the ref will have been enqueued by then.
I wouldn't say irrelevant as it double-checks the interaction between the ref.get() and the queue.remove() - the result of one should imply the result of the other, but if enqueuing had a bug ....
While it is true that calling gc() only once is unreliable, a better fix is to put the code from 108-120 in a loop with a fixed number of durations
That would also work - say 5 loops and reduce TIMEOUT to 4000.
and add Reachability.reachabilityFence(loaderRef) to ensure the ref is not ignored.
Adding ReachabilityFence, alone, may solve the observed problem given one gc() seems to be working in practice (and because we don't actually have the leaked loaders anymore because those threads (sun.misc.GC threads) don't exist anymore).
Cheers, David
Regards, Roger
Hi Jie, Roger, I think this has now consumed far too many cycles for everyone, dealing with a test that is checking for a leak that can't even exist any more. Alan was fine with the original proposed patch, as was I, so I think we can should proceed with that. Obviously there is more than one way to tackle the Xcomp problem here, and there will always be issues with any test relying on GC interaction, but the proposed patch seems "good enough" to me. Cheers, David On 29/01/2019 11:46 am, Jie Fu wrote:
Hi,
I agree that the simplest way to fix the issue is just adding the reachabilityFence. But this patch might also fail since the VM doesn't guarantee that a GC would be performed.
I didn't make such patch since I've learned from Sergey and Alan that calling "System.gc()" several times is unreliable to trigger a gc[1]. So I still prefer the original one[2].
Thanks a lot.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/beans-dev/2019-January/000396.html [2] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/28 下午11:39, Roger Riggs wrote:
Hi,
The simplest fix for this failing test is to add a call to reachabilityFence to prevent the loader and loaderRef from going out of scope early. It maintains getting debug output on a failure.
Offlist, Jie and I explored some alternate ways to write the test and settled on this one.
Please review:
diff --git a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
+++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
@@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea Reference dequeued = refQueue.remove(TIMEOUT); if (dequeued == null) { System.err.println( - "TEST FAILED: loader not deteced weakly reachable"); + "TEST FAILED: loader not detected weakly reachable"); dumpThreads(); throw new RuntimeException( "TEST FAILED: loader not detected weakly reachable"); } + Reference.reachabilityFence(loader); + Reference.reachabilityFence(loaderRef);
System.err.println( "TEST PASSED: loader detected weakly reachable");
Thanks, Roger
On 01/11/2019 07:25 PM, Jie Fu wrote:
Thanks David and Roger.
On 2019年01月12日 06:52, David Holmes wrote:
Hi Roger,
On 12/01/2019 2:22 am, Roger Riggs wrote:
Hi,
The proposed patch changes the test in a way that is unintended.
Adding the infinite loop of gc() and sleep, will change the timeout behavior from the existing timeout of TIMEOUT to the jtreg default timeout of the whole test.
Partially true. If the new loop gets stuck then yes the jtreg default timeout will apply - I don't see that is necessarily a bad thing. The existing timeout only applies to the refQueue.remove operation itself, you don't know how much time was spent before you got there, nor how much will be spent after in the dumpThreads() calls - so the jtreg timeout can still come into affect.
Further, it renders the check at lines 114-120 irrelevant since loaderRef.get() will have returned null and the ref will have been enqueued by then.
I wouldn't say irrelevant as it double-checks the interaction between the ref.get() and the queue.remove() - the result of one should imply the result of the other, but if enqueuing had a bug ....
While it is true that calling gc() only once is unreliable, a better fix is to put the code from 108-120 in a loop with a fixed number of durations
That would also work - say 5 loops and reduce TIMEOUT to 4000.
and add Reachability.reachabilityFence(loaderRef) to ensure the ref is not ignored.
Adding ReachabilityFence, alone, may solve the observed problem given one gc() seems to be working in practice (and because we don't actually have the leaked loaders anymore because those threads (sun.misc.GC threads) don't exist anymore).
Cheers, David
Regards, Roger
Hi David, I prefer the original patch[1]. Could you please sponsor this issue or help me find a sponsor. I really appreciate it. Thank you very much. Also thanks Roger. We had a pleasant discussion offlist. Best regards, Jie [1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht... On 2019/1/31 上午10:09, David Holmes wrote:
Hi Jie, Roger,
I think this has now consumed far too many cycles for everyone, dealing with a test that is checking for a leak that can't even exist any more. Alan was fine with the original proposed patch, as was I, so I think we can should proceed with that. Obviously there is more than one way to tackle the Xcomp problem here, and there will always be issues with any test relying on GC interaction, but the proposed patch seems "good enough" to me.
Cheers, David
On 29/01/2019 11:46 am, Jie Fu wrote:
Hi,
I agree that the simplest way to fix the issue is just adding the reachabilityFence. But this patch might also fail since the VM doesn't guarantee that a GC would be performed.
I didn't make such patch since I've learned from Sergey and Alan that calling "System.gc()" several times is unreliable to trigger a gc[1]. So I still prefer the original one[2].
Thanks a lot.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/beans-dev/2019-January/000396.html [2] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/28 下午11:39, Roger Riggs wrote:
Hi,
The simplest fix for this failing test is to add a call to reachabilityFence to prevent the loader and loaderRef from going out of scope early. It maintains getting debug output on a failure.
Offlist, Jie and I explored some alternate ways to write the test and settled on this one.
Please review:
diff --git a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
+++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
@@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea Reference dequeued = refQueue.remove(TIMEOUT); if (dequeued == null) { System.err.println( - "TEST FAILED: loader not deteced weakly reachable"); + "TEST FAILED: loader not detected weakly reachable"); dumpThreads(); throw new RuntimeException( "TEST FAILED: loader not detected weakly reachable"); } + Reference.reachabilityFence(loader); + Reference.reachabilityFence(loaderRef);
System.err.println( "TEST PASSED: loader detected weakly reachable");
Thanks, Roger
On 01/11/2019 07:25 PM, Jie Fu wrote:
Thanks David and Roger.
On 2019年01月12日 06:52, David Holmes wrote:
Hi Roger,
On 12/01/2019 2:22 am, Roger Riggs wrote:
Hi,
The proposed patch changes the test in a way that is unintended.
Adding the infinite loop of gc() and sleep, will change the timeout behavior from the existing timeout of TIMEOUT to the jtreg default timeout of the whole test.
Partially true. If the new loop gets stuck then yes the jtreg default timeout will apply - I don't see that is necessarily a bad thing. The existing timeout only applies to the refQueue.remove operation itself, you don't know how much time was spent before you got there, nor how much will be spent after in the dumpThreads() calls - so the jtreg timeout can still come into affect.
Further, it renders the check at lines 114-120 irrelevant since loaderRef.get() will have returned null and the ref will have been enqueued by then.
I wouldn't say irrelevant as it double-checks the interaction between the ref.get() and the queue.remove() - the result of one should imply the result of the other, but if enqueuing had a bug ....
While it is true that calling gc() only once is unreliable, a better fix is to put the code from 108-120 in a loop with a fixed number of durations
That would also work - say 5 loops and reduce TIMEOUT to 4000.
and add Reachability.reachabilityFence(loaderRef) to ensure the ref is not ignored.
Adding ReachabilityFence, alone, may solve the observed problem given one gc() seems to be working in practice (and because we don't actually have the leaked loaders anymore because those threads (sun.misc.GC threads) don't exist anymore).
Cheers, David
Regards, Roger
On 31/01/2019 12:44 pm, Jie Fu wrote:
Hi David,
I prefer the original patch[1].
Could you please sponsor this issue or help me find a sponsor. I really appreciate it. Thank you very much.
Hopefully Roger will still sponsor this. Thanks, David
Also thanks Roger. We had a pleasant discussion offlist.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/31 上午10:09, David Holmes wrote:
Hi Jie, Roger,
I think this has now consumed far too many cycles for everyone, dealing with a test that is checking for a leak that can't even exist any more. Alan was fine with the original proposed patch, as was I, so I think we can should proceed with that. Obviously there is more than one way to tackle the Xcomp problem here, and there will always be issues with any test relying on GC interaction, but the proposed patch seems "good enough" to me.
Cheers, David
On 29/01/2019 11:46 am, Jie Fu wrote:
Hi,
I agree that the simplest way to fix the issue is just adding the reachabilityFence. But this patch might also fail since the VM doesn't guarantee that a GC would be performed.
I didn't make such patch since I've learned from Sergey and Alan that calling "System.gc()" several times is unreliable to trigger a gc[1]. So I still prefer the original one[2].
Thanks a lot.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/beans-dev/2019-January/000396.html
[2] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/28 下午11:39, Roger Riggs wrote:
Hi,
The simplest fix for this failing test is to add a call to reachabilityFence to prevent the loader and loaderRef from going out of scope early. It maintains getting debug output on a failure.
Offlist, Jie and I explored some alternate ways to write the test and settled on this one.
Please review:
diff --git a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
+++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
@@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea Reference dequeued = refQueue.remove(TIMEOUT); if (dequeued == null) { System.err.println( - "TEST FAILED: loader not deteced weakly reachable"); + "TEST FAILED: loader not detected weakly reachable"); dumpThreads(); throw new RuntimeException( "TEST FAILED: loader not detected weakly reachable"); } + Reference.reachabilityFence(loader); + Reference.reachabilityFence(loaderRef);
System.err.println( "TEST PASSED: loader detected weakly reachable");
Thanks, Roger
On 01/11/2019 07:25 PM, Jie Fu wrote:
Thanks David and Roger.
On 2019年01月12日 06:52, David Holmes wrote:
Hi Roger,
On 12/01/2019 2:22 am, Roger Riggs wrote: > Hi, > > The proposed patch changes the test in a way that is unintended. > > Adding the infinite loop of gc() and sleep, will change the > timeout behavior > from the existing timeout of TIMEOUT to the jtreg default timeout > of the whole test.
Partially true. If the new loop gets stuck then yes the jtreg default timeout will apply - I don't see that is necessarily a bad thing. The existing timeout only applies to the refQueue.remove operation itself, you don't know how much time was spent before you got there, nor how much will be spent after in the dumpThreads() calls - so the jtreg timeout can still come into affect.
> Further, it renders the check at lines 114-120 irrelevant since > loaderRef.get() > will have returned null and the ref will have been enqueued by then.
I wouldn't say irrelevant as it double-checks the interaction between the ref.get() and the queue.remove() - the result of one should imply the result of the other, but if enqueuing had a bug ....
> While it is true that calling gc() only once is unreliable, a > better fix is to > put the code from 108-120 in a loop with a fixed number of durations
That would also work - say 5 loops and reduce TIMEOUT to 4000.
> and add Reachability.reachabilityFence(loaderRef) to ensure the > ref is not ignored.
Adding ReachabilityFence, alone, may solve the observed problem given one gc() seems to be working in practice (and because we don't actually have the leaked loaders anymore because those threads (sun.misc.GC threads) don't exist anymore).
Cheers, David
> Regards, Roger
Hi Roger, I really hope you can still sponsor this. Could you help me, please? Thanks again. Best regards, Jie On 2019/1/31 上午10:59, David Holmes wrote:
On 31/01/2019 12:44 pm, Jie Fu wrote:
Hi David,
I prefer the original patch[1].
Could you please sponsor this issue or help me find a sponsor. I really appreciate it. Thank you very much.
Hopefully Roger will still sponsor this.
Thanks, David
Also thanks Roger. We had a pleasant discussion offlist.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/31 上午10:09, David Holmes wrote:
Hi Jie, Roger,
I think this has now consumed far too many cycles for everyone, dealing with a test that is checking for a leak that can't even exist any more. Alan was fine with the original proposed patch, as was I, so I think we can should proceed with that. Obviously there is more than one way to tackle the Xcomp problem here, and there will always be issues with any test relying on GC interaction, but the proposed patch seems "good enough" to me.
Cheers, David
On 29/01/2019 11:46 am, Jie Fu wrote:
Hi,
I agree that the simplest way to fix the issue is just adding the reachabilityFence. But this patch might also fail since the VM doesn't guarantee that a GC would be performed.
I didn't make such patch since I've learned from Sergey and Alan that calling "System.gc()" several times is unreliable to trigger a gc[1]. So I still prefer the original one[2].
Thanks a lot.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/beans-dev/2019-January/000396.html
[2] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/28 下午11:39, Roger Riggs wrote:
Hi,
The simplest fix for this failing test is to add a call to reachabilityFence to prevent the loader and loaderRef from going out of scope early. It maintains getting debug output on a failure.
Offlist, Jie and I explored some alternate ways to write the test and settled on this one.
Please review:
diff --git a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
+++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
@@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea Reference dequeued = refQueue.remove(TIMEOUT); if (dequeued == null) { System.err.println( - "TEST FAILED: loader not deteced weakly reachable"); + "TEST FAILED: loader not detected weakly reachable"); dumpThreads(); throw new RuntimeException( "TEST FAILED: loader not detected weakly reachable"); } + Reference.reachabilityFence(loader); + Reference.reachabilityFence(loaderRef);
System.err.println( "TEST PASSED: loader detected weakly reachable");
Thanks, Roger
On 01/11/2019 07:25 PM, Jie Fu wrote:
Thanks David and Roger.
On 2019年01月12日 06:52, David Holmes wrote: > Hi Roger, > > On 12/01/2019 2:22 am, Roger Riggs wrote: >> Hi, >> >> The proposed patch changes the test in a way that is unintended. >> >> Adding the infinite loop of gc() and sleep, will change the >> timeout behavior >> from the existing timeout of TIMEOUT to the jtreg default >> timeout of the whole test. > > Partially true. If the new loop gets stuck then yes the jtreg > default timeout will apply - I don't see that is necessarily a > bad thing. The existing timeout only applies to the > refQueue.remove operation itself, you don't know how much time > was spent before you got there, nor how much will be spent after > in the dumpThreads() calls - so the jtreg timeout can still come > into affect. > >> Further, it renders the check at lines 114-120 irrelevant since >> loaderRef.get() >> will have returned null and the ref will have been enqueued by >> then. > > I wouldn't say irrelevant as it double-checks the interaction > between the ref.get() and the queue.remove() - the result of one > should imply the result of the other, but if enqueuing had a bug > .... > >> While it is true that calling gc() only once is unreliable, a >> better fix is to >> put the code from 108-120 in a loop with a fixed number of >> durations > > That would also work - say 5 loops and reduce TIMEOUT to 4000. > >> and add Reachability.reachabilityFence(loaderRef) to ensure the >> ref is not ignored. > > Adding ReachabilityFence, alone, may solve the observed problem > given one gc() seems to be working in practice (and because we > don't actually have the leaked loaders anymore because those > threads (sun.misc.GC threads) don't exist anymore). > > Cheers, > David > >> Regards, Roger
Pushed On 01/30/2019 10:12 PM, Jie Fu wrote:
Hi Roger,
I really hope you can still sponsor this. Could you help me, please? Thanks again.
Best regards, Jie
On 2019/1/31 上午10:59, David Holmes wrote:
On 31/01/2019 12:44 pm, Jie Fu wrote:
Hi David,
I prefer the original patch[1].
Could you please sponsor this issue or help me find a sponsor. I really appreciate it. Thank you very much.
Hopefully Roger will still sponsor this.
Thanks, David
Also thanks Roger. We had a pleasant discussion offlist.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/31 上午10:09, David Holmes wrote:
Hi Jie, Roger,
I think this has now consumed far too many cycles for everyone, dealing with a test that is checking for a leak that can't even exist any more. Alan was fine with the original proposed patch, as was I, so I think we can should proceed with that. Obviously there is more than one way to tackle the Xcomp problem here, and there will always be issues with any test relying on GC interaction, but the proposed patch seems "good enough" to me.
Cheers, David
On 29/01/2019 11:46 am, Jie Fu wrote:
Hi,
I agree that the simplest way to fix the issue is just adding the reachabilityFence. But this patch might also fail since the VM doesn't guarantee that a GC would be performed.
I didn't make such patch since I've learned from Sergey and Alan that calling "System.gc()" several times is unreliable to trigger a gc[1]. So I still prefer the original one[2].
Thanks a lot.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/beans-dev/2019-January/000396.html
[2] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/28 下午11:39, Roger Riggs wrote:
Hi,
The simplest fix for this failing test is to add a call to reachabilityFence to prevent the loader and loaderRef from going out of scope early. It maintains getting debug output on a failure.
Offlist, Jie and I explored some alternate ways to write the test and settled on this one.
Please review:
diff --git a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
--- a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
+++ b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java
@@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea Reference dequeued = refQueue.remove(TIMEOUT); if (dequeued == null) { System.err.println( - "TEST FAILED: loader not deteced weakly reachable"); + "TEST FAILED: loader not detected weakly reachable"); dumpThreads(); throw new RuntimeException( "TEST FAILED: loader not detected weakly reachable"); } + Reference.reachabilityFence(loader); + Reference.reachabilityFence(loaderRef);
System.err.println( "TEST PASSED: loader detected weakly reachable");
Thanks, Roger
On 01/11/2019 07:25 PM, Jie Fu wrote: > Thanks David and Roger. > > > On 2019年01月12日 06:52, David Holmes wrote: >> Hi Roger, >> >> On 12/01/2019 2:22 am, Roger Riggs wrote: >>> Hi, >>> >>> The proposed patch changes the test in a way that is unintended. >>> >>> Adding the infinite loop of gc() and sleep, will change the >>> timeout behavior >>> from the existing timeout of TIMEOUT to the jtreg default >>> timeout of the whole test. >> >> Partially true. If the new loop gets stuck then yes the jtreg >> default timeout will apply - I don't see that is necessarily a >> bad thing. The existing timeout only applies to the >> refQueue.remove operation itself, you don't know how much time >> was spent before you got there, nor how much will be spent >> after in the dumpThreads() calls - so the jtreg timeout can >> still come into affect. >> >>> Further, it renders the check at lines 114-120 irrelevant >>> since loaderRef.get() >>> will have returned null and the ref will have been enqueued by >>> then. >> >> I wouldn't say irrelevant as it double-checks the interaction >> between the ref.get() and the queue.remove() - the result of >> one should imply the result of the other, but if enqueuing had >> a bug .... >> >>> While it is true that calling gc() only once is unreliable, a >>> better fix is to >>> put the code from 108-120 in a loop with a fixed number of >>> durations >> >> That would also work - say 5 loops and reduce TIMEOUT to 4000. >> >>> and add Reachability.reachabilityFence(loaderRef) to ensure >>> the ref is not ignored. >> >> Adding ReachabilityFence, alone, may solve the observed problem >> given one gc() seems to be working in practice (and because we >> don't actually have the leaked loaders anymore because those >> threads (sun.misc.GC threads) don't exist anymore). >> >> Cheers, >> David >> >>> Regards, Roger > >
Thanks Roger. On 2019/1/31 下午11:16, Roger Riggs wrote:
Pushed
On 01/30/2019 10:12 PM, Jie Fu wrote:
Hi Roger,
I really hope you can still sponsor this. Could you help me, please? Thanks again.
Best regards, Jie
On 2019/1/31 上午10:59, David Holmes wrote:
On 31/01/2019 12:44 pm, Jie Fu wrote:
Hi David,
I prefer the original patch[1].
Could you please sponsor this issue or help me find a sponsor. I really appreciate it. Thank you very much.
Hopefully Roger will still sponsor this.
Thanks, David
Also thanks Roger. We had a pleasant discussion offlist.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/31 上午10:09, David Holmes wrote:
Hi Jie, Roger,
I think this has now consumed far too many cycles for everyone, dealing with a test that is checking for a leak that can't even exist any more. Alan was fine with the original proposed patch, as was I, so I think we can should proceed with that. Obviously there is more than one way to tackle the Xcomp problem here, and there will always be issues with any test relying on GC interaction, but the proposed patch seems "good enough" to me.
Cheers, David
On 29/01/2019 11:46 am, Jie Fu wrote:
Hi,
I agree that the simplest way to fix the issue is just adding the reachabilityFence. But this patch might also fail since the VM doesn't guarantee that a GC would be performed.
I didn't make such patch since I've learned from Sergey and Alan that calling "System.gc()" several times is unreliable to trigger a gc[1]. So I still prefer the original one[2].
Thanks a lot.
Best regards, Jie
[1] https://mail.openjdk.java.net/pipermail/beans-dev/2019-January/000396.html
[2] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.ht...
On 2019/1/28 下午11:39, Roger Riggs wrote: > Hi, > > The simplest fix for this failing test is to add a call to > reachabilityFence to prevent > the loader and loaderRef from going out of scope early. It > maintains getting debug > output on a failure. > > Offlist, Jie and I explored some alternate ways to write the > test and settled on this one. > > Please review: > > diff --git > a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java > b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java > > --- > a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java > > +++ > b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java > > @@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea > Reference dequeued = refQueue.remove(TIMEOUT); > if (dequeued == null) { > System.err.println( > - "TEST FAILED: loader not deteced weakly > reachable"); > + "TEST FAILED: loader not detected weakly > reachable"); > dumpThreads(); > throw new RuntimeException( > "TEST FAILED: loader not detected weakly > reachable"); > } > + Reference.reachabilityFence(loader); > + Reference.reachabilityFence(loaderRef); > > System.err.println( > "TEST PASSED: loader detected weakly reachable"); > > Thanks, Roger > > > > On 01/11/2019 07:25 PM, Jie Fu wrote: >> Thanks David and Roger. >> >> >> On 2019年01月12日 06:52, David Holmes wrote: >>> Hi Roger, >>> >>> On 12/01/2019 2:22 am, Roger Riggs wrote: >>>> Hi, >>>> >>>> The proposed patch changes the test in a way that is unintended. >>>> >>>> Adding the infinite loop of gc() and sleep, will change the >>>> timeout behavior >>>> from the existing timeout of TIMEOUT to the jtreg default >>>> timeout of the whole test. >>> >>> Partially true. If the new loop gets stuck then yes the jtreg >>> default timeout will apply - I don't see that is necessarily a >>> bad thing. The existing timeout only applies to the >>> refQueue.remove operation itself, you don't know how much time >>> was spent before you got there, nor how much will be spent >>> after in the dumpThreads() calls - so the jtreg timeout can >>> still come into affect. >>> >>>> Further, it renders the check at lines 114-120 irrelevant >>>> since loaderRef.get() >>>> will have returned null and the ref will have been enqueued >>>> by then. >>> >>> I wouldn't say irrelevant as it double-checks the interaction >>> between the ref.get() and the queue.remove() - the result of >>> one should imply the result of the other, but if enqueuing had >>> a bug .... >>> >>>> While it is true that calling gc() only once is unreliable, a >>>> better fix is to >>>> put the code from 108-120 in a loop with a fixed number of >>>> durations >>> >>> That would also work - say 5 loops and reduce TIMEOUT to 4000. >>> >>>> and add Reachability.reachabilityFence(loaderRef) to ensure >>>> the ref is not ignored. >>> >>> Adding ReachabilityFence, alone, may solve the observed >>> problem given one gc() seems to be working in practice (and >>> because we don't actually have the leaked loaders anymore >>> because those threads (sun.misc.GC threads) don't exist anymore). >>> >>> Cheers, >>> David >>> >>>> Regards, Roger >> >> >
On 11/01/2019 04:16, David Holmes wrote:
I see three choices for you here :)
1. Don't try to run all tests under Xcomp but just stick to the "core" sets of tests already tested by others.
2. Fix the given test as outlined. (I tested it on linux-x64 and it fixed the problem.)
3. Exclude the given test from Xcomp by adding: @requires vm.compMode != "Xcomp"
If you chose options 2 or 3 please update the @bug line with 8216528.
The core-libs folk may have more to say here and they will need to provide a sponsor for the commit.
Jie has been discussing another one on beans-dev in the last few days. #1 would likely avoid at least some whack-a-mole but it might be beneficial to others to have more tests capable of running with -Xcomp. I assume trying more test areas will lead to #3, if nothing else but to exclude tests that run too slowing in that mode. In any case, the patch to the RMI test looks okay. -Alan
Thanks Alan. On 2019年01月11日 22:14, Alan Bateman wrote:
On 11/01/2019 04:16, David Holmes wrote:
I see three choices for you here :)
1. Don't try to run all tests under Xcomp but just stick to the "core" sets of tests already tested by others.
2. Fix the given test as outlined. (I tested it on linux-x64 and it fixed the problem.)
3. Exclude the given test from Xcomp by adding: @requires vm.compMode != "Xcomp"
If you chose options 2 or 3 please update the @bug line with 8216528.
The core-libs folk may have more to say here and they will need to provide a sponsor for the commit.
Jie has been discussing another one on beans-dev in the last few days. #1 would likely avoid at least some whack-a-mole but it might be beneficial to others to have more tests capable of running with -Xcomp. I assume trying more test areas will lead to #3, if nothing else but to exclude tests that run too slowing in that mode. In any case, the patch to the RMI test looks okay.
-Alan
On 11/01/2019 12:13 pm, David Holmes wrote:
Hi Jie,
On 11/01/2019 11:58 am, Jie Fu wrote:
Hi David,
Thanks and apologies.
No apology needed :)
This issue was discovered by a broad -Xcomp testing with jtreg on Loongson CPUs (MIPS compatible processors). It was intended to test our MIPS port of OpenJDK. We've found and fixed quite a lot of JIT bugs for our MIPS implementation by this approach.
Okay, you may well be testing more tests under Xcomp than what we regularly do, so that may well expose a number of tests that may not work at all, or which fail intermittently. I'm trying to find out if there is a relatively easy way to enumerate the tests we do run under -Xcomp.
No easy way but I can tell you we only run a subset of tests in java/lang, java/math and java/util under -Xcomp. David
Cheers, David
I'll ask Ao Qi to file a bug on JBS and post a webrev soon. Thanks again.
Best regards, Jie
For hotspot testing we have certain sets of tests that are run under -Xcomp, but this is obviously not one of them. Did you discover this by chance or because you are attempting to do broad -Xcomp testing? Not every test will work with Xcomp (for various reasons) and we aren't actively trying to make every test pass with Xcomp.
But by all means file a bug and fix it.
Cheers, David
Okay, I got it. Thanks David.
Okay, you may well be testing more tests under Xcomp than what we regularly do, so that may well expose a number of tests that may not work at all, or which fail intermittently. I'm trying to find out if there is a relatively easy way to enumerate the tests we do run under -Xcomp.
No easy way but I can tell you we only run a subset of tests in java/lang, java/math and java/util under -Xcomp.
David
participants (5)
-
Alan Bateman
-
Ao Qi
-
David Holmes
-
Jie Fu
-
Roger Riggs