On Tue, 4 Apr 2023 15:22:48 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
> This provides and uses a new implementation of `ExpressionHelper`, called
> `ListenerManager` with improved semantics.
>
> # 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 overhead|
> |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per
> listener (excluding unused slots)|61 + 4 per listener (excluding unused
> slots)|
>
> # About nested changes
>
> Nested changes are simply changes that are made to a property that is
> currently in the process of notifying its listeners. This...
This change is a great improvement over the current implementation, I'm looking
forward to it.
Some comments below:
`ListenerManager` is an obvious improvement, as it fixes incorrect behavior and
allows listeners to veto changes. However, the behavior of `ListenerManager` is
also an implementation detail and not documented anywhere. This leads me to the
following questions:
1. How will users know that they can now do all of the things that were
previously broken? Do we need a specification for what is allowed and what's
not allowed?
2. Should this behavior be required for all valid `ObservableValue`
implementations? (This would render many existing implementations defective.)
3. If `ObservableValue` implementations are not required to replicate the
`ListenerManager` behavior, we should probably make it easily discoverable
whether any particular implementation (most of them are properties) supports
nested changes/vetoing. In most of the public API, there's no obvious way to
see (without looking at the source code) whether a property implementation
extends one of the `*PropertyBase` classes.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
58:
> 56: * Constructs a new instance.
> 57: *
> 58: * @param accessor an {@link Accessor}, cannot be {@code null}
There is no `accessor` parameter.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
94:
> 92: *
> 93: * @param instance the instance it is located in, cannot be {@code
> null}
> 94: * @param array the occupied slots of the array to set
The parameter is named `occupiedSlots`, not `array`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
168:
> 166: * @param instance a reference to the instance where the array is
> stored, cannot be {@code null}
> 167: * @param index an index to remove, cannot be negative, or greater
> than or equal to the number of occupied slots
> 168: * @returns the element that was removed, can be {@code null} if the
> element at the given index was {@code null}
Should be `@return`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
217:
> 215: * @param instance a reference to the instance where the array is
> stored, cannot be {@code null}
> 216: * @param index an index, cannot be negative, or greater than or
> equal to the number of occupied slots
> 217: * @returns the element at the given index, can be {@code null} if
> the element at the given index was {@code null}
Should be `@return`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
238:
> 236: * @param index an index to set, cannot be negative, or greater than
> or equal to the number of occupied slots
> 237: * @param element an element to set, can be {@code null}
> 238: * @returns the element that was previously at the given index, can
> be {@code null} if the element at the given index was {@code null}
Should be `@return`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
374:
> 372: while (needed > max) {
> 373: min = mid;
> 374: mid = max;
These two lines don't seem to be useful, as neither `min` nor `mid` are ever
accessed after this point.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 243:
> 241:
> 242: /**
> 243: * Gets the {@link ChangeLisener} at the given index. This can be
> {@code null} if the
Typo: `ChangeListener`
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 84:
> 82: * @throws NullPointerException when listener is {@code null}
> 83: */
> 84: public void addListener(I instance, ChangeListener<? super T>
> listener) {
The code of this method is a duplicate of `addListener(I,
InvalidationListener)`.
Maybe you could use a common implementation for both overloads.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 125:
> 123:
> 124: /**
> 125: * Notifies the listeners managed in the given instance.<p>
Minor: unncessary `<p>`
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 143:
> 141: */
> 142: public void fireValueChanged(I instance, T oldValue) {
> 143: Object data = getData(instance);
The `data` value could be passed into this method, which would save a
(potentially not devirtualized) method call.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 145:
> 143: Object data = getData(instance);
> 144:
> 145: if (data instanceof ListenerList) {
Why is `ListenerList` checked first, when most observables only have a single
`InvalidationListener`?
modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerList.java
line 101:
> 99: * notification otherwise {@code false}
> 100: */
> 101: public boolean notifyListeners(ObservableValue<T> observableValue) {
The code in this method is _almost_ identical to
`ListenerList.notifyListeners(ObservableValue<T>, T)`.
Given that this method is somewhat complex, I think it would be good to use a
common implementation.
This will help with code review, and decrease the chance that both methods
diverge further with future modifications.
modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerList.java
line 164:
> 162: }
> 163:
> 164: private void callInvalidationListener(ObservableValue<?> instance,
> InvalidationListener listener) {
This method is identical to `ListenerList.callInvalidationListener`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerList.java
line 173:
> 171: }
> 172:
> 173: private void callChangeListener(ObservableValue<T> instance,
> ChangeListener<T> changeListener, T oldValue, T newValue) {
Again, _almost_ identical to `ListenerList.callChangeListener`.
modules/javafx.base/src/main/java/javafx/beans/property/ObjectPropertyBase.java
line 91:
> 89: @Override
> 90: public void addListener(InvalidationListener listener) {
> 91: LISTENER_MANAGER.addListener((ObjectPropertyBase<Object>) this,
> listener);
I think the unchecked casts here can be removed if `ListenerManagerBase` is
declared as `ListenerManagerBase<T, I extends ObservableValue<? extends T>>`,
and `OldValueCachingListenerManager` accordingly. Then the `LISTENER_MANAGER`
instance can be parameterized as `OldValueCachingListenerManager<Object,
ObjectPropertyBase<?>>`.
modules/javafx.base/src/main/java/javafx/beans/property/ReadOnlyObjectPropertyBase.java
line 66:
> 64: @Override
> 65: public void addListener(InvalidationListener listener) {
> 66: LISTENER_MANAGER.addListener((ReadOnlyObjectPropertyBase<Object>)
> this, listener);
See the comment for `ObjectPropertyBase`.
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValueBase.java
line 70:
> 68: @Override
> 69: public void addListener(InvalidationListener listener) {
> 70: LISTENER_MANAGER.addListener((ObservableValueBase<Object>) this,
> listener);
See the comment for `ObjectPropertyBase`.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ArrayManagerTest.java
line 3:
> 1: package test.com.sun.javafx.binding;
> 2:
> 3: import static org.junit.Assert.assertNull;
Wrong import
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ArrayManagerTest.java
line 91:
> 89:
> 90: @Test
> 91: void setSshouldRejectSettingIllegalIndices() {
Typo: `Sshould`
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-1386650763
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2018017130
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167686027
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167686153
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167686217
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167686291
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167686304
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167686565
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167686624
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167687366
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167687470
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272690289
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272692011
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167688825
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167688937
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167688953
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167690228
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167690295
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167690187
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167690735
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167690436
