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

Nir Lisker nlisker at openjdk.org
Mon Mar 10 02:37:08 UTC 2025


On Thu, 6 Mar 2025 16:21:33 GMT, John Hendrikx <jhendrikx at openjdk.org> 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:
> 
>   Fix non-convergence logic one more time...

First part of the review.

There are several class (and their methods) that are `public`, but are only used in their package and can just have package-access:
`OldValueCachingListenerList`
`ListenerManagerBase`
`ListenerListBase`
`ListenerList`
`ArrayManager`

If they are `public` because of tests, please add a comment like "public for testing purpose".

modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java line 45:

> 43:  * a {@link ListenerList}. It is recommended to never inspect this field directly
> 44:  * but always use this manager to interact with it.
> 45:  *

I suggest adding a line that says that this class is used by bindings.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java line 49:

> 47:  * @param <I> the type of the instance providing listener data
> 48:  */
> 49: public abstract class ListenerManager<T, I extends ObservableValue<? extends T>> extends ListenerManagerBase<T, I> {

Same comments from `OldValueCachingListenerManager`.

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerList.java line 41:

> 39:  */
> 40: public class OldValueCachingListenerList<T> extends ListenerList<T> {
> 41:     private T latestValue;

Empty line after class declaration.

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 43:

> 41:  * only a single invalidation listener, the field will contain only that
> 42:  * listener (change listeners are wrapped to track old value). When there is more
> 43:  * than one listener, the field will hold a {@link OldValueCachingListenerList}. It

Suggestion:

 * than one listener, the field will hold an {@link OldValueCachingListenerList}. It

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 53:

> 51:  * within listener list. If possible use {@link ListenerManager}, as it has less
> 52:  * storage requirements and is faster.
> 53:  *

I suggest adding a line that says that this class is used by properties.

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 64:

> 62:      * @param instance the instance to which the listeners belong, cannot be {@code null}
> 63:      * @param listener a listener to add, cannot be {@code null}
> 64:      * @throws NullPointerException when listener is {@code null}

And when `instance` is too, no? One will throw implicitly and the other explicitly. Does it matter for the docs?

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 90:

> 88:         else {
> 89:             setData(instance, new OldValueCachingListenerList<>(data, listener));
> 90:         }

This can now be a pattern matching `switch`:

        switch (data) {
            case null -> setData(instance, listener);
            case OldValueCachingListenerList<?> list -> list.add(listener);
            case ChangeListenerWrapper<?> wrapper -> {
                var list = new OldValueCachingListenerList<>(wrapper.listener, listener);

                list.putLatestValue(wrapper.latestValue);

                setData(instance, list);
            }
            default -> setData(instance, new OldValueCachingListenerList<>(data, listener));
        }

Note that in the case of `ChangeListenerWrapper` (in your code too) there's no compile-time need to cast to a `T` type because `OldValueCachingListenerList` takes `Object`s and so does `setData`. Is this a required runtime check?

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 100:

> 98:      * @throws NullPointerException when listener is {@code null}
> 99:      */
> 100:     public void addListener(I instance, ChangeListener<? super T> listener) {

Same remarks from the invalidation listener method, but does `instance` not need a `null` check or does `getData` do that? It's not so clear.

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 143:

> 141:      * @throws NullPointerException when listener is {@code null}
> 142:      */
> 143:     public void removeListener(I instance, Object listener) {

Same comments.

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 168:

> 166:      *   can be {@code null} which means there are no listeners to notify
> 167:      */
> 168:     public void fireValueChanged(I instance, Object listenerData) {

Same comments. `null` check on `instance`?

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 206:

> 204:     }
> 205: 
> 206:     static class ChangeListenerWrapper<T> implements ChangeListener<T> {

Can be `private`?

modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java line 207:

> 205: 
> 206:     static class ChangeListenerWrapper<T> implements ChangeListener<T> {
> 207:         private final ChangeListener<T> listener;

Empty line.

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

PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-2669538281
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986508160
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986507024
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986433772
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986436187
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986508555
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986437446
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986435854
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986437897
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986440318
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986443411
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986495228
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1986444995


More information about the openjfx-dev mailing list