RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]

[Andy Goryachev]

On Tue, 25 Oct 2022 17:22:44 GMT, John Hendrikx <jhendrikx at 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