On Sun, 11 Oct 2026 20:02:58 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:
>
> Remove redundant invalidation note (covered by super interface)
some nitpicks mostly.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 58:
> 56: *
> 57: * This handles compacting elements that are {@code null}, as well as
> elements that implement
> 58: * {@link WeakListener} that were garbage collected in the mean time.
Suggestion:
* {@link WeakListener} that were garbage collected in the meantime.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/AbstractListenerManagerTest.java
line 185:
> 183: ChangeListener<String> cl2 = (_, o, n) ->
> notifications.add("CL2: " + o + " -> " + n);
> 184:
> 185: helper.addListener(ov, (ChangeListener<String>) (_, o, n) -> {
This cast isn't needed. (and most of the time below as well. Sometimes it's
needed).
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ArrayManagerTest.java
line 238:
> 236: assertEquals(manager.get(ArrayManagerTest.this, 0), "A");
> 237: assertEquals(manager.get(ArrayManagerTest.this, 1), "B");
> 238: assertEquals(manager.get(ArrayManagerTest.this, 2), "C");
Isn't this the wrong order? First the expected value, which is `A`. Otherwise
the error message would be misleading.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ArrayManagerTest.java
line 248:
> 246: assertEquals(manager.get(ArrayManagerTest.this, 0), "C");
> 247: assertEquals(manager.get(ArrayManagerTest.this, 1), "D");
> 248: assertEquals(manager.get(ArrayManagerTest.this, 2), "E");
same here
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ArrayManagerTest.java
line 258:
> 256:
> 257: assertEquals(1, size);
> 258: assertEquals(manager.get(ArrayManagerTest.this, 0), "E");
same here
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
line 67:
> 65: }
> 66:
> 67: public boolean accessibleUnlock() {
the return type here is unused, so I would prefer making it `void` in the test
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
line 490:
> 488: assertListeners(list, List.of(il2, cl2));
> 489:
> 490: assertEquals(null, list.get(0));
`assertNull` would be a better fit (better error message as well)
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
line 492:
> 490: assertEquals(null, list.get(0));
> 491: assertEquals(il2, list.get(1));
> 492: assertEquals(null, list.get(2));
`assertNull`
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
line 596:
> 594:
> 595: @Override
> 596: public void changed(ObservableValue<? extends Object>
> observable, Object oldValue, Object newValue) {
Suggestion:
public void changed(ObservableValue<?> observable, Object oldValue,
Object newValue) {
`?` always extends `Object`, we may as well remove it.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListTestBase.java
line 696:
> 694:
> 695: private Pattern PATTERN = Pattern.compile("(?<listener>.*): changed
> from (?<old>.*) to (?<new>.*)");
> 696: private Pattern INVALIDATION_PATTERN =
> Pattern.compile("(?<listener>.*): current value (?<current>.*)");
I would prefer those at the start of the class (like the other attributes), but
minor.
(Also both could be static final, variable is written in uppercase, so this may
was your intention?)
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/TestLogging.java
line 42:
> 40: // This is the value that is used in the application
> 41: // other test might set it to true
> 42: Logging.setKeepLastLogRecord(false);
I hope that we can at one point remove this weird logging hack
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 385:
> 383: _ -> valueSetter.accept(action.getValue().equals(value1) ?
> value2 : value1),
> 384: calls,
> 385: new int[] {50, 25, 13, 6, 3, 2, 1, 1, 1} // won't reach 0
> as a new listener is added each time
`new int[]` can be removed here and below
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-5493187398
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4249971842
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4249993278
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250011580
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250012694
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250014175
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250023697
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250027960
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250029261
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250037780
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250056783
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250062702
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4250075886
