RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v3]
Nir Lisker
nlisker at openjdk.org
Mon Feb 17 01:32:20 UTC 2025
On Sat, 1 Feb 2025 12:57:34 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 five additional commits since the last revision:
>
> - Clean-up and add tests
> - Pass in listener data directly for fireValueChanged calls
> - Fix documentation in a few places
> - Remove unused interface
> - Update copyrights and add missing
I've tested 9 cases before and after and I've just received an odd result in one of them.
var property = new SimpleIntegerProperty(0);
ChangeListener<? super Number> listenerA = (obs, ov, nv) -> {
System.out.println("A got " + ov + "->" + nv + " (" + property.get() + ")");
System.out.println("A set->5");
property.set(5);
};
ChangeListener<? super Number> listenerB = (obs, ov, nv) -> {
System.out.println("B got " + ov + "->" + nv + " (" + property.get() + ")");
System.out.println("B removes A");
property.removeListener(listenerA);
System.out.println("B set->6");
property.set(6);
};
property.addListener(listenerA);
property.addListener(listenerB);
property.set(1);
This prints
A got 0->1 (1)
A set->5
A got 1->5 (5)
A set->5
B got 0->5 (5)
B removes A
B set->6
B got 5->null (6)
B removes A
B set->6
I don't think that `null` should be there,
I'll give a more detailed report later, but wanted to point to this early.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2661758582
More information about the openjfx-dev
mailing list