On Fri, 13 Jan 2023 09:25:24 GMT, Florian Kirmaier <[email protected]>
wrote:
>> Florian Kirmaier has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains eight commits:
>>
>> - Merge remote-tracking branch 'origjfx/master' into
>> JDK-8277848-list-binding-leak
>>
>> # Conflicts:
>> #
>> modules/javafx.base/src/main/java/javafx/beans/property/ListPropertyBase.java
>> #
>> modules/javafx.base/src/main/java/javafx/beans/property/SetPropertyBase.java
>> #
>> modules/javafx.base/src/test/java/test/javafx/beans/property/SetPropertyBaseTest.java
>> - Merge remote-tracking branch 'origjfx/master' into
>> JDK-8277848-list-binding-leak
>> - JDK-8277848
>> Added tests to ensure no premature garbage collection is happening.
>> - JDK-8277848
>> Added 3 more tests to verify that a bug discussed in the PR does not
>> appear.
>> - JDK-8277848
>> Added the 3 requests whitespaces
>> - JDK-8277848
>> Further optimization based code review.
>> This Bugfix should now event improve the performance
>> - JDK-8277848
>> Added missing change
>> - JDK-8277848
>> Fixed memoryleak, when binding and unbinding a ListProperty. The same was
>> fixed for SetProperty and MapProperty.
>
> To my knowledge, many memory leaks, are not really possible to implement
> elegantly without weak references.
> For that reason, I usually come from the opposite direction, and ask myself,
> how can I test code with WeakReferences properly.
@FlorianKirmaier
> To my knowledge, many memory leaks, are not really possible to implement
> elegantly without weak references. For that reason, I usually come from the
> opposite direction, and ask myself, how can I test code with WeakReferences
> properly.
They aren't leaks when you asked for them to be created. Unexpected memory
leaks are the real problem, and JavaFX is the culprit because it does lots of
unexpected things:
It stems from the standard classes JavaFX gives us. A `ListProperty` registers
**eagerly** upon construction with an `ObservableList`. If I create a 100 of
them in a loop, none of them disappear (assuming no weak listeners are used).
Why does it do this when those properties are unused? The problem of memory
leaks is not so much that they're real memory leaks, the problem is that the
user doesn't **expect** there to be leaks from just the construction of a
component, because that's not how Java works (even `new Thread` requires
`start` before the thread "leaks"):
new ListProperty<>(observableList); // -> memory leak, eager listener
registration occurred
The above causing a leak is indeed unexpected. But now take this:
new ListProperty<>(observableList).emptyProperty().addListener( ... );
This is not a leak. The listener was added by the user, and it should keep
working until the user removes it. The user expects this to work, and can
reasonable expect there to be some memory allocated to keep it working, until
such time the user undoes this action.
Now, the first example can be fixed in two ways:
1) Use weak listeners.
2) Don't register listeners until the user actually needs them
Option 1 breaks the second example and leads to **new** problems; those new
problems are even harder to debug than a simple memory leak, as all the
elaborate tests all over the JavaFX code base that rely on `System.gc()`
clearly show. It doesn't solve the problem fully either, so now we have two
problems: Unexpected memory leaks are still a problem (as JavaFX keeps doing
things that are "unexpected" from a user perspective), and we now have to deal
with unexpected memory reclamation because some vague rule was broken that you
must keep a reference to some things, but not others (see other post that you
can't rely on this at all when dealing with API's that provide properties).
Option 2 leads to several benefits; no listeners being registered until needed
means less memory use, less "events" being send between components
unnecessarily and the main benefit, no memory leaks that the user didn't
specifically ask for. This is how a major problem was fixed in `Node` where
listeners were registered on `Scene` and `Window` for each and every node in a
scene. Those listeners weren't necessary as they weren't actually used by the
user, yet they were eagerly registered causing tens of thousands of listeners
(on a single property) for large scenes.
Use of weak listeners are a cure worse than the disease, because the second
case now breaks randomly. We've traded a fairly tractable problem with a far
worse problem. Weak references have their place, but using them to second
guess the users idea of what should be in memory and what shouldn't be is not
one of them. Their use cases are limited to associating data with objects
whose life times or fields you do not control, and to respond to memory
pressure. Using them to clean up eagerly registered listeners to fix
unexpected coupling between components is beyond ridiculous. Memory leaks have
been replaced with overzealous memory reclamation, to great surprise of the
user.
-------------
PR: https://git.openjdk.org/jfx/pull/689
