RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v5]
John Hendrikx
jhendrikx at openjdk.org
Sun Apr 16 09:27:47 UTC 2023
> 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)
John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
Small bug fix in OldValueCachingListenerList
- Added a test case to detect and avoid this problem
-------------
Changes:
- all: https://git.openjdk.org/jfx/pull/1081/files
- new: https://git.openjdk.org/jfx/pull/1081/files/997bdcb2..42729644
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1081&range=04
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1081&range=03-04
Stats: 84 lines in 2 files changed: 81 ins; 1 del; 2 mod
Patch: https://git.openjdk.org/jfx/pull/1081.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1081/head:pull/1081
PR: https://git.openjdk.org/jfx/pull/1081
More information about the openjfx-dev
mailing list