On Mon, 24 Feb 2020 09:59:28 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> As far as I know a LinkedHashMap doesn't automatically remove weak listener >> entries. The listener maps can be holding weak listeners as well as normal >> listeners. >> So we need to keep the original behaviour from before where a count was >> checked on every addListener call and the weak references were purged from >> the array using the original calculation for this count. >> Otherwise the map would never garbage collect these weak references. >> >> The initial value of 2 for these counts was just the starting count although >> this is not necessarily strictly accurate. To be completely accurate then we >> would have to set the appropriate count in each constructor as follows: >> >> i.e. in the Constructor with 2 InvalidationListeners: >> weakChangeListenerGcCount = 0 >> weakInvalidationListenerGcCount = 2 >> >> in the Constructor with 2 ChangeListeners: >> weakChangeListenerGcCount = 2 >> weakInvalidationListenerGcCount = 0 >> >> in the Constructor with 1 InvalidationListener and 1 ChangeListener: >> weakChangeListenerGcCount = 1 >> weakInvalidationListenerGcCount = 1 >> >> Now, I could have used a WeakHashMap to store the listeners where it would >> automatically purge weak listeners but this doesn't maintain insertion >> order. Even though the specification doesn't mandate that listeners should >> be called back in the order they are registered, the unit tests failed when >> I didn't maintain order. >> >> I am happy to remove this weak listener purge code (as it would make the >> code much simpler) but then we wouldn't automatically remove the weak >> listeners, but this may not be deemed a problem anyway? > >> So we need to keep the original behaviour from before where a count was >> checked on every addListener call and the weak references were purged from >> the array using the original calculation for this count. > > The GC'd weak listeners do need to be purged, but why is the original > behavior of the counters still applicable? Just wanted to keep a similar behaviour as before using the same calculation based originally on the size of the listeners array list and now based on the size of the map. So in theory the weak listeners should be trimmed at the same rate as before. Happy to hear alternatives. >> Nope, actually: >> `listeners.keySet().removeIf(p::test) ` >> >> is not the same as: >> `listeners.entrySet().removeIf(e -> p.test(e.getKey()));` >> >> We need to test against the entrySet.key not the entrySet itself. > >> We need to test against the entrySet.key not the entrySet itself. > > I suggested to test against the elements in `keySet()`, which are the same as > the ones in `entrySet().getKey()`. Gotcha, sorry I missed that. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
