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
I left a few minor formatting comments.
And in case it is helpful, here is a list of files with the `else` on a
separate line from the closing brace of the `if`:
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/AbstractListenerManagerTest.java
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/OldValueCachingListenerManagerTest.java
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
396:
> 394: }
> 395: }
> 396:
Very minor: extra blank line.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerList.java line
80:
> 78: notifyWhileLocked(observableValue, oldValue, wasLocked);
> 79: }
> 80: finally {
Suggestion:
} finally {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 472:
> 470: listener.invalidated(instance);
> 471: }
> 472: catch (Exception e) {
Suggestion:
} catch (Exception e) {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 481:
> 479: changeListener.changed(instance, oldValue, newValue);
> 480: }
> 481: catch (Exception e) {
Suggestion:
} catch (Exception e) {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManagerBase.java
line 245:
> 243: ListenerListBase.callInvalidationListener(instance,
> listener);
> 244: }
> 245: finally {
Suggestion:
} finally {
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperUtility.java
line 148:
> 146: return List.of((ChangeListener<T>) field.get(helper));
> 147: }
> 148: catch(Exception e) {}
Minor formatting:
} catch (Exception e) {}
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperUtility.java
line 361:
> 359: return field.get(bean);
> 360: }
> 361: catch(Exception ex2) {
Minor formatting:
Suggestion:
} catch (Exception ex2) {
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
line 222:
> 220: assertListeners(list, List.of(il1, weakListener));
> 221:
> 222: for(int i = 0; i < 100; i++) {
Minor: `for(` --> `for (`
Here and elsewhere in this file
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
line 293:
> 291: double nextDouble = rnd.nextDouble();
> 292:
> 293: if(nextDouble < 0.05) {
Minor: `if(` --> `if (`
Here and elsewhere in this file (
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListTestBase.java
line 153:
> 151: assertEquals(4, exceptions.get());
> 152: }
> 153: finally {
Suggestion:
} finally {
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 260:
> 258: }
> 259: }
> 260: finally {
Suggestion:
} finally {
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-5359644575
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139270647
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139285789
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139276886
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139280033
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139288999
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139334972
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139338880
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139299158
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139296687
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139342221
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4139344430
