RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs
John Hendrikx
jhendrikx at openjdk.org
Thu Jun 27 17:32:01 UTC 2024
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 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...
# Benchmarks
Here are are some performance tests. The main take away is that invalidation performs roughly the same as before, while change listener notifications, which have acquired some more complexity, are about twice as slow. Also worth nothing is that the methods timed are generally so fast that a branch mispredication can have a big impact on the absolute performance. For example, when I re-order the method that "switches" on the data type (ListenerList, InvalidationListener or ChangeListener) I can get slightly better performance for some of these, losing performance somewhere else. I've picked a balance for now that makes the code easiest to read.
The branch mispredication is also the reason that a single `ChangeListener` performs about as good as a `ListenerList` with up to 3 listeners (but of course, the single listener case consumes less memory).
ExpressionHelper ListenerManager
Benchmark (inv_chg_ListenerCounts) Mode Cnt Score Error Units Score Error Units
FireValueChanged.binding 0, 0 avgt 10 2.247 ± 0.022 ns/op 2.335 ± 0.021 ns/op
FireValueChanged.binding 0, 1 avgt 10 8.858 ± 0.136 ns/op 17.715 ± 0.092 ns/op
FireValueChanged.binding 1, 0 avgt 10 2.551 ± 0.080 ns/op 2.267 ± 0.028 ns/op
FireValueChanged.binding 0, 2 avgt 10 10.278 ± 0.148 ns/op 12.049 ± 0.103 ns/op
FireValueChanged.binding 2, 0 avgt 10 4.831 ± 0.064 ns/op 5.560 ± 0.104 ns/op
FireValueChanged.binding 2, 2 avgt 10 11.517 ± 0.114 ns/op 14.409 ± 0.511 ns/op
FireValueChanged.binding 0, 3 avgt 10 11.301 ± 0.096 ns/op 13.756 ± 0.308 ns/op
FireValueChanged.binding 3, 0 avgt 10 6.411 ± 0.246 ns/op 6.822 ± 0.105 ns/op
FireValueChanged.binding 3, 3 avgt 10 13.410 ± 0.151 ns/op 16.564 ± 0.305 ns/op
FireValueChanged.binding 0, 20 avgt 10 17.952 ± 0.182 ns/op 36.591 ± 0.344 ns/op
FireValueChanged.binding 20, 0 avgt 10 11.817 ± 0.105 ns/op 12.869 ± 0.115 ns/op
FireValueChanged.binding 20, 20 avgt 10 26.356 ± 0.344 ns/op 50.574 ± 2.497 ns/op
FireValueChanged.longBinding 0, 0 avgt 10 1.337 ± 0.029 ns/op 2.221 ± 0.022 ns/op
FireValueChanged.longBinding 0, 1 avgt 10 5.707 ± 0.084 ns/op 14.803 ± 0.051 ns/op
FireValueChanged.longBinding 1, 0 avgt 10 1.408 ± 0.011 ns/op 2.355 ± 0.029 ns/op
FireValueChanged.longBinding 0, 2 avgt 10 7.890 ± 0.080 ns/op 13.723 ± 0.329 ns/op
FireValueChanged.longBinding 2, 0 avgt 10 4.082 ± 0.054 ns/op 5.419 ± 0.107 ns/op
FireValueChanged.longBinding 2, 2 avgt 10 8.306 ± 0.133 ns/op 13.915 ± 0.206 ns/op
FireValueChanged.longBinding 0, 3 avgt 10 8.695 ± 0.118 ns/op 16.345 ± 0.172 ns/op
FireValueChanged.longBinding 3, 0 avgt 10 4.379 ± 0.063 ns/op 6.642 ± 0.155 ns/op
FireValueChanged.longBinding 3, 3 avgt 10 10.434 ± 0.242 ns/op 16.890 ± 0.188 ns/op
FireValueChanged.longBinding 0, 20 avgt 10 14.948 ± 0.172 ns/op 57.031 ± 0.631 ns/op
FireValueChanged.longBinding 20, 0 avgt 10 11.345 ± 0.106 ns/op 12.995 ± 0.162 ns/op
FireValueChanged.longBinding 20, 20 avgt 10 24.327 ± 0.606 ns/op 64.793 ± 0.903 ns/op
FireValueChanged.property 0, 0 avgt 10 3.645 ± 0.101 ns/op 3.654 ± 0.053 ns/op
FireValueChanged.property 0, 1 avgt 10 8.631 ± 0.131 ns/op 19.572 ± 0.224 ns/op
FireValueChanged.property 1, 0 avgt 10 4.208 ± 0.041 ns/op 3.742 ± 0.066 ns/op
FireValueChanged.property 0, 2 avgt 10 11.540 ± 0.345 ns/op 15.681 ± 0.240 ns/op
FireValueChanged.property 2, 0 avgt 10 6.447 ± 0.231 ns/op 7.544 ± 0.165 ns/op
FireValueChanged.property 2, 2 avgt 10 13.335 ± 0.267 ns/op 15.447 ± 0.365 ns/op
FireValueChanged.property 0, 3 avgt 10 12.325 ± 0.237 ns/op 17.656 ± 0.488 ns/op
FireValueChanged.property 3, 0 avgt 10 7.334 ± 0.087 ns/op 8.295 ± 0.151 ns/op
FireValueChanged.property 3, 3 avgt 10 15.640 ± 0.264 ns/op 18.467 ± 0.489 ns/op
FireValueChanged.property 0, 20 avgt 10 18.696 ± 0.346 ns/op 51.939 ± 0.965 ns/op
FireValueChanged.property 20, 0 avgt 10 13.002 ± 0.163 ns/op 14.507 ± 0.334 ns/op
FireValueChanged.property 20, 20 avgt 10 27.254 ± 0.328 ns/op 48.335 ± 0.916 ns/op
Keep it open
Keep it open
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1510471173
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1687501586
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1725415864
More information about the openjfx-dev
mailing list