On Tue, 25 Oct 2022 17:22:44 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>>> Thank you. You do bring a good point - clearing the map. There is some mild >>> language warning about clearing the map in javadoc already, but perhaps >>> `Node.getProperties().clear()` should be forbidden (throw an IOE?). >> >> A map can also be cleared with `Map.keySet().clear()`, or by enumerating its >> keys and removing each mapping one by one. >> I don't think we can make the `getProperties()` map mutable, but at the same >> time safe to store final objects, while still respecting the `Map` contract. >> >> Maybe we could have something like "hidden" keys, which are not enumerable >> and are never cleared by bulk operations. > > I think we can conclude here that this is a bit outside the scope of this PR. > > I certainly wouldn't be in favor of wrapping the property map to prevent > clearing or any such thing; if we want to save space in `Node` this way, a > dedicated structure that can't be accessed by users would be the way to go in > my opinion. The space we save should compensate for the pointer to this > structure. > > Before we'd even consider that though, I'd like to see where most memory is > used in FX so efforts can be focused in areas where it would net the greatest > effect. For example, Properties are far more numerous than nodes. > > I noticed the use of the properties map by JavaFX itself -- the API states > that this is the case, but I wonder if that's really required. It might be > better to keep these things in `WeakHashMap`, which is intended for this > purpose instead of strewing these properties about over the various nodes > which are then forced to instantiate the properties map vs just a single > `WeakHashMap`. > > For example, `ButtonBarSkin` seems to do it to keep track of a variable > amount of listeners. It effectively creates the properties map for each > button added (assuming the map is usually empty) vs a single `WeakHashMap`; > also it does this on an `ObservableList`. This looks like something that > could be a standard pattern (given an observable collection, automatically > add/remove a given listener as the collection changes). ReactFX had > `Subscription#dynamic` for this. > > It also looks like `ButtonBarSkin` might be buggy, which it wouldn't have > been if had used a `WeakHashMap`. I see no `dispose` method and the > listeners added don't use the `register` system, which means it can leave > behind listeners that it stores in properties. A new skin may conclude the > listener on some buttons is already present. Or an old skin may not get > garbage collected. FYI, `ButtonBarSkin` does have memory leak issues, which are addressed in https://github.com/openjdk/jfx/pull/921 ------------- PR: https://git.openjdk.org/jfx/pull/830
