On Mon, 17 Feb 2025 15:37:56 GMT, John Hendrikx <jhendr...@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 one additional
> commit since the last revision:
>
> Fix fix for regression :)
> > I'm going to assume that A and B are aware of each other, and are somehow
> > working together, as this scenario makes little sense otherwise. The code
> > that is using these two listeners is then likely coming from the same
> > implementation (how else would A have a reference to B).
>
> Yes, here is an example:
>
> ```java
> var property = new SimpleIntegerProperty(0);
>
> ChangeListener<? super Number> listenerB = (obs, ov, nv) -> {
> };
>
> ChangeListener<? super Number> listenerA = (obs, ov, nv) -> {
> property.set(5);
> property.removeListener(listenerB);
> };
>
> property.addListener(listenerA);
> property.addListener(listenerB);
>
> property.set(1);
> ```
>
> > So, given that, if an implementation decides to remove B, no matter when
> > that happens exactly (outside a notification call, or within one), would it
> > be reasonable to expect a callback still?
>
> Here is how I walked through this in my head:
>
> ```
> A gets 0->1; sets 5
> A gets 1->5; setting 5 does nothing
> B gets 0->5
> A removes B (in the 1->5 event)
> A removes B (in the 0->1 event)
> ```
I see, so from reading the code, you see `property.set(5)` and then assume (not
unjustified) that everyone will first see the value change to `5`. Then the
removal of B happens. I can see how that is a logical thought process. What
exactly happens however is more complicated, and I guess that's why a warning
to not modify the same property within a callback is still justified even with
this new implementation.
There may be a way to make this a bit more logical, I think I could offer an
alternative solution. I've added a passive listener C after B and a passive
listener Z before A to ensure they don't receive bad values:
- (`progress` is set to 0 and we call Z)
- Z gets 0 -> 1
- (`progress` is set to 1 and we call A)
- A gets 0 -> 1; sets 5 triggering a new notification
- (nested level starts, but continues with next listener instead of from
start, communicated down with the `progress` field)
- (`progress` is set to 2 and we call B)
- B gets 0 -> 5
- (`progress` is set to 3 and we call C)
- C gets 0 -> 5
- (no more listeners, set progress to `-1` to indicate a nested notification
occurred and exit)
- (we now arrive back in the listener A code)
- A removes B
- (listener callback A has now completed and returned control back to the
top-level notification loop; this loop notices there was a nested notification
as `progress` variable now contains `-1` instead of the index; it is going to
assume that every subsequent listener was notified already by the nested level,
and so breaks off its loop not continuing with B or C)
- (an explicit check is done if the last called listener from its perspective
(A) resulted in the value changing; normally yes, otherwise there was no nested
call, but in the nested call it could have been modified again back to the
value A last received)
- (if the current value (5 in this case) is not the same as what A received (1)
then start a partial loop containing only the listeners notified so far in this
loop)
- (`progress` is set to 0 and we call Z)
- Z gets 1 -> 5
- (`progress` is set to 1 and we call A)
- A gets 1 -> 5
I've looked at this for a few hours now, but can't say yet for sure if this is
a better alternative or if it would work correctly if more listeners make
changes; I think it may work. It's sort of the opposite of what the PR
implementation is doing now.
The PR basically will restart from the first listener in a nested level and
stop when reaching the last listener notified in the parent level. The parent
level then continues where it left off. Assuming we have listeners Z, A, B, C,
D, and that listeners A and C make a change once it would look like:
Z (0 -> 1)
A (0 -> 1) wants >1 so sets 2
Z (1 -> 2)
A (1 -> 2)
B (0 -> 2)
C (0 -> 2) wants >2 so sets 3
Z (2 -> 3)
A (2 -> 3)
B (2 -> 3)
C (2 -> 3)
D (0 -> 3)
The alternative idea above sort of inverts this; a nested level continues where
the parent level left off and the parent will renotify all listeners it
notified at its level if the current value isn't what it thinks it should be:
Z (0 -> 1)
A (0 -> 1) wants >1 so sets 2
B (0 -> 2)
C (0 -> 2) wants >2 so sets 3
D (0 -> 3)
(check if value changed, renotify anyone notified in this level, in
this case B and C)
B (2 -> 3)
C (2 -> 3)
(check if value changed, renotify anyone notified in this level, in this
case Z and A)
Z (1 -> 3)
A (1 -> 3)
Now, whether this will be more logical from a user perspective remains to be
seen. It calls listeners in a different order, and doesn't always end with the
"last" listener (this could have consequences for added listeners, but its a
niche case as said before).
One thing that jumps out is that it needs even less calls to achieve the same
goal.
It's really late here, and I've been thinking about this for a few hours now; I
see some potential for this alternative, it may indeed be more logical, and I
think it wouldn't require more information to be passed between nested levels
than I do currently (currently that's the `progress` field only).
I will have to get back to you on this.
> That is, after A reacts to the new value 5, it's B's turn to react _before_
> it's removed. It could be just me though.
As you can see from the above, I think you may have a valid point here.
> > Here is a comparison table:
>
> It looks good, but it doesn't strictly contradict the example. A listener
> should stop receiving notifications immediately when it's removed, but here
> it's not removed yet and still doesn't receive notifications.
It's the nature of the PR implementation, but may be odd from a user
perspective.
> > I feel I also should point out that such dependencies between listeners
> > (where listener A removes B, or adds C or whatever) can only happen in code
> > that is aware of these other listeners. There is no way to obtain a
> > reference to a listener from the system, and so all of these scenario's
> > have perfect knowledge of what each listener does and their life cycles.
> > There can be no surprises here, listeners can't be added or removed by some
> > other class or 3rd party dependency without a reference to them.
>
> Yes, this is just another case where I'm not sure we're doing the right
> thing, not that I'm sure we're not.
It would still be nice if the system makes sense for implementations that
manage multiple listeners in this way, however, we're basically discussing
scenario's where 1 implementation for some inexplicable reason has multiple
listeners on the **same** property, and is actively adding/removing them at
random times, and then hopes it all still makes some sense (for varying
definitions of sense... :))
Perhaps the proposed alternative would improve these extremely exotic
scenario's, but we should really only consider such an alternative if it
doesn't make the far more common cases harder to grasp for users...
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2667438500
