On Fri, 1 May 2020 15:05:26 GMT, Jesper Skov
wrote:
>> @FlorianKirmaier
>> Please integrate this PR by issuing `/integrate` command in a new comment.
>> After that one of us can sponsor it.
>
> @FlorianKirmaier did you want to persue a common shared in-tree GC testing
> facility?
>
> Otherwi
On Tue, 17 Mar 2020 11:08:36 GMT, Ambarish Rapte wrote:
>> Marked as reviewed by arapte (Reviewer).
>
> @FlorianKirmaier
> Please integrate this PR by issuing `/integrate` command in a new comment.
> After that one of us can sponsor it.
@FlorianKirmaier did you want to persue a common shared i
Changeset: b81cf32b
Author:Florian Kirmaier
Committer: Ambarish Rapte
Date: 2020-03-17 13:44:41 +
URL: https://git.openjdk.java.net/jfx/commit/b81cf32b
8236259: MemoryLeak in ProgressIndicator
Reviewed-by: kcr, arapte
!
modules/javafx.controls/src/main/java/javafx/scene/con
On Fri, 13 Mar 2020 05:31:52 GMT, Ambarish Rapte wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8200224
>> removed create garbage
>
> Marked as reviewed by arapte (Reviewer).
@FlorianKirmaier
Please int
On Thu, 12 Mar 2020 15:54:23 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 mo
On Thu, 12 Mar 2020 15:54:23 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 mo
On Wed, 11 Mar 2020 12:38:48 GMT, Ambarish Rapte wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8200224
>> cleaned up code based on code review
>
> tests/system/src/test/java/test/javafx/scene/control/Pr
> 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.co
> 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.co
On Wed, 11 Mar 2020 12:51:12 GMT, Kevin Rushforth wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java
>> line 550:
>>
>>> 549: }
>>> 550:
>>> 551: private void setFillOverride(Paint fillOverride) {
>>
>> The issue can be alternat
On Wed, 11 Mar 2020 12:43:40 GMT, Ambarish Rapte wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8200224
>> cleaned up code based on code review
>
> modules/javafx.controls/src/main/java/javafx/scene/cont
On Tue, 10 Mar 2020 14:35:59 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 mo
On Tue, 10 Mar 2020 16:28:03 GMT, Kevin Rushforth wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8200224
>> cleaned up code based on code review
>
> Marked as reviewed by kcr (Lead).
@arapte or @kleopat
On Tue, 10 Mar 2020 14:35:59 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 mo
On Tue, 10 Mar 2020 14:34:57 GMT, Florian Kirmaier
wrote:
>> The fix looks good to me. I've verified that it fixes the leak.
>>
>> The newly added test looks good as well, with some comments left inline
>> (formatting, a missing copyright header, and a
>> couple other suggestions).
>
> @kevinr
> 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.co
On Thu, 5 Mar 2020 22:15:07 GMT, Kevin Rushforth wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8236259
>> readded unit-test without any dependency
>
> tests/system/src/test/java/test/javafx/scene/contro
On Thu, 5 Mar 2020 22:19:45 GMT, Kevin Rushforth wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8236259
>> readded unit-test without any dependency
>
> The fix looks good to me. I've verified that it fix
On Thu, 5 Mar 2020 22:11:26 GMT, Kevin Rushforth wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8236259
>> readded unit-test without any dependency
>
> tests/system/src/test/java/test/javafx/scene/contro
On Thu, 5 Mar 2020 21:47:35 GMT, Kevin Rushforth wrote:
>> 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,
>> typi
On Mon, 2 Mar 2020 10:09:57 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
On Thu, 19 Dec 2019 14:43:29 GMT, Jeanette Winzenburg
wrote:
>> 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 (sho
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?
>
> @kleo
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.ProgressIndicatorLeakTe
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
> 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.co
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 ho
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 Enha
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
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;
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 bes
> 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.co
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 tes
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
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 tra
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).
>
> @dsgr
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 Gr
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, wit
On Thu, 19 Dec 2019 16:16:28 GMT, Kevin Rushforth wrote:
>> probably not invented so often - just c&p'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
[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
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 mo
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 thi
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 memo
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
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
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
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
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 mo
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
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 on
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 indica
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:
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, o
> 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.co
On Thu, 19 Dec 2019 13:39:07 GMT, Jeanette Winzenburg
wrote:
>> Yes, do you know an API for registerChangeListener which allows me to only
>> remove a specific listener?
>
> no way, afaik - there had been debates when the api was added somewhere in
> jbs, don't recall exactly where
here's wh
On Thu, 19 Dec 2019 13:35:51 GMT, Florian Kirmaier
wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java
>> line 555:
>>
>>> 554: }
>>> 555:
>>> 556: private void setFillOverride(Paint fillOverride) {
>>
>> just a side-note: this
On Thu, 19 Dec 2019 13:23:25 GMT, Jeanette Winzenburg
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
On Thu, 19 Dec 2019 13:28:07 GMT, Jeanette Winzenburg
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
On Thu, 19 Dec 2019 12:35:56 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 compl
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/JMemoryBudd
60 matches
Mail list logo
