RFR: 8277848 Binding and Unbinding to List leads to memory leak [v8]

[John Hendrikx]

On Fri, 13 Jan 2023 09:25:24 GMT, Florian Kirmaier <fkirmaier at openjdk.org> 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