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

[John Hendrikx]

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 :)

<h2>Rules</h2>
This PR was created because there is an unwritten contract for `ChangeListener`s that people expect, but that we are currently not guaranteeing. I've written these down as three rules:

Rule 1: Old value received in callback X must match new value received in callback X-1
Rule 2: Old value should never Object::equals new value (collection properties break this rule)
Rule 3: The received new value is the same as property::get

I think these rules need to become part of the specification of `ChangeListener`.

A possible further rule that I think we may want to explicitly specify somewhere is:

Rule 4: A listener is never notified after removal

<h3>Edge Case: Two listeners upon notification change the value, and cannot reach agreement</h3>

|ExpressionHelper|This PR|Specification|
|---|---|---|
|This leads to a `StackOverflowError` as new nested levels of notifications keep being added until the stack is exhausted. During this process the listeners receive old values they may never have seen before violating Rule 1|The later listener will stop being called as its last received value equals the notification value if it was changed back; the listener will not be called (to preserve Rule 2) and so is unaware that the value has changed to something it didn't set|Specify that conflicting changes lead to an exception|

Opinion: this PR should probably not silently allow this but instead throw an exception

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

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