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

[John Hendrikx]

On Wed, 5 Mar 2025 18:06:26 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:
> 
>   Throw StackOverflowError if non-converging listener changes are detected

I've added code that will (sometimes manually, sometimes automatically) throw a `StackOverflowError` when problems with non-converging listener changes are detected.  I'm open to doing this differently, but have opted for `StackOverflowError` for two reasons:

1. ExpressionHelper would throw this (although not by choice)
2. Any "normal" exception is currently only logged as an exception thrown by the listener and so may disappear in the noise

Now, I'm not entirely sure this is the best decision, so I'm fine to changing this to a warning.  Note though that if I change it to a warning, then one (or more) listeners may be unaware that a value they set was reset (but the warning would prompt the developer to investigate).  I guess it is similar to the warning that FX emits when a property could not be reset when errors occur during binding.

Note that the non-convergence detection logic is pretty smart, as the test case where multiple listeners are trying to agree upon a value that is divisible by 3, 5, 7 and 11 still works fine; however, there may be cases that are detected as a problem too soon (especially if listeners only do their changes after X number of calls, or only if the call number is even or something weird like that).

Some of these cases may not have resulted in a stack overflow with `ExpressionHelper` (although with such tricks you can make the stack arbitrarily deep and then still "resolve" the issue before actually running out of stack).  However, the detection logic now in place does not have the luxury of just seeing if the problem resolves itself. This is because there is no actual call to the listeners involved to check if they aren't changing their behavior as the only way to do an actual call would be to call it with an old value that is equal to its new value, which we would rather not see (if we change our mind on this, then the detection logic can be removed and we can just enjoy **real** `StackOverflowError`s).

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

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