Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Mon, 25 Mar 2024 13:32:11 GMT, Michael Strauß wrote: > `ListenerManager` is an obvious improvement, as it fixes incorrect behavior > and allows listeners to veto changes. However, the behavior of > `ListenerManager` is also an implementation detail and not documented > anywhere. This leads me to the following questions: > > 1. How will users know that they can now do all of the things that were > previously broken? Do we need a specification for what is allowed and what's > not allowed? Currently the specification is vague enough that there's a lot of wiggle room. For example, we don't specify whether invalidation listeners are called before change listeners, yet a lot of code will be relying on that unknowingly. We also don't specify whether successive change listener calls should always be a change (ie. never get `A -> A`), or that it should match with what the previous change reported (ie. if called with `? -> B`, then the next call must be `B -> ?`). IMHO we should though. I would specify for example that: - Invalidation listeners are called before Change listeners (reason: invalidation listeners are a lower level concept defined in a higher level interface). They definitely should not be mixed (they're defined by two different interfaces). - Change listeners should (obviously as this MR fixes this) guarantee the old value makes sense: - Old value will be equal to previous new value (essential for patterns that use the old value to unregister a related listener) - Never called when old value equals new value (it's not a change then) -- this allows vetoing, and generally saves unnecessary calls We should probably also specify the order of calls (as code will again unknowingly be relying on this already): - A listener registered after a listener of the same type will always be called after earlier registered listeners (code relies on this in various ways, even in FX itself) - Listeners of different types follow a fixed order: invalidation first, changes second (code relies on this already) - The behavior of `ObservableValue`s that contain mutable values (ie. lists/sets/maps/atomics) will be undefined if those values are mutated while held by an observable (same as when you mutate keys that are currently part of a `Set`). We can also specify some behavior with regards to when an event can be received when adding and removing listeners, although I think that's less of an issue. > 2. Should this behavior be required for all valid `ObservableValue` > implementations? (This would render many existing implementations defective.) It's hard to require anything in an interface, but I think the interface should specify this regardless. Just look at an interface like `Set` that requires a specific way of implementing `hashCode`. You can violate it easily, but you will suffer the consequences when comparing sets of different types. Same with custom implementations of `ObservableValue`. You take a risk when using some unvetted 3rd party implementation. At a minimum all implementations in JavaFX should follow the specification. This will likely cover most implementations of `ObservableValue`, leaving only a few custom implementations that are not 100% spec compliant (note: a lot of the problems only occur with nested changes, which occur only in complicated code that triggers a cascade of changes, usually layout/skin/css related). A problem there are the Set/List/Map `ObservableValue` implementations. They are not observable values, they are observable collections that deserve their own interface. Invalidation listeners are fine, but value listeners make no sense. I've looked into these before, and all I can say is that they take great liberties with what is considered a "change" (ignoring even the most basic specifications). I'd recommend deprecating the observable value parts of these, and moving users towards either invalidation or the collection specific change listeners. > 3. If `ObservableValue` implementations are not required to replicate the > `ListenerManager` behavior, we should probably make it easily discoverable > whether any particular implementation (most of them are properties) supports > nested changes/vetoing. In most of the public API, there's no obvious way to > see (without looking at the source code) whether a property implementation > extends one of the `*PropertyBase` classes. I think if the implementation is in `javafx.*` it should be correct. Anyone can violate any interface (just look at custom collection implementations which often fail to follow the spec). We could provide a more lenient abstract base class or helper to make it easier to conform to the spec. - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2018972993
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx 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... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix generic warnings `ListenerManager` is an obvious improvement, as it fixes incorrect behavior and allows listeners to veto changes. However, the behavior of `ListenerManager` is also an implementation detail and not documented anywhere. This leads me to the following questions: 1. How will users know that they can now do all of the things that were previously broken? Do we need a specification for what is allowed and what's not allowed? 2. Should this behavior be required for all valid `ObservableValue` implementations? (This would render many existing implementations defective.) 3. If `ObservableValue` implementations are not required to replicate the `ListenerManager` behavior, we should probably make it easily discoverable whether any particular implementation (most of them are properties) supports nested changes/vetoing. In most of the public API, there's no obvious way to see (without looking at the source code) whether a property implementation extends one of the `*PropertyBase` classes. - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2018017130
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 8 Mar 2024 18:08:15 GMT, Marius Hanl wrote: > Tested with a big application, did not find any regression. All listeners > still work as expected, tested especially a lot of 'if something is selected, > this button should be enabled/disabled' and the like. I did not checked the > code yet, just a little bit. One question: Should I test something special/do > you see a case which could cause a problem here? I think testing it with a big application is excellent (I do the same here with my own large application), and I'm happy to hear you did not spot any regressions! This change does change (unspecified) behavior a little bit, although I think well within the documented contract of how change listeners operate (ie. applications should not be relying on the unspecified behavior). It will be hard to notice any change directly, as the changes are all related to advanced usage of listeners (adding/removing listeners during listener callbacks) or nested changes (the same property gets changed **during** a callback, directly or indirectly). 1. During a listener callback, you remove a listener that is currently being notified (ie. yourself, or any listener that may have triggered your callback that is further up the call stack) - Before this change, there is a chance that such a removed listener may still be called a few more times, despite it being removed in nested cases; its doubtful anyone is relying on that behavior, and (IMHO) these extra callbacks are more likely to break things, because things have been cleaned up (who assumes that their listener might still get called after removal?) 2. During a listener callback, you add a listener that is currently being notified (this can't be yourself, but it could be a listener on a property that triggered your callback that is further up the stack) - Before this change, this newly added listener may start participating in the current nested listener notification (ie. it would start participating halfway during a chain of nested changes). After this change, such a newly added listener would only be notified on the **next** top level change. It's unlikely anyone is relying on this behavior. 3. During a listener callback, you change the value the property it is attached to (or any property that may have led to your callback being called further up the callstack). - The most "common" scenario for this is veto-ing changes. For example, a property that can't be empty may get set to a non-empty value (or its original value) in a listener, triggering another notification. Your listener would not notice any differences (it would get called immediately with the change that you just did), but any listeners added after yours may not "see" the empty value because it was changed to something else before they were called. They will only see the new value (and the corresponding **correct** old value since last they were called -- they may also be not called at all if the value was changed back to its original, and so they would be completely unaware of the temporary change. - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1986734228
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 9 Jun 2023 12: 00: 06 GMT, John Hendrikx wrote: >> This provides and uses a new implementation of `ExpressionHelper`, called `ListenerManager` with improved semantics. >> >> # Behavior >> ZjQcmQRYFpfptBannerStart This Message Is From an External Sender This message came from outside your organization. Report Suspicious ZjQcmQRYFpfptBannerEnd On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx 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... > > John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: > > Fix generic warnings Tested with a big application, did not find any regression. All listeners still work as expected, tested especially a lot of 'if something is selected, this button should be enabled/disabled' and the like. I did not checked the code yet, just a little bit. One question: Should I test something special/do you see a case which could cause a problem here? - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1986169590
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Thu, 8 Feb 2024 21:25:41 GMT, Marius Hanl wrote: > I completely forgot about this PR, but it looks very interesting, especially > about the nested events. If helpful, I can test this soon in a bigger > application, especially for any regressions. And of course also review the > code, but need more time for that. @Maran23 both will be appreciated; I've tested this with my own FX applications for a while, and haven't encountered anything unexpected. - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1937078085
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx 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 notif... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix generic warnings It's the first item on my review list for FX 23. Hope to get the time for the review queue in 1-2 weeks. - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1934971092
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx 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 notif... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix generic warnings I completely forgot about this PR, but it looks very interesting, especially about the nested events. If helpful, I can test this soon in a bigger application, especially for any regressions. And of course also review the code, but need more time for that. - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1934958209
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Mon, 24 Jul 2023 22:09:49 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
>> line 143:
>>
>>> 141: */
>>> 142: public void fireValueChanged(I instance, T oldValue) {
>>> 143: Object data = getData(instance);
>>
>> The `data` value could be passed into this method, which would save a
>> (potentially not devirtualized) method call.
>
> Thanks, I'll look into that, it might speed up the 1 listener cases a bit.
> The same applies to OldValueCachingListenerManager#getValue I think. I know
> it isn't possible for the add/remove calls, as the data may change if they're
> nested, but for `fireValueChanged` I never really checked after going to this
> strategy.
Have you considered passing `data` directly into the method? What is your
conclusion?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1361519194
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx 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 notif... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix generic warnings Keep it open - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1725415864
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx 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 notif... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix generic warnings Keep it open - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1687501586
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Mon, 24 Jul 2023 19:56:06 GMT, Michael Strauß wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix generic warnings
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
> line 143:
>
>> 141: */
>> 142: public void fireValueChanged(I instance, T oldValue) {
>> 143: Object data = getData(instance);
>
> The `data` value could be passed into this method, which would save a
> (potentially not devirtualized) method call.
Thanks, I'll look into that, it might speed up the 1 listener cases a bit. The
same applies to OldValueCachingListenerManager#getValue I think. I know it
isn't possible for the add/remove calls, as the data may change if they're
nested, but for `fireValueChanged` I never really checked after going to this
strategy.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272805838
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Mon, 24 Jul 2023 19:58:04 GMT, Michael Strauß wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix generic warnings
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
> line 145:
>
>> 143: Object data = getData(instance);
>> 144:
>> 145: if (data instanceof ListenerList) {
>
> Why is `ListenerList` checked first, when most observables only have a single
> `InvalidationListener`?
For some (unclear to me) reason this order performs better in my benchmark,
even for the cases that only have a single invalidation listener. I've tweaked
this method extensively, with different orders, and this was about the best I
could get it. That said, the differences are small, and we can go with a more
logical order.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272801049
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx 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 notif...
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix generic warnings
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 143:
> 141: */
> 142: public void fireValueChanged(I instance, T oldValue) {
> 143: Object data = getData(instance);
The `data` value could be passed into this method, which would save a
(potentially not devirtualized) method call.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 145:
> 143: Object data = getData(instance);
> 144:
> 145: if (data instanceof ListenerList) {
Why is `ListenerList` checked first, when most observables only have a single
`InvalidationListener`?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272690289
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272692011
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
> 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 th... John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: Fix generic warnings - Changes: - all: https://git.openjdk.org/jfx/pull/1081/files - new: https://git.openjdk.org/jfx/pull/1081/files/9ac121b8..b5db37e5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1081=10 - incr: https://webrevs.openjdk.org/?repo=jfx=1081=09-10 Stats: 10 lines in 1 file changed: 3 ins; 0 del; 7 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
