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

Wed Mar 12 01:32:10 UTC 2025

On Tue, 11 Mar 2025 06:45:58 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:
> 
>   Break up long lines of code

Finished the 2nd part of the implementation review. I didn't delve into the logic of the listener management, but it looks sane :)

I'll review the tests as the final part.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java line 96:

> 94: 
> 95:     private static final ArrayManager<ListenerListBase, InvalidationListener> INVALIDATION_LISTENERS = new CompactingArrayManager<>(InvalidationListener.class) {
> 96:         @Override

Empty line

modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java line 118:

> 116: 
> 117:     private static final ArrayManager<ListenerListBase, Object> CHANGE_LISTENERS = new CompactingArrayManager<>(Object.class) {
> 118:         @Override

Empty line

modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java line 193:

> 191:         else {
> 192:             CHANGE_LISTENERS.add(this, listener1);
> 193:         }

These can be

switch (listener1) {
    case InvalidationListener il -> INVALIDATION_LISTENERS.add(this, il);
    default -> CHANGE_LISTENERS.add(this, listener1);
}

but I don't know if it helps.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java line 422:

> 420: 
> 421:     private void assertInvalidationListenerIndex(int index) {
> 422:         assert index < invalidationListenersCount : index + " >= " + invalidationListenersCount + ", results would be undefined";

Do we allow `assert` in production code? This will require to run the application with `ea`. Or are these intended to be skipped in production and are for tests only?

modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java line 109:

> 107:     public void addListener(ChangeListener<? super T> listener) {
> 108:         observed = observed || listener != null;
> 109:         LISTENER_MANAGER.addListener(this, (ChangeListener<Object>) listener);

This cast gives a warning.

modules/javafx.base/src/main/java/javafx/beans/property/ObjectPropertyBase.java line 101:

> 99:     @Override
> 100:     public void addListener(ChangeListener<? super T> listener) {
> 101:         LISTENER_MANAGER.addListener(this, (ChangeListener<Object>) listener);

This cast gives a warning.

modules/javafx.base/src/main/java/javafx/beans/property/ReadOnlyObjectPropertyBase.java line 76:

> 74:     @Override
> 75:     public void addListener(ChangeListener<? super T> listener) {
> 76:         LISTENER_MANAGER.addListener(this, (ChangeListener<Object>) listener);

Warning

modules/javafx.base/src/main/java/javafx/beans/value/ObservableValueBase.java line 78:

> 76:     @Override
> 77:     public void addListener(ChangeListener<? super T> listener) {
> 78:         LISTENER_MANAGER.addListener(this, (ChangeListener<Object>) listener);

Warning

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

PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-2676530002
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990363868
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990363984
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990366337
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990370006
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990385681
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990385008
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990386512
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1990387231