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

[John Hendrikx]

On Thu, 13 Apr 2023 15:58:32 GMT, Nir Lisker <nlisker 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 WeakListeneres in the process|Tracked (append at end)|
>> |Removal during notification|As above|Entry is `null`ed|
>> |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 notif...
>
> 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)
> 
> **Recurs...

@nlisker I think I addressed all the issues, do you think it can move forward?

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

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