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 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)
-------------
Commit messages:
- Merge remote-tracking branch 'upstream/master' into
feature/nested-emission-with-correct-old-values
- Fix generic warnings
- Fix merge
- Merge remote-tracking branch 'upstream/master' into
- Prevent removal of weak listeners during unlock
- Use an overridable method to store latest value
- Merge the recursive notification loop code
- Small bug fix in OldValueCachingListenerList
- Improve doc
- Move listener call code to ListListenerBase
- ... and 17 more: https://git.openjdk.org/jfx/compare/54005125...ffa9b299
Changes: https://git.openjdk.org/jfx/pull/1081/files
Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1081&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8290310
Stats: 4358 lines in 39 files changed: 4195 ins; 7 del; 156 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
