On Wed, 8 Jul 2026 23:15:01 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:
>
> Fix flakey check on (asynchronous) stderr output
This is one big, far reaching PR that might need a lot of time to review. Here
is the first batch of comments, code review only.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
2:
> 1: /*
> 2: * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
2026 here and elsewhere (hopefully, it won't need to be changed to 2027)
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
48:
> 46: /**
> 47: * The minimum array size. (3 is a good number considering it would
> use exactly 24 bytes
> 48: * with compressed oops without wasted space due to alignment issues).
oops?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
85:
> 83:
> 84: /**
> 85: * Gets the occupied slots of the array under management.
it's an internal class, but "occupied slots" is confusing - the number of, or
some kind of bitmap telling which slots are occupied and which are not?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
260:
> 258: * that satisfy the given predicate. If the predicate throws errors
> or
> 259: * exceptions, these are relayed to the caller, and the state of the
> array
> 260: * will be undefined.
interesting. should it remain in some safe configuration instead?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
281:
> 279: if (filter.test(array[i])) {
> 280: shift++;
> 281: }
} else if {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
284:
> 282: else if (shift > 0) {
> 283: array[i - shift] = array[i];
> 284: array[i] = null;
this nulls unused elements only when `shift>0`. what happens if the unused
element are at the end? I think they will remain strongly referenced via
`array[]`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
315:
> 313: * completes.<p>
> 314: *
> 315: * Note: it is not allowed to change the array fields during this
> call;
minor: "array fields" is a bit confusing. array elements?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
347:
> 345: * When the minimum size of the array is 3 (MINIMUM_SIZE),
> 346: * then the resizing algorithm used here always uses specific
> 347: * sizes: 3, 7, 13, 22, 36, etc...
question: wouldn't it use more memory than necessary if the underlying memory
allocation uses power of two blocks? wouldn't it make more sense to use power
of two sized blocks?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerList.java line
178:
> 176: */
> 177:
> 178: Logging.getLogger().warning(
rant: it's amazing that we reinvent the wheel all the time instead of making a
good logging facade a part of java.
in this case, can the log output be disabled? I suspect even when disabled,
the `formatted()` will be called.
related: is `MessageFormat` a better choice for logging?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 284:
> 282: CHANGE_LISTENERS.add(this, listener); // even invalidation
> listeners go into this list when locked!
> 283: }
> 284: else {
} else {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 308:
> 306: nulledListenerCount++;
> 307: }
> 308: else {
} else {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 319:
> 317: CHANGE_LISTENERS.remove(this, index); // not
> locked, or was added during lock, so can just remove directly
> 318: }
> 319: else {
} else {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 350:
> 348: Object listener = CHANGE_LISTENERS.get(this, i);
> 349:
> 350: if (listener instanceof InvalidationListener il) {
a listener might implement both `InvalidationListener` and `ChangeListener`,
the logic here will break.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 372:
> 370: * - The notification ends
> 371: * - While unlocking, a weak listener is determined to be
> expired
> 372: * - Removing this weak listener would not go through
> proper channels and thus
weird indent
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerListBase.java
line 423:
> 421:
> 422: private int totalSize() {
> 423: return isLocked() ? lockedSize : invalidationListenersCount +
> changeListenersCount;
what happens when an invalidation listener removes all the change listeners
while it's locked? the totalSize() will report incorrect value, breaking
downstream logic, right?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 73:
> 71: }
> 72:
> 73: private void addListenerInternal(I instance, Object listener) {
what will happen when I add a listener object that implements both
`InvalidationListener` and `ChangeListener`?
what will happen when I remove it?
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 135:
> 133: callMultipleListeners(instance, list, oldValue);
> 134: }
> 135: else if (listenerData instanceof InvalidationListener il) {
} else if {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 138:
> 136: ListenerListBase.callInvalidationListener(instance, il);
> 137: }
> 138: else if (listenerData instanceof ChangeListener) {
} else if {
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 165:
> 163: setData(instance, list.getInvalidationListener(0));
> 164: }
> 165: else if (changeListenersSize == 1) {
... it must be your style, but it throws my pattern recognition. if no one
else objects, you can ignore my pain.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
line 173:
> 171: }
> 172: }
> 173: }
missing newline
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-4665176415
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553775930
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553779563
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553787329
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553910368
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553473800
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553496240
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553915605
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553929567
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554280779
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554304809
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554305757
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554306768
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554313509
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554356428
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553724159
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3553459079
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554379406
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554380854
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554395189
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3554383337
