On Thu, 18 Dec 2025 02:19:03 GMT, John Hendrikx <[email protected]> wrote:
>> Fixes a problem with leaked references in XYChart. A few things conspire >> here to make this into a memory leak: >> >> - The weak bindings used by StringBinding leave behind listener "stubs" when >> GC'd; that's just how they work, it is a "strong" listener that wraps a weak >> referenced listener. The strong part remains behind, and is cleaned up when >> a new listener is added/removed by ExpressionHelper (and if that never >> happens, those stubs remain there indefinitely). >> - The fluent bindings (map/flatMap/orElse) use normal listeners, but only >> when they are observed themselves (lazy listeners) >> - The "stub" that is left behind counts as being observed, so the fluent >> bindings don't unsubscribe themselves >> >> The leak has nothing to do with the node or the accessible property, but >> purely by the StringBinding leaving behind stubs on the flat mapped >> properties. >> >> The leak is actually because the Series to which the Data object belongs is >> referencing the Data object. The flatMaps track the series object so they >> added a listener to the series object, and they think they are observed >> indefinitely because of the listener stub. >> >> The easiest solution here is to ensure the Series object is not tracked when >> not needed; this can be achieved by setting the series to `null` in the >> ListChangeListener for the Data list. > > John Hendrikx has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Fix memory leak Excellent work, the memory leak is fixed, thank you so much @hjohn ! <img width="706" height="421" alt="Screenshot 2025-12-18 at 08 36 17" src="https://github.com/user-attachments/assets/f555a51d-c446-4a55-bbbf-469afddb9f68"; /> Questions: 1. the seriesProperty holds the same object as the series field, can we remove the field? 2. could we apply the same treatment to PieChart (perhaps in a followup, unless it's an easy fix)? 3. perhaps, as a followup, we ought to add a parameterized test to make sure all other charts don't exhibit the same issue? ------------- PR Comment: https://git.openjdk.org/jfx/pull/2013#issuecomment-3671158947
