On Thu, 13 Apr 2023 15:58:32 GMT, Nir Lisker <nlis...@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 >> 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 and I discussed this off-list. I will write a short review of this > change. > > ### Behavior > The solution implements has the following behaviors, which are compared to > the current (partially-flawed) ones: > * Listeners are invoked in the order they were registered, with invalidation > listeners being invoked before change listeners. This is also the current > behavior. The behavior of invoking all listeners according to the order of > registrations will be investigated. > * Listeners that are removed during event propagation will be removed > immediately, and will not receive the event (if they hadn't already). This > differs from the current behavior of removing the listeners only after the > event finished (buggy implementation). > * Listeners that are added during event propagation will be effectively added > after the event finishes, and will not receive the event during which they > were added. This is also the current behavior (buggy implementation). The > behavior of adding the listeners immediately, as is done with removal, will > be investigated. > * Nested events are invoked "depth-first", meaning that the parent event > propagation is halted until the nested event finishes (see below). This > differs from the current behavior that takes the "breadth-first" approach - > each event finishes before the nested one starts (buggy implementation). > * Nested events are only handled by listeners who received the parent event > already so that they can react to the new change. Listeners that did not > receive the parent event will only get a single (updated) event so that they > don't react to irrelevant values. This allows vetoing, This differs from the > current behavior that sends all events to all listeners (buggy > implementation). > > ### Examples > Suppose 5 change listeners are registered when an event starts. > **Removal during an event** > > L1 gets the event > L2 gets the event and removes L4 > L3 gets the event and removes L2 (L2 already got the event) > L4 does not get the event (removed by L2) > L5 gets the event > final listeners: L1, L3, L5 > > **Addition during an event** > > L1 gets the event > L2 gets the event and adds L6 > L3-L5 get the event > L6 does not get the event (added by L2) > final listeners: L1 - L6 > > **Nested event** (value change during an event) > > The observable value changes from 0 to 1 > L1 gets 0->1 > L2 gets 0->1 > L3 gets 0->1 and sets the value to 2 (vetoing) > L1-L3 get 1->2 (nested event - listeners can react to the new change) > L4-L5 get 0->2 (parent event continues with the updated value) > > **Recurs... @nlisker I think I addressed all the issues, do you think it can move forward? ------------- PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-1625396623
