On Sat, 26 Sep 2026 18:07:33 GMT, John Hendrikx <[email protected]> wrote:
>> This provides and uses a new implementation of `ExpressionHelper`, called
>> `ListenerManager` with improved semantics.
>>
>> See also #837 for a previous attempt which instead of triggering nested
>> emissions immediately (like this PR and `ExpressionHelper`) would wait until
>> the current emission finishes and then start a new (non-nested) emission.
>>
>> # 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 overhe...
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update documentation according to review comments
The API docs changes looks good. I left a couple minor comments. Once you
answer them, please create the CSR. It looks ready.
The implementation changes look good. The more I think about it, the more I
like leaving that last problem (finding 5) for a follow-on.
I have a few minor code style issues that I'll report in the next comment.
And may I just say that this will be a _great_ improvement to how listeners --
especially change listeners -- are handled in JavaFX.
modules/javafx.base/src/main/java/javafx/beans/value/ObservableListValue.java
line 35:
> 33: * @implNote
> 34: * The implementations of this interface in the JavaFX library do not
> provide all
> 35: * of the guarantees described by {@link ObservableValue} for change
> listeners: the
In addition to the ones listed below, can you add that equal values may trigger
a notification? That guarantee from [ObservableValue lines
97-99](https://github.com/hjohn/jfx/blob/871cbc31695403dd2f9bfaaaa5353478a0d8fb33/modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java#L97-L99)
doesn't hold. This also applies to the observable map and set value interfaces.
modules/javafx.base/src/main/java/javafx/collections/ObservableList.java line
48:
> 46: * guarantees described by {@link Observable} for their invalidation
> listeners: a listener
> 47: * that is removed while a notification is in progress may still be
> notified, and a nested
> 48: * notification notifies all listeners rather than only those that have
> already been notified.
Question: you removed the Observable's guarantee about nested notifications, so
does the part about the nested notification still apply as an exception? Same
question applies to ObservableMap and ObservableSet.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-5359467729
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139126573
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139220795
