On Sat, 1 Feb 2025 12:57:34 GMT, John Hendrikx <jhendr...@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
Changes look good to me. Some nitpick in the javadoc which was at least for me
hard to understand.
Found no regression while testing. Test Coverage looks good as well.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerList.java line
35:
> 33:
> 34: /**
> 35: * Extension of {@link ListenerListBase} which given an {@link
> ObservableValue}
I had a hard time reading this sentence. Maybe it can be rephrased, so it is
better understandable what the intent of this class is? Same in
`OldValueCachingListenerList`.
Maybe something like:
`Extension of {@link ListenerListBase}, which allows a {@link ObservableValue}
and its old value to notify all contained listeners with a depth first
approach.`
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-2595527965
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1942776507
