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

[Nir Lisker]

On Mon, 17 Feb 2025 15:37:56 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...
>
> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix fix for regression :)

I've thought about it and I would like to hear what others think. I have a behavior in mind that I find intuitive, but it doesn't mean others will. It actually includes throwing SOEs when listeners fight to set a value, I don't think it's an incorrect behavior (user's fault). Using your Z, A to D order, where A and C change values,

Z 0->1
A 0->1; set 2
  Z 1->2;
  A 1->2; no change
  B 0->2
  C 0->2; sets 3
    Z 2->3
    A 2->3; if A sets 2 we will get a SOE because of recursive changes; let's say A wants value>=2 and not ==2
    B 2->3
    C 2->3; no change
    D 0->3
  D ignores set->2 event since it got a more updated value (tracked by the progress value?)
B, C, D ignore set->1 event since they got a more updated value

This should still preserve the 3 rules you outlined. Each listener reacts immediately (depth first) and always in order. This causes the last listener to act to be the one that determines the final value, and not the first one. The ignored values end up being the later ones.
This, of course, doesn't deal with removal/addition of listeners. I think that removal should work immediately as it does now, but still allow the immediate notifications prior. About addition I'm still not sure.

---

I want to take a boarder view again for a moment. I went back to https://github.com/openjdk/jfx/pull/837 to make sure I didn't drift off course. The fixes for change listeners only pertain to nested events, right? Without nested events, even the current `ExpressionListener` works correctly, yes?

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

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