On Tue, 15 Sep 2026 14:57:00 GMT, John Hendrikx <[email protected]> wrote:
>> This provides and uses a new implementation of `ExpressionHelper`, called >> `ListenerManager` with improved semantics. >> >> See also #837 for a previous attempt which instead of triggering nested >> emissions immediately (like this PR and `ExpressionHelper`) would wait until >> the current emission finishes and then start a new (non-nested) emission. >> >> # 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 overhe... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Implement notifying boolean The updated docs look good with one comment inline. There are a few more corner cases in the implementation to consider. Only the first one is confirmed. Addressing the others in follow-up issues is probably OK even if they are reproducible: Confirmed cases: 1. If the sole change listener of an ObservableValue replaces itself with another change listener and makes a nested change, it will not work correctly. A) If L1 first removes itself, then adds L2, and finally makes a change, that second listener will be notified during that cycle. B) If L1 first adds L2, then removes itself, and finally makes a change, it will lose track of L2 and not notify it on a subsequent change. Unconfirmed (potential) cases: 2. Two nested changes from the same change listener callback can cause the change event to be wrong Given L1 and L2 and an initial value of 0; set value to 1; In L1 if newval == 1, change the value to 2 and then to 3; L1 will see a nested 1->2 (and miss 2-> 3), while L2 will see 0->3 (as expected); then make a top-level change to set value to 4; L1 sees 3->4 never having seen 2->3 3. Adding a second change listener from a nested change listener callback can unlock the list twice Given L1 and an initial value of 0; set value to 1; in L1 if newval == 1, change val to 2, if newval == 2, add L2. The list may be unlocked twice, leading to an exception or erroneous behavior. 4. Adding a single change listener when there are two invalidation listeners can leave the wrong cached value if the first invalidation listener vetoes the change Given IL1 and IL2 and an initial value of 0; set val to 1; In IL1, the first time it is called, add CL1 and set the property back to 0 (vetoing it); CL1 will not be notified (which is correct), but the property will remain invalid, further the ListenerManager's cached value will be incorrectly left at 1, meaning that it will miss a subsequent change to 1 if that later change is not vetoed. This is an admittedly bizarre thing to do in an invalidation listener. 5. Scalar Bindings capture the cached value before calling `onInvalidating`, which can misbehave if called reentrantly Given a binding observing a value of 0; if its dependency changes to 1, and an override of onInvalidating reads the binding and changes the dependency to 2, the nested invalidation will report 1->2 and then 0->2 after the original binding resumes. This would be a very uncommon corner case. modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 112: > 110: * </ul> > 111: * The collection property classes and collection binding classes in the > JavaFX > 112: * library do not provide all of the guarantees above. For these, a Perhaps a similar disclaimer could be given for the JavaBeans adapter properties. The JavaBean adapter properties unconditionally notify their invalidation listener when set. ------------- PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-5272316273 PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r4066832390
