On Fri, 9 Jun 2023 12:00:06 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
>> WeakListeneres in the process|Tracked (append at end)|
>> |Removal during notification|As above|Entry is `null`ed|
>> |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 that are made to a property that is
>> currently in the process of notif...
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix generic warnings
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 143:
> 141: */
> 142: public void fireValueChanged(I instance, T oldValue) {
> 143: Object data = getData(instance);
The `data` value could be passed into this method, which would save a
(potentially not devirtualized) method call.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 145:
> 143: Object data = getData(instance);
> 144:
> 145: if (data instanceof ListenerList) {
Why is `ListenerList` checked first, when most observables only have a single
`InvalidationListener`?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272690289
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272692011
