On Fri, 10 Jul 2026 15:24:49 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 six additional
> commits since the last revision:
>
> - Ensure change/invalidation listeners are not mixed up
> - Clarify ArrayManager docs
> - Update copyright year on new files
> - Clarify occupied slots
> - Add missing LF at end of file
> - Sealed ListenerManagerBase
more comments, thanks for addressing the earlier ones!
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 72:
> 70:
> 71: private void addAnyListener(I instance, Object listener) {
> 72: Objects.requireNonNull(listener);
this class makes no distinction between `InvalidationListener` and
`ChangeListener`. should it?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManagerBase.java
line 63:
> 61: protected abstract void setData(I instance, Object data);
> 62:
> 63: protected abstract void addInvalidationListener(I instance,
> InvalidationListener listener);
this is an internal sealed class, so it's probably ok not to add javadoc to
protected methods... still, there were javadocs. might be helpful in the
future.
modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerManager.java
line 253:
> 251: }
> 252: }
> 253: }
missing newline
modules/javafx.base/src/main/java/javafx/beans/binding/BooleanBinding.java line
190:
> 188: public final void invalidate() {
> 189: if (valid) {
> 190: boolean oldValue = value;
question: how does it work if you set(1)-invalidate-set(2)-invalidate ?
modules/javafx.base/src/main/java/javafx/beans/property/LongPropertyBase.java
line 2:
> 1: /*
> 2: * Copyright (c) 2011, 2025, Oracle and/or its affiliates. All rights
> reserved.
in the year 252525... I know we have a script, but it also might create
merging problems...
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ListenerListBaseTest.java
line 180:
> 178:
> 179: @Test
> 180: void shouldNeverSilentlyRemoveWeakListeners() {
question: does this tests the public specification, or the implementation?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-4673823824
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3561000817
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3560977880
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3561075679
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3561129347
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3561525097
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3561619298
