Re: Testing no memory leak occurs via references
On 08/03/2023 10:36, David Holmes wrote: On 7/03/2023 8:16 pm, Kim Barrett wrote: On Mar 6, 2023, at 10:11 AM, Aleksei Ivanov wrote: On 06/03/2023 13:51, Albert Yang wrote: Upon a cursory inspection of ForceGC.java, it seems that the fundamental logic involves waiting for a certain duration and relying on chance. However, I am of the opinion that utilizing the WhiteBox API can provide greater determinism and potentially strengthen some of the assertions. Yes, it calls System.gc in a loop and waits for a reference to become cleared. WhiteBox.fullGC is better. But is awkward to use within tests. Makes me wonder whether System.gc() should do whatever WhiteBox.fullGC() actually does? Would this be available to test code outside the JDK? Outside the JDK, there also exists a need to ask the system if a reference can be cleared. This is usually part of test code to see if no memory is being leaked unexpectedly. System.gc() is currently our only hope to perform such tests, but it is clear it isn't intended for that purpose. Perhaps there should be a more targetted solution; the intent isn't to run a GC (although that may be necessary depending on the implementation) but to test if a reference can be cleared. Implementations that can't or won't clear references could throw an exception to indicate this kind of test would not work with the current configuration. --John System.gc may not do a full GC; consider, for example, G1 with -XX:+ExplicitGCInvokesConcurrent. Because of potential cross-generational j.l.r.Reference and referent, one invocation might not clear a Reference that would be cleared by a full GC, and might in fact require many iterations. Also, because of SATB requirements, G1 with -XX:+ExplicitGCInvokesConcurrent might never clear a Reference if it is being checked for being cleared in the traditional way (by ref.get() == null) rather than by using the newer ref.refersTo(null). WhiteBox.fullGC deals with both of those, and there's no need for looping. The loops in functions like ForceGC are to deal with those kinds of issues. And waiting for clearing is completely pointless. A given GC invocation will either clear or not, and there's not a delay afterward. The ZGC equivalent of the second can still be a problem. Checking for a cleared Reference really should be done using Reference.refersTo.
Re: Testing no memory leak occurs via references
On 7/03/2023 8:16 pm, Kim Barrett wrote: On Mar 6, 2023, at 10:11 AM, Aleksei Ivanov wrote: On 06/03/2023 13:51, Albert Yang wrote: Upon a cursory inspection of ForceGC.java, it seems that the fundamental logic involves waiting for a certain duration and relying on chance. However, I am of the opinion that utilizing the WhiteBox API can provide greater determinism and potentially strengthen some of the assertions. Yes, it calls System.gc in a loop and waits for a reference to become cleared. WhiteBox.fullGC is better. But is awkward to use within tests. Makes me wonder whether System.gc() should do whatever WhiteBox.fullGC() actually does? David - System.gc may not do a full GC; consider, for example, G1 with -XX:+ExplicitGCInvokesConcurrent. Because of potential cross-generational j.l.r.Reference and referent, one invocation might not clear a Reference that would be cleared by a full GC, and might in fact require many iterations. Also, because of SATB requirements, G1 with -XX:+ExplicitGCInvokesConcurrent might never clear a Reference if it is being checked for being cleared in the traditional way (by ref.get() == null) rather than by using the newer ref.refersTo(null). WhiteBox.fullGC deals with both of those, and there's no need for looping. The loops in functions like ForceGC are to deal with those kinds of issues. And waiting for clearing is completely pointless. A given GC invocation will either clear or not, and there's not a delay afterward. The ZGC equivalent of the second can still be a problem. Checking for a cleared Reference really should be done using Reference.refersTo.
Re: Testing no memory leak occurs via references
Having just a very few sources of wisdom on this in the JDK test suites is a good idea because then any tests that might be affected by policy changes would be easily spotted. I say this despite an instinctive reticence to rely on "frameworks" and "utilities" in jtreg tests. As resources allow we should look into that (across all areas). I don't know if tests which expect to run with the default GC would be wise setting a specific GC. Testing "out of the box" is more important to (eg) client tests. I also think native code is a problem because a lot of tests are run using jtreg *outside* of a build. ie add jtreg and a build to your path and then run jtreg. This is actually normal not an aberration. It has come to me as a surprise in the past that folks who work on VM etc were surprised at this :-) So its not easy to build the native code then. The observations about the fragility of the VM in OOME situations is noted. Also othervm mode just seems a lot safer for all the above tests. -phil. PS someone On 3/6/23 7:11 AM, Aleksei Ivanov wrote: On 06/03/2023 13:51, Albert Yang wrote: Upon a cursory inspection of ForceGC.java, it seems that the fundamental logic involves waiting for a certain duration and relying on chance. However, I am of the opinion that utilizing the WhiteBox API can provide greater determinism and potentially strengthen some of the assertions. Yes, it calls System.gc in a loop and waits for a reference to become cleared. (It looks as if the body of ForceGC duplicates what one would have in the passed BooleanSupplier which again tests if a reference is cleared.) I decided ForceGC is simpler and easier to use I was not aware of your specific requirements, so I cannot say for certain which approach is best. (However, it is worth noting that the WhiteBox API can be utilized to implement ForceGC if necessary.) My test is written to ensure awt.List gets garbage-collected when there are no strong references to it. Before JDK-8040076 had been fixed it wasn't. So the test creates awt.List, adds it to a frame, calls setMultipleMode(true) to enable multi-selection in the list component, removes it from the frame. At this point, I want to confirm the awt.List can be garbage-collected. The original test created a very long String to cause OutOfMemoryError and then verified whether the WeakReference to awt.List is cleared or not. In my first fix, I replaced generating OutOfMemoryError with a call to System.gc() in a loop and waited for the reference object to be cleared. Usually, the reference is cleared in the second iteration if not in the first one. Essentially, ForceGC does the same thing. So, it replaced my custom code with ForceGC.
Re: Testing no memory leak occurs via references
> On Mar 6, 2023, at 10:11 AM, Aleksei Ivanov wrote: > > On 06/03/2023 13:51, Albert Yang wrote: >> Upon a cursory inspection of ForceGC.java, it seems that the fundamental >> logic involves waiting for a certain duration and relying on chance. >> However, I am of the opinion that utilizing the WhiteBox API can provide >> greater determinism and potentially strengthen some of the assertions. > > Yes, it calls System.gc in a loop and waits for a reference to become cleared. WhiteBox.fullGC is better. System.gc may not do a full GC; consider, for example, G1 with -XX:+ExplicitGCInvokesConcurrent. Because of potential cross-generational j.l.r.Reference and referent, one invocation might not clear a Reference that would be cleared by a full GC, and might in fact require many iterations. Also, because of SATB requirements, G1 with -XX:+ExplicitGCInvokesConcurrent might never clear a Reference if it is being checked for being cleared in the traditional way (by ref.get() == null) rather than by using the newer ref.refersTo(null). WhiteBox.fullGC deals with both of those, and there's no need for looping. The loops in functions like ForceGC are to deal with those kinds of issues. And waiting for clearing is completely pointless. A given GC invocation will either clear or not, and there's not a delay afterward. The ZGC equivalent of the second can still be a problem. Checking for a cleared Reference really should be done using Reference.refersTo. signature.asc Description: Message signed with OpenPGP
Re: Testing no memory leak occurs via references
On 06/03/2023 13:51, Albert Yang wrote: Upon a cursory inspection of ForceGC.java, it seems that the fundamental logic involves waiting for a certain duration and relying on chance. However, I am of the opinion that utilizing the WhiteBox API can provide greater determinism and potentially strengthen some of the assertions. Yes, it calls System.gc in a loop and waits for a reference to become cleared. (It looks as if the body of ForceGC duplicates what one would have in the passed BooleanSupplier which again tests if a reference is cleared.) I decided ForceGC is simpler and easier to use I was not aware of your specific requirements, so I cannot say for certain which approach is best. (However, it is worth noting that the WhiteBox API can be utilized to implement ForceGC if necessary.) My test is written to ensure awt.List gets garbage-collected when there are no strong references to it. Before JDK-8040076 had been fixed it wasn't. So the test creates awt.List, adds it to a frame, calls setMultipleMode(true) to enable multi-selection in the list component, removes it from the frame. At this point, I want to confirm the awt.List can be garbage-collected. The original test created a very long String to cause OutOfMemoryError and then verified whether the WeakReference to awt.List is cleared or not. In my first fix, I replaced generating OutOfMemoryError with a call to System.gc() in a loop and waited for the reference object to be cleared. Usually, the reference is cleared in the second iteration if not in the first one. Essentially, ForceGC does the same thing. So, it replaced my custom code with ForceGC. -- Alexey
Re: Testing no memory leak occurs via references
Hi Albert, The only downside of the WhiteBox API is the need to link and run with native code. It make adhoc testing more difficult. I would not say ForceGC relies on chance, since it is testing the specific condition as requested by the caller. It uses mechanisms available to normal applications and conditions they would encounter and not wait longer than necessary. Regards, Roger On 3/6/23 8:51 AM, Albert Yang wrote: Upon a cursory inspection of ForceGC.java, it seems that the fundamental logic involves waiting for a certain duration and relying on chance. However, I am of the opinion that utilizing the WhiteBox API can provide greater determinism and potentially strengthen some of the assertions. I decided ForceGC is simpler and easier to use I was not aware of your specific requirements, so I cannot say for certain which approach is best. (However, it is worth noting that the WhiteBox API can be utilized to implement ForceGC if necessary.) /Albert
Re: Testing no memory leak occurs via references
Upon a cursory inspection of ForceGC.java, it seems that the fundamental logic involves waiting for a certain duration and relying on chance. However, I am of the opinion that utilizing the WhiteBox API can provide greater determinism and potentially strengthen some of the assertions. > I decided ForceGC is simpler and easier to use I was not aware of your specific requirements, so I cannot say for certain which approach is best. (However, it is worth noting that the WhiteBox API can be utilized to implement ForceGC if necessary.) /Albert
Re: Testing no memory leak occurs via references
Thank you, Stuart and Roger, for the suggestion. I didn't know about ForceGC class. We should use the common library, I have updated my PR [7] with ForceGC. Albert and Thomas suggested using WhiteBox API. It would probably work too, however, I decided ForceGC is simpler and easier to use, so I went for it. It does its job and it's used in core-libs tests. Thanks to everyone who provided their input, I appreciate your help. -- Regards, Alexey [7] https://github.com/openjdk/jdk/pull/12594 On 03/03/2023 22:54, Stuart Marks wrote: As Roger mentioned, there is a ForceGC utility in the test library: test/lib/jdk/test/lib/util/ForceGC.java and it's used in a variety of places in the core libs tests. Essentially it sets up a PhantomReference and a ReferenceQueue and runs System.gc() in a loop. I'd strongly recommend using this in preference to allocating a lot of memory in order to provoke OutOfMemoryError. That technique has historically been a cause of test flakiness, and it still is, as you've discovered. There is also MemoryMXBean.gc(), which does the same thing System.gc() does -- it calls Runtime.getRuntime().gc(). It's true that System.gc() may sometimes be ignored -- for instance if Epsilon GC is enabled -- but for practical purposes, on Hotspot using a standard collector, calling it will eventually cause garbage collection and reference processing. If at some point the behavior provided by System.gc() is inadequate for our testing, we'll need to plumb a JDK-specific interface that has stronger semantics, and then convert ForceGC to use it so that individual tests won't have to be updated. There are still some tests that allocate lots of memory in order to provoke OOME and consequently reference processing. They probably need to be run in /othervm mode in order to set custom heap sizes and to avoid interfering with other tests. It would be interesting to see if those could be adjusted to use something ForceGC so that they can share the JVM with other tests and also avoid allocating lots of memory. s'marks On 3/3/23 10:02 AM, Aleksei Ivanov wrote: Hello, In clientlibs, there's occasionally a need to verify an object isn't leaked. For this purpose, WeakReference or PhantomReference is used. Then, we need to make the reference object be cleared, so a GC cycle need to be triggered. The common approach is generating OutOfMemoryError, catching it and verifying whether the reference is cleared. Some tests use a utility method regtesthelpers/Util.generateOOME [1]. For example, these tests follow the above approach: https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/border/TestTitledBorderLeak.java https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/List/ListGarbageCollectionTest/AwtListGarbageCollectionTest.java The AwtListGarbageCollectionTest.java test started to fail pretty often in the end of January 2023. I followed a piece of advice provided in a JBS comment for JDK-8300727 [2] and replaced generating OOME with a simple call to System.gc() along with adding a loop for re-trying. The specification for System.gc() [3] mentions that this call can be ignored, which started a discussion in the PR #12594 [4] that System.gc() should not be used, at the very least without generating OOME in addition to invoking System.gc(). At the same time, many tests for Reference objects, such as ReferenceEnqueue.java [5] and PhantomReferentClearing.java [6], rely solely on System.gc. What would be your recommendation? Are there best practices in core-libs and hotspot for testing for memory leaks that clientlibs should follow? -- Regards, Alexey [1] https://github.com/openjdk/jdk/blob/29ee7c3b70ded8cd124ca5b4a38a2aee7c39068b/test/jdk/javax/swing/regtesthelpers/Util.java#L87 [2] https://bugs.openjdk.org/browse/JDK-8300727 [3] https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/System.html#gc() [4] https://github.com/openjdk/jdk/pull/12594 [5] https://github.com/openjdk/jdk/blob/f612dcfebea7ffd4390f833646ad45d6f0ebd04f/test/jdk/java/lang/ref/ReferenceEnqueue.java#L54-L60 [6] https://github.com/openjdk/jdk/blob/f612dcfebea7ffd4390f833646ad45d6f0ebd04f/test/jdk/java/lang/ref/PhantomReferentClearing.java#L85-L92
Re: Testing no memory leak occurs via references
Hi, On 03.03.23 19:02, Aleksei Ivanov wrote: > Hello, > > In clientlibs, there's occasionally a need to verify an object isn't > leaked. For this purpose, WeakReference or PhantomReference is used. > > Then, we need to make the reference object be cleared, so a GC cycle > need to be triggered. The common approach is generating > OutOfMemoryError, catching it and verifying whether the reference is > cleared. > > Some tests use a utility method regtesthelpers/Util.generateOOME [1]. > > For example, these tests follow the above approach: > https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/border/TestTitledBorderLeak.java > https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/List/ListGarbageCollectionTest/AwtListGarbageCollectionTest.java > > > The AwtListGarbageCollectionTest.java test started to fail pretty often > in the end of January 2023. > > I followed a piece of advice provided in a JBS comment for JDK-8300727 > [2] and replaced generating OOME with a simple call to System.gc() along > with adding a loop for re-trying. > > The specification for System.gc() [3] mentions that this call can be > ignored, which started a discussion in the PR #12594 [4] that > System.gc() should not be used, at the very least without generating > OOME in addition to invoking System.gc(). > > At the same time, many tests for Reference objects, such as > ReferenceEnqueue.java [5] and PhantomReferentClearing.java [6], rely > solely on System.gc. > > > What would be your recommendation? Are there best practices in core-libs > and hotspot for testing for memory leaks that clientlibs should follow? > For tests, the Whitebox methods allow you to specify the exact GC you want to have with a guarantee that it will be executed (in addition to potentially others caused naturally by the application!), i.e. minor/major gc, and specific to collectors, start concurrent gcs etc. E.g. test/hotspot/jtreg/gc/whitebox/TestWBGC.java gives an example how to execute young gcs. It may not be a comprehensive API to test all kinds of GCs, but it fits the use cases we need so far. I recommend to not use OOME for anything unless in extremely specific situations (test OOME is triggered or something). At/after OOME the VM is in a very precarious state that can give unexpected VM bailouts. Thanks, Thomas
Re: Testing no memory leak occurs via references
As Roger mentioned, there is a ForceGC utility in the test library: test/lib/jdk/test/lib/util/ForceGC.java and it's used in a variety of places in the core libs tests. Essentially it sets up a PhantomReference and a ReferenceQueue and runs System.gc() in a loop. I'd strongly recommend using this in preference to allocating a lot of memory in order to provoke OutOfMemoryError. That technique has historically been a cause of test flakiness, and it still is, as you've discovered. There is also MemoryMXBean.gc(), which does the same thing System.gc() does -- it calls Runtime.getRuntime().gc(). It's true that System.gc() may sometimes be ignored -- for instance if Epsilon GC is enabled -- but for practical purposes, on Hotspot using a standard collector, calling it will eventually cause garbage collection and reference processing. If at some point the behavior provided by System.gc() is inadequate for our testing, we'll need to plumb a JDK-specific interface that has stronger semantics, and then convert ForceGC to use it so that individual tests won't have to be updated. There are still some tests that allocate lots of memory in order to provoke OOME and consequently reference processing. They probably need to be run in /othervm mode in order to set custom heap sizes and to avoid interfering with other tests. It would be interesting to see if those could be adjusted to use something ForceGC so that they can share the JVM with other tests and also avoid allocating lots of memory. s'marks On 3/3/23 10:02 AM, Aleksei Ivanov wrote: Hello, In clientlibs, there's occasionally a need to verify an object isn't leaked. For this purpose, WeakReference or PhantomReference is used. Then, we need to make the reference object be cleared, so a GC cycle need to be triggered. The common approach is generating OutOfMemoryError, catching it and verifying whether the reference is cleared. Some tests use a utility method regtesthelpers/Util.generateOOME [1]. For example, these tests follow the above approach: https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/border/TestTitledBorderLeak.java https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/List/ListGarbageCollectionTest/AwtListGarbageCollectionTest.java The AwtListGarbageCollectionTest.java test started to fail pretty often in the end of January 2023. I followed a piece of advice provided in a JBS comment for JDK-8300727 [2] and replaced generating OOME with a simple call to System.gc() along with adding a loop for re-trying. The specification for System.gc() [3] mentions that this call can be ignored, which started a discussion in the PR #12594 [4] that System.gc() should not be used, at the very least without generating OOME in addition to invoking System.gc(). At the same time, many tests for Reference objects, such as ReferenceEnqueue.java [5] and PhantomReferentClearing.java [6], rely solely on System.gc. What would be your recommendation? Are there best practices in core-libs and hotspot for testing for memory leaks that clientlibs should follow? -- Regards, Alexey [1] https://github.com/openjdk/jdk/blob/29ee7c3b70ded8cd124ca5b4a38a2aee7c39068b/test/jdk/javax/swing/regtesthelpers/Util.java#L87 [2] https://bugs.openjdk.org/browse/JDK-8300727 [3] https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/System.html#gc() [4] https://github.com/openjdk/jdk/pull/12594 [5] https://github.com/openjdk/jdk/blob/f612dcfebea7ffd4390f833646ad45d6f0ebd04f/test/jdk/java/lang/ref/ReferenceEnqueue.java#L54-L60 [6] https://github.com/openjdk/jdk/blob/f612dcfebea7ffd4390f833646ad45d6f0ebd04f/test/jdk/java/lang/ref/PhantomReferentClearing.java#L85-L92
Re: Testing no memory leak occurs via references
I wonder if `WhiteBoxAPI` can be used if the goal is to trigger a (Full) GC cycle. An example is `whitebox.fullGC()` in `test/jdk/java/lang/ref/CleanerTest.java`. /Albert
Re: Testing no memory leak occurs via references
Hi, There is test library class jdk.test.lib.util.ForceGC that is useful for that case. It uses a PhantomReference to detect that gc has been run. And it can be called with a function that can test for another condition. For an example, look at the tests that use ForceGC in test/jdk/... Regards, Roger On 3/3/23 1:02 PM, Aleksei Ivanov wrote: Hello, In clientlibs, there's occasionally a need to verify an object isn't leaked. For this purpose, WeakReference or PhantomReference is used. Then, we need to make the reference object be cleared, so a GC cycle need to be triggered. The common approach is generating OutOfMemoryError, catching it and verifying whether the reference is cleared. Some tests use a utility method regtesthelpers/Util.generateOOME [1]. For example, these tests follow the above approach: https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/border/TestTitledBorderLeak.java https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/List/ListGarbageCollectionTest/AwtListGarbageCollectionTest.java The AwtListGarbageCollectionTest.java test started to fail pretty often in the end of January 2023. I followed a piece of advice provided in a JBS comment for JDK-8300727 [2] and replaced generating OOME with a simple call to System.gc() along with adding a loop for re-trying. The specification for System.gc() [3] mentions that this call can be ignored, which started a discussion in the PR #12594 [4] that System.gc() should not be used, at the very least without generating OOME in addition to invoking System.gc(). At the same time, many tests for Reference objects, such as ReferenceEnqueue.java [5] and PhantomReferentClearing.java [6], rely solely on System.gc. What would be your recommendation? Are there best practices in core-libs and hotspot for testing for memory leaks that clientlibs should follow? -- Regards, Alexey [1] https://github.com/openjdk/jdk/blob/29ee7c3b70ded8cd124ca5b4a38a2aee7c39068b/test/jdk/javax/swing/regtesthelpers/Util.java#L87 [2] https://bugs.openjdk.org/browse/JDK-8300727 [3] https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/System.html#gc() [4] https://github.com/openjdk/jdk/pull/12594 [5] https://github.com/openjdk/jdk/blob/f612dcfebea7ffd4390f833646ad45d6f0ebd04f/test/jdk/java/lang/ref/ReferenceEnqueue.java#L54-L60 [6] https://github.com/openjdk/jdk/blob/f612dcfebea7ffd4390f833646ad45d6f0ebd04f/test/jdk/java/lang/ref/PhantomReferentClearing.java#L85-L92
