RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v3]

[John Hendrikx]

On Sat, 1 Feb 2025 12:57:34 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...
>
> John Hendrikx has updated the pull request incrementally with five additional commits since the last revision:
> 
>  - Clean-up and add tests
>  - Pass in listener data directly for fireValueChanged calls
>  - Fix documentation in a few places
>  - Remove unused interface
>  - Update copyrights and add missing

Thanks for taking a look again!

> I'd say that the biggest surprise a user would have is about the nested addition of listeners. By the way, your table says that added listeners don't receive any event, but the "The PR" section says that they do. I've observed that they don't. I think that not receiving events for added listeners is confusing (we discussed this briefly under #837). Is there a good reason for this? It prevents SO exceptions in some cases, but I would say that that's the user's fault. I need to think about this more, but was wondering what you concluded.

What part in the PR section?  I found this:

> Added listeners are only called when a new non-nested (top level) notification starts

Both `ExpressionHelper` and this implementation will not notify an added listener within a callback for the same notification.  `ExpressionHelper` will notify the newly added listener though if a *nested* change occurs before the original notification completes, while this implementation considers this to be integral with the first change.

These are rare cases, and to observe this you would, as part of a single notification, have to a) add a listener, and b) change the value again.  This is currently just an artifact of the implementation and is not specified anywhere.

But let's see what we could do.  For example, if I have 5 listeners, the top level notification would be A, B, C, D, E.  If C decides to change the value, then it would for `ExpressionHelper` look like:

     ExpressionHelper | time ----------------------------->
     change 1: A B C             D E (the last two with incorrect old values)
     change 2:        A B C D E (the last two with incorrect old values)

For this implementation it would look like:

     PR implementation | time ----------------------------->
     change 1: A B C         D E
     change 2:        A B C

Now, if you added a new listener `F` in `B`, then it would change nothing in the current PR.  `ExpressionHelper` however would look like:

     ExpressionHelper | time ----------------------------->
     change 1: A B C               D E (the last two with incorrect old values)
     change 2:        A B C D E F (the last three with incorrect old values)

`F` gets called as part of the nested change, but it then continues notifying listeners from the original chain, but not again `F`.

So, for the current implementation, you can clearly see that the notifications for the 2nd change only involve the listeners notified so far (A, B and C).  In that 2nd change there is no logical place to call the newly added `F`; trying to fit it in there would break most of the logic that ensures correct old values, or would require something nasty like inserting `F` in a specific spot instead of at the end of the listener list...

But perhaps we can call it where it does seem to fit nicely, after `E`.  You would get:

     change 1: A B C         D E F
     change 2:        A B C

Since `F` is now the last listener, and the last listener is in this implementation always called only as part of the top level change (unless it makes another modification), the only place where we could reasonably call it is at the top level (even if the listener was added during a deeper nested notification).
     
Let's say we indeed implement this, then there are some consequences:
- It still won't match `ExpressionHelper` :)
- Even in non-nested cases, a newly added `F` would get notified as part of the ongoing notification (I won't be able to distinguish at what level it was added, nor if there was a nested change made)
- The most common case (listener added as part of notification, but no nested change done) now differs significantly from `ExpressionHelper` as the newly added listener is called immediately, while `ExpressionHelper` only does that if there was a nested change

Let me know what you think.  I did not look if this viable to implement, but I'm pretty sure it could be done.

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2662001787