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
> WeakListeneres in the process|Tracked (append at end)|
> |Removal during notification|As above|Entry is `null`ed|
> |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 all occurs on the
> same thread, and a nested change is nothing more than the same property being
> modified, triggering its listeners again deeper in the call stack with
> another notification, while higher up the call stack a notification is still
> being handled:
>
> (top of stack)
> fireValueChangedEvent (property A) <-- nested notification
> setValue (property A) <-- listener modifies property A
> changed (Listener 1) <-- a listener called by original
> notification
> fireValueChangedEvent (property A) <-- original notification
>
> ## How do nested changes look?
>
> Let's say we have three listeners, where the middle listener changes values
> to uppercase. When changing a property with the initial value "A" to a
> lowercase "b" the listeners would see the following events:
>
> ### ExpressionHelper
> |Nesting Level|Time|Listener 1|Listener 2|Listener 3|Comment|
> |---|---|---|---|---|---|
> | 0 |T1 |A -> b| | | |
> | 0 |T2 | |A -> b| |Value is changed to B|
> | 1 |T3 |b -> B| | | A nested loop started deeper on the call stack |
> | 1 |T4 | |b -> B| | |
> | 1 |T5 | | |b -> B| |
> | 0 |T6 | | |A -> B|Top level loop is resumed|
>
> Note how the values received by the 3rd listener are non-sensical. It
> receives two changes both of which changes to B from old values that are out
> of order.
>
> ### ListenerManager (new)
> This how ListenerManager sends out these events:
> |Nesting Level|Time|Listener 1|Listener 2|Listener 3|Comment|
> |---|---|---|---|---|---|
> | 0 |T1 |A -> b| | | |
> | 0 |T2 | |A -> b| |Value is changed to B|
> | 1 |T3 |b -> B| | | A nested loop started deeper on the call stack |
> | 1 |T4 | |b -> B| | The nested loop is terminated early at this point |
> | 0 |T5 | | |A -> B|Top level loop is resumed|
>
> Note how the 3rd listener now receives an event that reflects what actually
> happened from its perspective. Also note that less events overall needed to
> be send out.
>
> # About Invocation Order
>
> A lot of code depends on the fact that an earlier registered listener of the
> same type is called **before** a later registered later of the same type. For
> listeners of different types it is a bit less clear. What is clear however
> is that invalidation and change listeners are defined by separate interfaces.
> Mixing their invocations (to conserve registration order) would not make
> sense. Historically, invalidation listeners are called before change
> listeners. No doubt, code will be (unknowingly) relying on this in today's
> JavaFX applications so changing this is not recommended. Perhaps there is
> reason to say that invalidation listeners should be called first as they're
> defined by the super interface of `ObservableValue` which introduces change
> listeners.
>
> # About Listener Add/Remove performance
>
> Many discussions have happened in the past to improve the performance of
> removing listeners, ranging from using maps to ordered data structures with
> better remove performance. Often these solutions would subtly change the
> notification order, or increase the overhead per listener significantly.
>
> But these discussions never really looked at the other consequences of having
> tens of thousands of listeners. Even if listeners could be removed in
> something approaching O(1) time (additions are already O(1) and so are not
> the issue), what about the performance of notifying that many listeners?
> That will still be O(n), and so even if JavaFX could handle addition and
> removal of that many listeners comfortably, actually using a property with
> that many listeners is still impossible as it would block the FX thread for
> far too long when sending out that many notifications.
>
> Therefore, I'm of the opinion that "fixing" this "problem" is pointless.
> Instead, having that many listeners should be considered a design flaw in the
> application. A solution that registers only a single listener that updates a
> shared model may be much more appropriate.
>
> # About Old Value Correctness
> ...and why it is important.
>
> A change listener provides a callback that gives the old and the new value.
> One may reasonably expect that these values are never the same, and one may
> reasonably expect that the given old value is the same as the previous
> invocation's new value (if there was a previous invocation).
>
> In JavaFX, many change listeners are used for important tasks, ranging from
> reverting changes in order to veto something, to registering and
> unregistering listeners on properties. Many of those change listeners do not
> care about the old value, but there are a significant number that use it and
> rely on it being correct. A common example is the registering of a listener
> on the "new" value, and removing the same listener from the "old" value in
> order to maintain a link to some other property that changes location:
>
> (obs, old, current) -> {
> if (old != null) {
> old.removeListener(x);
> }
> if (current != null) {
> current.addListener(x);
> }
> }
>
> The above code looks bug free, and it would be if the provided old values are
> always correct. Unfortunately, this does not have to be the case. When a
> nested change is made (which can be made by a user registered listener on the
> same property), `ExpressionHelper` makes no effort to ensure that for all
> registered listener the received old and new values make sense. This leads
> to listeners being notified twice with the same "new" value for example, but
> with a different old value. Imagine the above listener receives the
> following change events:
>
> scene1 becomes scene3
> scene2 becomes scene3
>
> The above code would remove its listeners from `scene1` and `scene2`, and
> register two listeners on `scene3`. This leads to the listener being called
> twice when something changes. When later the scene changes to `scene4`, it
> receives:
>
> scene3 becomes scene4
>
> Because it registered its listener twice on `scene3`, and only removes one of
> them, it now has listeners on both `scene3` and `scene4`.
>
> Clearly it is incredibly important that changes make sense, or even simple
> code that looks innocuous becomes problematic.
>
> # The PR
>
> The `ListenerManager` differs from `ExpressionHelper` in the following ways:
> - Provides correct old/new values to `ChangeListener`s under all circumstances
> - Unnecessary change events are never sent
> - Single invalidation or change listeners are inlined directly into the
> observable class (in other words, properties with only a single listener
> don't take up any extra space at all)
> - Performance is slightly worse when calling **change** listeners (but
> remember that `ExpressionHelper` is not following the contract).
> - Removed listeners are never called after being removed (even if they were
> part of the initial list when the notification triggered)
> - Added listeners are only called when a new non-nested (top level)
> notification starts
> - Locking and maintaining the listener list works a bit differently -- the
> main takeaway is that the list indices remain the same when modified during
> nested modifications, which allows using the same list no matter how deep the
> nesting
> - No reference is stored to the ObservableValue and no copy is kept of the
> current value
> - Memory use when there is more than 1 listener should be similar, and better
> when not
> - Although complicated, the code is I think much better separated, and more
> focused on accomplishing a single task:
> - `ListenerManager` manages the listener storage in property classes, and
> the transformation between the listener variants (it either uses listeners
> directly, or uses a `ListenerList` when there are multiple listeners).
> - `ListenerListBase` handles the locking and compacting of its listener
> lists.
> - `ListenerList` which extends `ListenerListBase` is only concerned with
> the recursion algorithm for notification.
> - `ArrayManager` handles resizing and reallocating of arrays.
> - There are variants of `ListenerList` and `ListenerManager` which can
> cache their old values when its not possible to supply these (this has a
> cost, and is what `ExpressionHelper` does by default).
>
> The notification mechanism deals with nested notifications by tracking how
> many listeners were notified already before the nested notification occurs.
> For example, if 5 listeners were notified, and then listener 5 makes a nested
> change, then in that nested change only the first 5 listeners are notified
> again (if they still exist). The nested loop is then terminated early, at
> which point the top level loop resumes: it continues where it left of and
> notifies listener 6 and so on. This ensures that all notifications are always
> correct, and that listeners that "veto" changes can effectively block later
> listeners from seeing those at all.
>
> For example, if the first listener always uppercases any received values,
> then any succeeding listeners will always see only uppercase values. The
> first listener receives two notifications (`X -> a` and `a -> A`), and the
> second receives only `X -> A`. Contrast this with the old
> `ExpressionHelper`, which sends odd notifications to the second listener (`a
> -> A` and `X -> A`, in that order).
>
> Unfortunately, due to a somewhat weird design choice in the PropertyBase
> classes, the strategy of not having to cache the "current value" (or old
> value) can't be used (it can only be used for Bindings for now). So there is
> another variant of this helper, called `OldValueCachingListenerHelper`, with
> some slight differences:
>
> - Has an extra field for storing the old value when there are any
> `ChangeListener`s active
> - Can't store a `ChangeListener` inline; a minimal wrapper is needed to track
> the old value (ExpressionHelper does the same)
This change is a great improvement over the current implementation, I'm looking
forward to it.
Some comments below:
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/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 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_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
