RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs

[Nir Lisker]

On Tue, 4 Apr 2023 15:22:48 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

> This provides and uses a new implementation of `ExpressionHelper`, called `ListenerManager` with improved semantics.
> 
> # Behavior
> 
> |Listener...|ExpressionHelper|ListenerManager|
> |---|---|---|
> |Invocation Order|In order they were registered, invalidation listeners always before change listeners|(unchanged)|
> |Removal during Notification|All listeners present when notification started are notified, but excluded for any nested changes|Listeners are removed immediately regardless of nesting|
> |Addition during Notification|Only listeners present when notification started are notified, but included for any nested changes|New listeners are never called during the current notification regardless of nesting|
> 
> ## Nested notifications:
> 
> | |ExpressionHelper|ListenerManager|
> |---|---|---|
> |Type|Depth first (call stack increases for each nested level)|(same)|
> |# of Calls|Listeners * Depth (using incorrect old values)|Collapses nested changes, skipping non-changes|
> |Vetoing Possible?|No|Yes|
> |Old Value correctness|Only for listeners called before listeners making nested changes|Always|
> 
> # Performance
> 
> |Listener|ExpressionHelper|ListenerManager|
> |---|---|---|
> |Addition|Array based, append in empty slot, resize as needed|(same)|
> |Removal|Array based, shift array, resize as needed|(same)|
> |Addition during notification|Array is copied, removing collected WeakListeners in the process|Appended when notification finishes|
> |Removal during notification|As above|Entry is `null`ed (to avoid moving elements in array that is being iterated)|
> |Notification completion with changes|-|Null entries (and collected WeakListeners) are removed|
> |Notifying Invalidation Listeners|1 ns each|(same)|
> |Notifying Change Listeners|1 ns each (*)|2-3 ns each|
> 
> (*) a simple for loop is close to optimal, but unfortunately does not provide correct old values
> 
> # Memory Use 
> 
> Does not include alignment, and assumes a 32-bit VM or one that is using compressed oops.
> 
> |Listener|ExpressionHelper|ListenerManager|OldValueCaching ListenerManager|
> |---|---|---|---|
> |No Listeners|none|none|none|
> |Single InvalidationListener|16 bytes overhead|none|none|
> |Single ChangeListener|20 bytes overhead|none|16 bytes overhead|
> |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per listener (excluding unused slots)|61 + 4 per listener (excluding unused slots)|
> 
> # About nested changes
> 
> Nested changes are simply changes that are made to a property that is currently in the process of notifying its listeners. This...

John and I discussed this off-list. I will write a short review of this change.

### Behavior
The solution implements has the following behaviors, which are compared to the current (partially-flawed) ones:
* Listeners are invoked in the order they were registered, with invalidation listeners being invoked before change listeners. This is also the current behavior. The behavior of invoking all listeners according to the order of registrations will be investigated.
* Listeners that are removed during event propagation will be removed immediately, and will not receive the event (if they hadn't already). This differs from the current behavior of removing the listeners only after the event finished (buggy implementation).
* Listeners that are added during event propagation will be effectively added after the event finishes, and will not receive the event during which they were added. This is also the current behavior (buggy implementation). The behavior of adding the listeners immediately, as is done with removal, will be investigated.
* Nested events are invoked "depth-first", meaning that the parent event propagation is halted until the nested event finishes (see below). This differs from the current behavior that takes the "breadth-first" approach - each event finishes before the nested one starts (buggy implementation).
* Nested events are only handled by listeners who received the parent event already so that they can react to the new change. Listeners that did not receive the parent event will only get a single (updated) event so that they don't react to irrelevant values. This allows vetoing, This differs from the current behavior that sends all events to all listeners (buggy implementation).

### Examples
Suppose 5 change listeners are registered when an event starts.
**Removal during an event**

L1 gets the event
L2 gets the event and removes L4
L3 gets the event and removes L2 (L2 already got the event)
L4 does not get the event (removed by L2)
L5 gets the event
final listeners: L1, L3, L5

**Addition during an event**

L1 gets the event
L2 gets the event and adds L6
L3-L5 get the event
L6 does not get the event (added by L2)
final listeners: L1 - L6

**Nested event** (value change during an event)

The observable value changes from 0 to 1
L1 gets 0->1
L2 gets 0->1
L3 gets 0->1 and sets the value to 2 (vetoing)
L1-L3 get 1->2 (nested event - listeners can react to the new change)
L4-L5 get 0->2 (parent event continues with the updated value)

**Recursive change** (see https://continuously.dev/blog/2015/02/10/val-a-better-observablevalue.html)
The code

IntegerProperty p = new SimpleIntegerProperty(0);
// L1
p.addListener((obs, old, val) -> {
    if (val.intValue() > 0) {
        p.set(val.intValue() - 1);
    }
});
// L2
p.addListener((obs, old, val) -> System.out.println(old + " -> " + val));
p.set(2);

will trigger

L1 0->2 (set 1)
L1 2->1 (set 0)
L1 2->0
L2 is not triggered because the updated event is 0->0
Nothing is printed

instead of the current behavior that will print

1->0
2->0
0->0


### Equality check
Change events require a comparison method for the old and new value. The 2 candidates are reference equality (`==`) and object equality (`Objects#equals`). There is some inconsistency in JavaFX about how this equality check is made (it is made in a few places on a few different types). It makes sense to do `==` with primitive types, and `equals` with `String` and the primitive wrappers. For other types, it depends on their characteristics. The "safer" option is `==` because a change that is triggered by `!=` can then be tested for `!oldValue.equals(newValue)` in the listener and be vetoed; the opposite is not possible. This might mean that the user will have to give the comparison method that is desired.

Currently, `==` is used except for `String`. The behavior is preserved in this change, but will be investigated further in order to allows for more sensible change events.


### Performance
Performance both in memory and speed is either equal or slightly worse than the current one. This is because the current behavior is wrong and fixing it entails more complications. In practice, the difference should be small. Registering many listeners on the same observable is not recommended and has caused issues in the past as well. Performance is a WIP and benchmarks will be posted later.

It's the first item on my review list for FX 23. Hope to get the time for the review queue in 1-2 weeks.

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

PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1507215618
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1934971092