On Thu, 6 Mar 2025 16:21:33 GMT, John Hendrikx <jhendr...@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
