Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Mon, 2 Mar 2020 12:53:21 GMT, Florian Kirmaier wrote: >>> It's now part of the systemtests, because the memory-semantics for the >>> tests in controls is changed due to the TestToolkit. >> >> just curious: does that imply that you think the tests in controls ... >> rather useless? > > @kleopatra > It's not useless, but having a special Toolkit which is only used inside of > JavaFX itself is very unreasonable. It would make much more sense, to use the > same basis for unit-test as the rest of the JavaFX-Community which is > monocle. The systemTests also seem to work quite well. The unit tests in the `javafx.controls` and `javafx.graphics modules` have used the StubToolkit since the early days. These tests are actually quite useful, but are not able to test anything that depends on the glass toolkit, prism rendering, etc. [JDK-8090538](https://bugs.openjdk.java.net/browse/JDK-8090538) was filed several years ago to explore replacing the StubToolkit with a headless, Monocle-based glass toolkit, but it didn't get very far. There would be some advantages in doing this, but some drawbacks as well (see the discussion thread referred by the above JBS issue). More importantly, it would be a lot of work to do it right, and it isn't clear at this point whether it would be justified by the benefit. So as far as _this_ PR goes, a test in the systemTests project seems fine. I'll put this back on my review queue. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Mon, 2 Mar 2020 10:59:12 GMT, Jeanette Winzenburg wrote: >> I've now readded the unit-test. It based on the "InitialNodesMemoryLeakTest". >> >> Command to execute: `./gradlew -PFULL_TEST=true -PUSE_ROBOT=true >> :systemTests:test --tests >> test.javafx.scene.control.ProgressIndicatorLeakTest` >> >> It's now part of the systemtests, because the memory-semantics for the tests >> in controls is changed due to the TestToolkit. > >> It's now part of the systemtests, because the memory-semantics for the tests >> in controls is changed due to the TestToolkit. > > just curious: does that imply that you think the tests in controls ... rather > useless? @kleopatra It's not useless, but having a special Toolkit which is only used inside of JavaFX itself is very unreasonable. It would make much more sense, to use the same basis for unit-test as the rest of the JavaFX-Community which is monocle. The systemTests also seem to work quite well. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Mon, 2 Mar 2020 10:07:24 GMT, Florian Kirmaier wrote: >> Rather than removing the test, I was suggesting that you create a test for >> memory leaks using the same ad hoc approach that our other memory leak tests >> use. This could later be modified to use the new GC test utility as part of >> creating that utility. The pattern used in, for example, >> [TabPaneHeaderLeakTest.java](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java), >> works well enough, even though it repeats a fair amount of boilerplate code. > > I've now readded the unit-test. It based on the "InitialNodesMemoryLeakTest". > > Command to execute: `./gradlew -PFULL_TEST=true -PUSE_ROBOT=true > :systemTests:test --tests test.javafx.scene.control.ProgressIndicatorLeakTest` > > It's now part of the systemtests, because the memory-semantics for the tests > in controls is changed due to the TestToolkit. > It's now part of the systemtests, because the memory-semantics for the tests > in controls is changed due to the TestToolkit. just curious: does that imply that you think the tests in controls ... rather useless? - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Tue, 4 Feb 2020 23:19:17 GMT, Kevin Rushforth wrote: >> A little bit late ... >> I have now removed unit-test and it's dependency. >> I will add a ticket about adding them again. > > Rather than removing the test, I was suggesting that you create a test for > memory leaks using the same ad hoc approach that our other memory leak tests > use. This could later be modified to use the new GC test utility as part of > creating that utility. The pattern used in, for example, > [TabPaneHeaderLeakTest.java](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java), > works well enough, even though it repeats a fair amount of boilerplate code. I've now readded the unit-test. It based on the "InitialNodesMemoryLeakTest". Command to execute: `./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests test.javafx.scene.control.ProgressIndicatorLeakTest` It's now part of the systemtests, because the memory-semantics for the tests in controls is changed due to the TestToolkit. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Mon, 27 Jan 2020 15:26:48 GMT, Florian Kirmaier wrote: >> The use of static analysis tools to catch certain types of problems is >> orthogonal to a regression test to validate a bug fix of a specific memory >> leak. >> >> @FlorianKirmaier as mentioned previously, please file a new JBS Enhancement >> to propose adding a test utility for memory leak test. You can then start a >> discussion on the openjfx-dev mailing list. >> >> As for fixing this memory leak, I recommend one of the following two >> approaches: >> 1. Modify the PR without relying on any new utilities (meaning you would >> create yet-another adhoc test for memory leaks). >> 2. Close this PR, wait for memory leak test utility to be integrated, and >> then resubmit the PR for this leak using that utility. >> >> Obviously, approach 1 would get the fix in faster. You could then modify the >> test to use the new utility as part of implementing the utility. > > A little bit late ... > I have now removed unit-test and it's dependency. > I will add a ticket about adding them again. Rather than removing the test, I was suggesting that you create a test for memory leaks using the same ad hoc approach that our other memory leak tests use. This could later be modified to use the new GC test utility as part of creating that utility. The pattern used in, for example, [TabPaneHeaderLeakTest.java](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java), works well enough, even though it repeats a fair amount of boilerplate code. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:09:02 GMT, Kevin Rushforth wrote:
>> The pull request has been updated with 1 additional commit.
>
> build.gradle line 2514:
>
>> 2513: compile project(':graphics')
>> 2514: testCompile "de.sandec:JMemoryBuddy:0.1.3"
>> 2515: }
>
> This is a new third-party dependency, which would need legal approval. You
> need to remove this.
Done
-
PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:10:32 GMT, Kevin Rushforth wrote:
>> The pull request has been updated with 1 additional commit.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 1929:
>
>> 1928: */
>> 1929: public List test_getRemoved() {
>> 1930: return removed;
>
> This is not an acceptable change. We do not add public API to support tests.
> Take a look at the Shim classes for an example of how to handle this.
Done
-
PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Mon, 6 Jan 2020 23:05:13 GMT, Kevin Rushforth wrote: >> I agree. Static analysis tools are quite limited in this regard, and are in >> now way a substitute for regression testing. >> >> So the question is how best to test fixes for memory leaks at runtime. Our >> current approach can be best characterized as "ad hoc", and is not all that >> robust (although works well enough in most cases and is still much better >> than doing no testing at all). I would welcome discussion of a more robust >> approach for testing, but it should be decoupled from this bug fix, and >> discussed as a separate JBS Enhancement request. > > The use of static analysis tools to catch certain types of problems is > orthogonal to a regression test to validate a bug fix of a specific memory > leak. > > @FlorianKirmaier as mentioned previously, please file a new JBS Enhancement > to propose adding a test utility for memory leak test. You can then start a > discussion on the openjfx-dev mailing list. > > As for fixing this memory leak, I recommend one of the following two > approaches: > 1. Modify the PR without relying on any new utilities (meaning you would > create yet-another adhoc test for memory leaks). > 2. Close this PR, wait for memory leak test utility to be integrated, and > then resubmit the PR for this leak using that utility. > > Obviously, approach 1 would get the fix in faster. You could then modify the > test to use the new utility as part of implementing the utility. A little bit late ... I have now removed unit-test and it's dependency. I will add a ticket about adding them again. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Fri, 20 Dec 2019 13:26:05 GMT, Kevin Rushforth wrote: >> I highly doubt that a code analysis tool will find such memoryleaks. > > I agree. Static analysis tools are quite limited in this regard, and are in > now way a substitute for regression testing. > > So the question is how best to test fixes for memory leaks at runtime. Our > current approach can be best characterized as "ad hoc", and is not all that > robust (although works well enough in most cases and is still much better > than doing no testing at all). I would welcome discussion of a more robust > approach for testing, but it should be decoupled from this bug fix, and > discussed as a separate JBS Enhancement request. The use of static analysis tools to catch certain types of problems is orthogonal to a regression test to validate a bug fix of a specific memory leak. @FlorianKirmaier as mentioned previously, please file a new JBS Enhancement to propose adding a test utility for memory leak test. You can then start a discussion on the openjfx-dev mailing list. As for fixing this memory leak, I recommend one of the following two approaches: 1. Modify the PR without relying on any new utilities (meaning you would create yet-another adhoc test for memory leaks). 2. Close this PR, wait for memory leak test utility to be integrated, and then resubmit the PR for this leak using that utility. Obviously, approach 1 would get the fix in faster. You could then modify the test to use the new utility as part of implementing the utility. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator (Code Analysis Discussion)
Accepting that static analysis tools are not best at finding memory leaks...however they can find some easy to find ones. I was just wondering if some of these can be run across the code base to find these "easy to find" ones to help in the process. Additionally, if there are certain checks that want to be flowed out across the code base (i.e. checks for initialization, then having the analysis run as part of a CI build jobs which could be periodically run and reported on to might help maintain some level of code quality over time. I know this can potentially find a lot of false failures but just thought it might be worth considering at some point. Eric Bresie ebre...@gmail.com On Fri, Dec 20, 2019 at 7:30 AM Kevin Rushforth wrote: > On Fri, 20 Dec 2019 09:53:56 GMT, Florian Kirmaier > wrote: > > >> @dsgrieve > >> It's worth mentioning that JavaFX already has many tests based on > System.gc(). > >> An advantage of having a testsuit as an library (or copyied from an > library) is, that its stability is regulary verified by the travis builds > for different JVMs. > >> The alternative would be to not test for memory-leaks at all which is > much worse than having slightly unstable tests. > >> Maybe it can make sense to seperate these tests for leaks in an own > testgroup. > >> > >> I'm introducing this library in more and more projects. I never had > problems with unstable tests. > >> I only had this kind of problem when I wrote the > WeakReference/System.gc/sleep-logic for every single test. > > > > I highly doubt that a code analysis tool will find such memoryleaks. > > I agree. Static analysis tools are quite limited in this regard, and are > in now way a substitute for regression testing. > > So the question is how best to test fixes for memory leaks at runtime. Our > current approach can be best characterized as "ad hoc", and is not all that > robust (although works well enough in most cases and is still much better > than doing no testing at all). I would welcome discussion of a more robust > approach for testing, but it should be decoupled from this bug fix, and > discussed as a separate JBS Enhancement request. > > - > > PR: https://git.openjdk.java.net/jfx/pull/71 >
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Fri, 20 Dec 2019 09:53:56 GMT, Florian Kirmaier wrote: >> @dsgrieve >> It's worth mentioning that JavaFX already has many tests based on >> System.gc(). >> An advantage of having a testsuit as an library (or copyied from an library) >> is, that its stability is regulary verified by the travis builds for >> different JVMs. >> The alternative would be to not test for memory-leaks at all which is much >> worse than having slightly unstable tests. >> Maybe it can make sense to seperate these tests for leaks in an own >> testgroup. >> >> I'm introducing this library in more and more projects. I never had problems >> with unstable tests. >> I only had this kind of problem when I wrote the >> WeakReference/System.gc/sleep-logic for every single test. > > I highly doubt that a code analysis tool will find such memoryleaks. I agree. Static analysis tools are quite limited in this regard, and are in now way a substitute for regression testing. So the question is how best to test fixes for memory leaks at runtime. Our current approach can be best characterized as "ad hoc", and is not all that robust (although works well enough in most cases and is still much better than doing no testing at all). I would welcome discussion of a more robust approach for testing, but it should be decoupled from this bug fix, and discussed as a separate JBS Enhancement request. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 22:22:55 GMT, Florian Kirmaier wrote: >> I would urge caution about incorporating JMemoryBuddy without seeking out >> advice from GC experts. I have my doubts that it will work reliably given >> its reliance on System.gc(). (Opinion is my own, not my employer's). > > @dsgrieve > It's worth mentioning that JavaFX already has many tests based on System.gc(). > An advantage of having a testsuit as an library (or copyied from an library) > is, that its stability is regulary verified by the travis builds for > different JVMs. > The alternative would be to not test for memory-leaks at all which is much > worse than having slightly unstable tests. > Maybe it can make sense to seperate these tests for leaks in an own testgroup. > > I'm introducing this library in more and more projects. I never had problems > with unstable tests. > I only had this kind of problem when I wrote the > WeakReference/System.gc/sleep-logic for every single test. I highly doubt that a code analysis tool will find such memoryleaks. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
Would it just be better to run some code analysis tools on the code base to hunt for these? Like maybe sonarqube integratedas part of CI build analysis? Eric Bresie ebre...@gmail.com > On December 19, 2019 at 4:23:14 PM CST, Florian Kirmaier > wrote: > On Thu, 19 Dec 2019 19:59:07 GMT, David Grieve wrote: > > > > Yes, exactly. > > > > > > @FlorianKirmaier Proposing to add test class to the test infrastructure > > > would be a reasonable alternative to pulling it in as a third-party > > > dependency. I recommend separating it out into its own enhancement, with > > > a separate JBS issue. I see that it has a dependency on the > > > `jdk.management` module. As long as that dependency only surfaces as a > > > test dependency, that won't be a problem. It would be nice if it were > > > available to all modules, meaning that putting it somewhere in > > > `javafx.base` would be good. > > > > I would urge caution about incorporating JMemoryBuddy without seeking out > > advice from GC experts. I have my doubts that it will work reliably given > > its reliance on System.gc(). (Opinion is my own, not my employer's). > > @dsgrieve > It's worth mentioning that JavaFX already has many tests based on System.gc(). > An advantage of having a testsuit as an library (or copyied from an library) > is, that its stability is regulary verified by the travis builds for > different JVMs. > The alternative would be to not test for memory-leaks at all which is much > worse than having slightly unstable tests. > Maybe it can make sense to seperate these tests for leaks in an own testgroup. > > I'm introducing this library in more and more projects. I never had problems > with unstable tests. > I only had this kind of problem when I wrote the > WeakReference/System.gc/sleep-logic for every single test. > > - > > PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 19:59:07 GMT, David Grieve wrote: >> Yes, exactly. >> >> @FlorianKirmaier Proposing to add test class to the test infrastructure >> would be a reasonable alternative to pulling it in as a third-party >> dependency. I recommend separating it out into its own enhancement, with a >> separate JBS issue. I see that it has a dependency on the `jdk.management` >> module. As long as that dependency only surfaces as a test dependency, that >> won't be a problem. It would be nice if it were available to all modules, >> meaning that putting it somewhere in `javafx.base` would be good. > > I would urge caution about incorporating JMemoryBuddy without seeking out > advice from GC experts. I have my doubts that it will work reliably given its > reliance on System.gc(). (Opinion is my own, not my employer's). @dsgrieve It's worth mentioning that JavaFX already has many tests based on System.gc(). An advantage of having a testsuit as an library (or copyied from an library) is, that its stability is regulary verified by the travis builds for different JVMs. The alternative would be to not test for memory-leaks at all which is much worse than having slightly unstable tests. Maybe it can make sense to seperate these tests for leaks in an own testgroup. I'm introducing this library in more and more projects. I never had problems with unstable tests. I only had this kind of problem when I wrote the WeakReference/System.gc/sleep-logic for every single test. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 16:16:28 GMT, Kevin Rushforth wrote: >> probably not invented so often - just c'd from searches of occurances of >> System.gc ;-) > > Yes, exactly. > > @FlorianKirmaier Proposing to add test class to the test infrastructure would > be a reasonable alternative to pulling it in as a third-party dependency. I > recommend separating it out into its own enhancement, with a separate JBS > issue. I see that it has a dependency on the `jdk.management` module. As long > as that dependency only surfaces as a test dependency, that won't be a > problem. It would be nice if it were available to all modules, meaning that > putting it somewhere in `javafx.base` would be good. I would urge caution about incorporating JMemoryBuddy without seeking out advice from GC experts. I have my doubts that it will work reliably given its reliance on System.gc(). (Opinion is my own, not my employer's). - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
Github failed me. My 'remove' comment was applied to the System.out.println, not the assert. > -Original Message- > From: openjfx-dev On Behalf Of > David Grieve > Sent: Thursday, December 19, 2019 2:29 PM > To: openjfx-dev@openjdk.java.net > Subject: [EXTERNAL] Re: [Rev 01] RFR: 8236259: MemoryLeak in > ProgressIndicator > > On Thu, 19 Dec 2019 19:28:34 GMT, Florian Kirmaier > wrote: > > >> Hi everyone, > >> > >> ticket: > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs > .openjdk.java.net%2Fbrowse%2FJDK- > 8236259data=02%7C01%7CDavid.Grieve%40microsoft.com%7C856e05 > 0ee1bf484a7a7008d784b9dde4%7C72f988bf86f141af91ab2d7cd011db47%7C1 > %7C0%7C637123806135627853sdata=yMuZMaBg7UmJEWzn8AGI6K8f5 > aRmFclOi4nov%2BLas%2BM%3Dreserved=0 > >> > >> The fix itself is quite straight forward. > >> It basically just removed the listener which causes the leak. > >> > >> The unit-test for the fix is a bit more complicated. > >> > >> I added a library JMemoryBuddy > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith > ub.com%2FSandec%2FJMemoryBuddydata=02%7C01%7CDavid.Griev > e%40microsoft.com%7C856e050ee1bf484a7a7008d784b9dde4%7C72f988bf8 > 6f141af91ab2d7cd011db47%7C1%7C0%7C637123806135627853sdata=6 > a1cOZEa9Ah%2F47NyvrRetz9YZyTgXhoJ1b13re8SZw8%3Dreserved=0 > (written by myself), which simplifies testing for memory leaks. > >> I think there are many places in the JavaFX-codebase that could highly > benefit from this library. > >> It could also simplify many of the already existing unit tests. > >> It makes testing for memory-leaks readably and reliable. > >> It would also be possible to just copy the code of the library into the > JavaFX-codebase. > >> It only contains a single class. > >> > >> I also had to make a method public, to write the test. I'm open to ideas, > how I could solve it differently. > > > > The pull request has been updated with 1 additional commit. > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/Progr > essIndicatorSkinTest.java line 106: > > > 105: Node detIndicator = > > indicator.getChildrenUnmodifiable().get(0); > > 106: > > System.out.println(detIndicator.getClass().getSimpleName()); > > 107: checker.assertCollectable(detIndicator); > > remove > > - > > PR: > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.o > penjdk.java.net%2Fjfx%2Fpull%2F71data=02%7C01%7CDavid.Grieve% > 40microsoft.com%7C856e050ee1bf484a7a7008d784b9dde4%7C72f988bf86f1 > 41af91ab2d7cd011db47%7C1%7C0%7C637123806135627853sdata=ATF > nmAXAw0x0ENGrqp%2Fde8fEuIg7mrQrYZmXn4gLnug%3Dreserved=0
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 19:28:34 GMT, Florian Kirmaier wrote: >> Hi everyone, >> >> ticket: https://bugs.openjdk.java.net/browse/JDK-8236259 >> >> The fix itself is quite straight forward. >> It basically just removed the listener which causes the leak. >> >> The unit-test for the fix is a bit more complicated. >> >> I added a library JMemoryBuddy https://github.com/Sandec/JMemoryBuddy >> (written by myself), which simplifies testing for memory leaks. >> I think there are many places in the JavaFX-codebase that could highly >> benefit from this library. >> It could also simplify many of the already existing unit tests. >> It makes testing for memory-leaks readably and reliable. >> It would also be possible to just copy the code of the library into the >> JavaFX-codebase. >> It only contains a single class. >> >> I also had to make a method public, to write the test. I'm open to ideas, >> how I could solve it differently. > > The pull request has been updated with 1 additional commit. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ProgressIndicatorSkinTest.java line 106: > 105: Node detIndicator = > indicator.getChildrenUnmodifiable().get(0); > 106: System.out.println(detIndicator.getClass().getSimpleName()); > 107: checker.assertCollectable(detIndicator); remove - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:48:17 GMT, Jeanette Winzenburg wrote: >> The point of having this GC-magic in a library is, that it's verified by the >> library that it's implementation is stable for all common JavaVersions. The >> attemptGC is basically bad practice. I think in the JavaFX-Codebase this >> kind of GC-Tests are invented about 10 times. No one knows which of these >> implementations are stable and their code is usually unreadable. I know it, >> I have done it multiple times. >> >> Copying to Infrastructure would be fine for me. It removes some benefits, >> like easy upgrades, Travis-verified implementation etc. But it's probably >> the best short-term solution. > > probably not invented so often - just c'd from searches of occurances of > System.gc ;-) Yes, exactly. @FlorianKirmaier Proposing to add test class to the test infrastructure would be a reasonable alternative to pulling it in as a third-party dependency. I recommend separating it out into its own enhancement, with a separate JBS issue. I see that it has a dependency on the `jdk.management` module. As long as that dependency only surfaces as a test dependency, that won't be a problem. It would be nice if it were available to all modules, meaning that putting it somewhere in `javafx.base` would be good. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:44:37 GMT, Florian Kirmaier wrote: >> for now, you could have a look at ProgressIndicatorTest (or >> ProgressSkinTest) attemptGC - might not be optimal (don't know enough about >> the dirty details of gc :) but gets the test failing/passing before/after >> fixing a memory leak > > The point of having this GC-magic in a library is, that it's verified by the > library that it's implementation is stable for all common JavaVersions. The > attemptGC is basically bad practice. I think in the JavaFX-Codebase this kind > of GC-Tests are invented about 10 times. No one knows which of these > implementations are stable and their code is usually unreadable. I know it, I > have done it multiple times. > > Copying to Infrastructure would be fine for me. It removes some benefits, > like easy upgrades, Travis-verified implementation etc. But it's probably the > best short-term solution. probably not invented so often - just c'd from searches of occurances of System.gc ;-) - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:33:09 GMT, Jeanette Winzenburg wrote: >> Florian, basically it's a single class isn't it? If so, it might be >> acceptable to add that class to the test.xx.infrastructure package (don't >> know if doing so would require a jbs issue) > > for now, you could have a look at ProgressIndicatorTest (or ProgressSkinTest) > attemptGC - might not be optimal (don't know enough about the dirty details > of gc :) but gets the test failing/passing before/after fixing a memory leak The point of having this GC-magic in a library is, that it's verified by the library that it's implementation is stable for all common JavaVersions. The attemptGC is basically bad practice. I think in the JavaFX-Codebase this kind of GC-Tests are invented about 10 times. No one knows which of these implementations are stable and their code is usually unreadable. I know it, I have done it multiple times. Copying to Infrastructure would be fine for me. It removes some benefits, like easy upgrades, Travis-verified implementation etc. But it's probably the best short-term solution. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:27:59 GMT, Jeanette Winzenburg wrote: >> As mentioned, I could copy the class of the library into the JavaFX-Project. >> Alternatively, I could remove the unit-test entirely. >> >> But to be honest, the current state related to memory-leaks in the JavaFX >> project is a bit questionable. JavaFX is filled with leaks and code which >> could have leaks. >> I would like to add many more tests like this into the JavaFX-CodeBase. But >> without the class/library I can only write unreadable code filled with >> sleeps, Weakreferences etc., which would be sad because now I know how to do >> it better. > > Florian, basically it's a single class isn't it? If so, it might be > acceptable to add that class to the test.xx.infrastructure package (don't > know if doing so would require a jbs issue) for now, you could have a look at ProgressIndicatorTest (or ProgressSkinTest) attemptGC - might not be optimal (don't know enough about the dirty details of gc :) but gets the test failing/passing before/after fixing a memory leak - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:12:01 GMT, Kevin Rushforth wrote: >> The pull request has been updated with 1 additional commit. > > I haven't looked at the fix itself, but there are two things that _must_ be > fixed before this PR can be considered: > 1. You need to remove the additional 3rd-party test library dependency. > 2. You need to revert the change that makes a test method public API. As mentioned, I could copy the class of the library into the JavaFX-Project. Alternatively, I could remove the unit-test entirely. But to be honest, the current state related to memory-leaks in the JavaFX project is a bit questionable. JavaFX is filled with leaks and code which could have leaks. I would like to add many more tests like this into the JavaFX-CodeBase. But without the class/library I can only write unreadable code filled with sleeps, Weakreferences etc., which would be sad because now I know how to do it better. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:24:48 GMT, Florian Kirmaier wrote: >> I haven't looked at the fix itself, but there are two things that _must_ be >> fixed before this PR can be considered: >> 1. You need to remove the additional 3rd-party test library dependency. >> 2. You need to revert the change that makes a test method public API. > > As mentioned, I could copy the class of the library into the JavaFX-Project. > Alternatively, I could remove the unit-test entirely. > > But to be honest, the current state related to memory-leaks in the JavaFX > project is a bit questionable. JavaFX is filled with leaks and code which > could have leaks. > I would like to add many more tests like this into the JavaFX-CodeBase. But > without the class/library I can only write unreadable code filled with > sleeps, Weakreferences etc., which would be sad because now I know how to do > it better. Florian, basically it's a single class isn't it? If so, it might be acceptable to add that class to the test.xx.infrastructure package (don't know if doing so would require a jbs issue) - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 15:12:12 GMT, Florian Kirmaier
wrote:
>> Hi everyone,
>>
>> ticket: https://bugs.openjdk.java.net/browse/JDK-8236259
>>
>> The fix itself is quite straight forward.
>> It basically just removed the listener which causes the leak.
>>
>> The unit-test for the fix is a bit more complicated.
>>
>> I added a library JMemoryBuddy https://github.com/Sandec/JMemoryBuddy
>> (written by myself), which simplifies testing for memory leaks.
>> I think there are many places in the JavaFX-codebase that could highly
>> benefit from this library.
>> It could also simplify many of the already existing unit tests.
>> It makes testing for memory-leaks readably and reliable.
>> It would also be possible to just copy the code of the library into the
>> JavaFX-codebase.
>> It only contains a single class.
>>
>> I also had to make a method public, to write the test. I'm open to ideas,
>> how I could solve it differently.
>
> The pull request has been updated with 1 additional commit.
I haven't looked at the fix itself, but there are two things that _must_ be
fixed before this PR can be considered:
1. You need to remove the additional 3rd-party test library dependency.
2. You need to revert the change that makes a test method public API.
build.gradle line 2514:
> 2513: compile project(':graphics')
> 2514: testCompile "de.sandec:JMemoryBuddy:0.1.3"
> 2515: }
This is a new third-party dependency, which would need legal approval. You need
to remove this.
modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 1929:
> 1928: */
> 1929: public List test_getRemoved() {
> 1930: return removed;
This is not an acceptable change. We do not add public API to support tests.
Take a look at the Shim classes for an example of how to handle this.
-
Changes requested by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 14:19:57 GMT, Florian Kirmaier wrote: >> just thinking aloud: actually, it's rather whacky that the indicator >> (mis-)uses the skin's multiplePropertyListener - it's api isn't meant for >> frequent register/unregisters (the unregisters is quite a heavy measure). >> Maybe it should have its own? Or the other way round: skin listening to the >> property and indicator having a method to update based on font which is >> invoked by skin's listener? > > Yes, that would be reasonable. > > I would rewrite the fix, add an method "updateFont" to the skin, which also > updates the DeterminationIndicator. > > When I look into the code, I'm a little bit confused. The fontListeners > doesn't get called without a font-change. In this case, the variable > doneTextWidth and doneTextHeight never get's initialized. Do I miss something > or is the current code wrong? welcome to the jungle :) looks like the code can't really work (and probably is untested) or is not important .. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 13:56:39 GMT, Florian Kirmaier wrote: >> here's where it was added: https://bugs.openjdk.java.net/browse/JDK-8151617 > > I don't see an API in the discussion, on how to remove a specific listener. > Did i miss something? It seems to me, that the API is written to support only > a single listener per property. the api is in a related issue (should be noted in the issue above). The use-cases mentioned in the isssue: it's a do-once scenario with the skin (and its subclasses) as the only user: multiple listeners can be registered, typically by a skin and/or its subclasses, each can be pre/postpended (or replaced) in the chain for that property and all are unregistered at dispose time. So I think it's really bad to use it in another collaborator like the indicator, particularly if that is re-created often and each of the instances need to remove the listener. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 14:01:08 GMT, Jeanette Winzenburg wrote: >> Yes, that doesn't make a lot of sense. It's probably an artifact because I >> expected a different API do unregister the listener. I'm gonna change it >> back! > > just thinking aloud: actually, it's rather whacky that the indicator > (mis-)uses the skin's multiplePropertyListener - it's api isn't meant for > frequent register/unregisters (the unregisters is quite a heavy measure). > Maybe it should have its own? Or the other way round: skin listening to the > property and indicator having a method to update based on font which is > invoked by skin's listener? Yes, that would be reasonable. I would rewrite the fix, add an method "updateFont" to the skin, which also updates the DeterminationIndicator. When I look into the code, I'm a little bit confused. The fontListeners doesn't get called without a font-change. In this case, the variable doneTextWidth and doneTextHeight never get's initialized. Do I miss something or is the current code wrong? - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 13:35:27 GMT, Florian Kirmaier wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java >> line 521: >> >>> 520: registerChangeListener(text.fontProperty(), >>> (Consumer>) fontListener); >>> 521: >>> 522: // The circular background for the progress pie piece >> >> wondering why you add a (strong) field reference here? >> >> Thought that registerChangeListener(observableValue, consumer) takes care of >> handling storage/cleanup - provided its counter-method >> unregisterChangeListener(observableValue) is called, which is missing, so >> seems to be the only part that has to be added. Or not? > > Yes, that doesn't make a lot of sense. It's probably an artifact because I > expected a different API do unregister the listener. I'm gonna change it back! just thinking aloud: actually, it's rather whacky that the indicator (mis-)uses the skin's multiplePropertyListener - it's api isn't meant for frequent register/unregisters (the unregisters is quite a heavy measure). Maybe it should have its own? Or the other way round: skin listening to the property and indicator having a method to update based on font which is invoked by skin's listener? - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 13:47:14 GMT, Jeanette Winzenburg wrote: >> no way, afaik - there had been debates when the api was added somewhere in >> jbs, don't recall exactly where > > here's where it was added: https://bugs.openjdk.java.net/browse/JDK-8151617 I don't see an API in the discussion, on how to remove a specific listener. Did i miss something? It seems to me, that the API is written to support only a single listener per property. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator
> Hi everyone, > > ticket: https://bugs.openjdk.java.net/browse/JDK-8236259 > > The fix itself is quite straight forward. > It basically just removed the listener which causes the leak. > > The unit-test for the fix is a bit more complicated. > > I added a library JMemoryBuddy https://github.com/Sandec/JMemoryBuddy > (written by myself), which simplifies testing for memory leaks. > I think there are many places in the JavaFX-codebase that could highly > benefit from this library. > It could also simplify many of the already existing unit tests. > It makes testing for memory-leaks readably and reliable. > It would also be possible to just copy the code of the library into the > JavaFX-codebase. > It only contains a single class. > > I also had to make a method public, to write the test. I'm open to ideas, how > I could solve it differently. The pull request has been updated with 1 additional commit. - Added commits: - e9ea4d8e: JDK-8236259: MemoryLeak in ProgressIndicator - DeterminateIndicator doesn't get collected Changes: - all: https://git.openjdk.java.net/jfx/pull/71/files - new: https://git.openjdk.java.net/jfx/pull/71/files/c4da2e7b..e9ea4d8e Webrevs: - full: https://webrevs.openjdk.java.net/jfx/71/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/71/webrev.00-01 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/71.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/71/head:pull/71 PR: https://git.openjdk.java.net/jfx/pull/71
