> 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
