RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs
Nir Lisker
nlisker at openjdk.org
Sat Apr 15 19:33:52 UTC 2023
On Tue, 4 Apr 2023 15:22:48 GMT, John Hendrikx <jhendrikx at 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 changed only the first 5 listeners are notified again (if they still exist). When the top level resumes, it continues where it left of and notifies listeners 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)
John and I discussed this off-list. I will write a short review of this change.
### Behavior
The solution implements has the following behaviors, which are compared to the current (partially-flawed) ones:
* Listeners are invoked in the order they were registered, with invalidation listeners being invoked before change listeners. This is also the current behavior. The behavior of invoking all listeners according to the order of registrations will be investigated.
* Listeners that are removed during event propagation will be removed immediately, and will not receive the event (if they hadn't already). This differs from the current behavior of removing the listeners only after the event finished (buggy implementation).
* Listeners that are added during event propagation will be effectively added after the event finishes, and will not receive the event during which they were added. This is also the current behavior (buggy implementation). The behavior of adding the listeners immediately, as is done with removal, will be investigated.
* Nested events are invoked "depth-first", meaning that the parent event propagation is halted until the nested event finishes (see below). This differs from the current behavior that takes the "breadth-first" approach - each event finishes before the nested one starts (buggy implementation).
* Nested events are only handled by listeners who received the parent event already so that they can react to the new change. Listeners that did not receive the parent event will only get a single (updated) event so that they don't react to irrelevant values. This allows vetoing, This differs from the current behavior that sends all events to all listeners (buggy implementation).
### Examples
Suppose 5 change listeners are registered when an event starts.
**Removal during an event**
L1 gets the event
L2 gets the event and removes L4
L3 gets the event and removes L2 (L2 already got the event)
L4 does not get the event (removed by L2)
L5 gets the event
final listeners: L1, L3, L5
**Addition during an event**
L1 gets the event
L2 gets the event and adds L6
L3-L5 get the event
L6 does not get the event (added by L2)
final listeners: L1 - L6
**Nested event** (value change during an event)
The observable value changes from 0 to 1
L1 gets 0->1
L2 gets 0->1
L3 gets 0->1 and sets the value to 2 (vetoing)
L1-L3 get 1->2 (nested event - listeners can react to the new change)
L4-L5 get 0->2 (parent event continues with the updated value)
**Recursive change** (see https://continuously.dev/blog/2015/02/10/val-a-better-observablevalue.html)
The code
IntegerProperty p = new SimpleIntegerProperty(0);
// L1
p.addListener((obs, old, val) -> {
if (val.intValue() > 0) {
p.set(val.intValue() - 1);
}
});
// L2
p.addListener((obs, old, val) -> System.out.println(old + " -> " + val));
p.set(2);
will trigger
L1 0->2 (set 1)
L1 2->1 (set 0)
L1 2->0
L2 is not triggered because the updated event is 0->0
Nothing is printed
instead of the current behavior that will print
1->0
2->0
0->0
### Equality check
Change events require a comparison method for the old and new value. The 2 candidates are reference equality (`==`) and object equality (`Objects#equals`). There is some inconsistency in JavaFX about how this equality check is made (it is made in a few places on a few different types). It makes sense to do `==` with primitive types, and `equals` with `String` and the primitive wrappers. For other types, it depends on their characteristics. The "safer" option is `==` because a change that is triggered by `!=` can then be tested for `!oldValue.equals(newValue)` in the listener and be vetoed; the opposite is not possible. This might mean that the user will have to give the comparison method that is desired.
Currently, `==` is used except for `String`. The behavior is preserved in this change, but will be investigated further in order to allows for more sensible change events.
### Performance
Performance both in memory and speed is either equal or slightly worse than the current one. This is because the current behavior is wrong and fixing it entails more complications. In practice, the difference should be small. Registering many listeners on the same observable is not recommended and has caused issues in the past as well. Performance is a WIP and benchmarks will be posted later.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1507215618
More information about the openjfx-dev
mailing list