RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Mon, 24 Feb 2020 08:14:02 GMT, dannygonzalez <github.com+6702882+dannygonzalez at openjdk.org> wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java line 197:
>> 
>>> 195:         private T currentValue;
>>> 196:         private int weakChangeListenerGcCount = 2;
>>> 197:         private int weakInvalidationListenerGcCount = 2;
>> 
>> Why are these set to 2 and why do you need them at all? The previous implementation needed to grow and shrink the array so it had to keep these, but `Map` takes care of this for you.
>
> I agree, I kept these in as I wasn't sure if there was a need to manually force the garbage collection of weak listeners at the same rate as the original implementation.
> Removing this would make sense to me also.
> 
> Updated my thoughts on this, see my comments below.

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?

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java line 64:
>> 
>>> 62:                 ((WeakListener)t).wasGarbageCollected();
>>> 63: 
>>> 64:         listeners.entrySet().removeIf(e -> p.test(e.getKey()));
>> 
>> This can be `listeners.keySet().removeIf(p::test);`.
>
> Agreed, will change.

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.

-------------

PR: https://git.openjdk.java.net/jfx/pull/108