On Wed, 12 Mar 2025 14:40:01 GMT, John Hendrikx <jhendr...@openjdk.org> 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 two additional
> commits since the last revision:
>
> - Change StackOverflowException to warning log
> - Support keeping last message in Logging helper
The test code is somewhat complex. I've reviewed some of it. Except for minor
formatting remarks about missing lines and spaces, it would help to add more
documentation (I gave some examples) since that will help the reader a lot;
there are several helper classes that require diving into in order to
understand what they do what they are for.
If you could give the test code a pass with these changes, it will also make
reviewing easier.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 67:
> 65:
> 66: public class ObservableValueTest {
> 67: private static final int[] PATTERN = new int[] {1, 2, 0, 50, 3, 7};
Minor: empty line after class here and other places.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 70:
> 68:
> 69: /*
> 70: * ObservableValue cases to test:
I would add that 2 values are provided to transition between for each
properties test. For bindings, a setter and a modifier are provided too. I
suggest that `Case`'s docs will outline what they are for.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 90:
> 88: Case.of(new SimpleDoubleProperty(), 0.5, 1.0, p -> p.add(2),
> (p, v) -> p.setValue(v.doubleValue() - 2)),
> 89: Case.of(new SimpleStringProperty(), "A!", "B!", p ->
> p.concat("!"), (p, v) -> p.setValue(v.substring(0, 1))),
> 90: Case.of(new SimpleObjectProperty<>(), "A!", "B!", p ->
> Bindings.createObjectBinding(() -> (p.get() + "!").intern(), p), (p, v) ->
> p.setValue(v.substring(0, 1).intern())), // intern() used to make sure
> ObjectBinding equality check works for this test
Break long line here and other places.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 118:
> 116: @ParameterizedTest
> 117: @MethodSource("inputs")
> 118: <T> void shouldIgnoreRemovingNonExistingListener(Action<T> action) {
Am I missing what is asserted in this test? Where is the "ignoring non-existing
listeners" check being done?
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 126:
> 124:
> 125: action.removeListener(obs -> {});
> 126: action.removeListener((obs, old, current) -> {});
These can use the unnamed variables pattern `_`. Also in other places.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 152:
> 150: if (changeListenerCount == 0) {
> 151: valueSetter.accept(value1);
> 152: action.assertEvents(); // when there are no change
> listeners, setting a different value (while invalid) should not trigger any
> events
You can put this comment above the line if you prefer. Personally, I tend to
put longer explanations above and more trivial notes on the same line, like
`makeChanges(0); // no-op`.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 156:
> 154:
> 155: valueSetter.accept(value2);
> 156: action.assertEvents();
Would add a comment such as "no event when setting the same value".
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 159:
> 157:
> 158: assertEquals(value2, action.getValue());
> 159: action.assertEvents();
Why do you need to `assertEvents` again? I don't see how `action.getValue()`
could have relevant side effects.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 162:
> 160:
> 161: valueSetter.accept(value2);
> 162: action.assertEvents();
Why is this needed again?
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 484:
> 482:
> 483: private static void assertCalls(Consumer<Integer> step,
> AtomicInteger calls, int... expectedCalls) {
> 484: for(int i = 0; i < expectedCalls.length; i++) {
Space after `for`s and `if`s, in other places too.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 490:
> 488: }
> 489:
> 490: static class Action<T> implements ObservableValue<T> {
Can be `private`. Same for `Case` and some others. Not that there's a big risk
of using these classes from other places in the package, but the confinement
helps the reader understand that its used specifically in this class.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 525:
> 523: }
> 524:
> 525: void setListenerCounts(int invalidationListenerCount, int
> changeListenerCount) {
I would add some docs/comments saying that this methods trims/pads to the given
number of listeners.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 545:
> 543: InvalidationListener invalidationListener = obs -> {
> 544: records.add("Invalidation of " + j);
> 545: };
Can be a a simple expression. Also in other places.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 642:
> 640: }
> 641:
> 642: static class Case<P extends Property<T>, T> {
I suggest adding some Javadoc or plain comments to this class and/or its
parameters that explain how it's used.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 669:
> 667: * Takes a list of values and creates an iterator of pairs of those
> values. The iterator
> 668: * does not only return all possible pair combinations, but also
> different transitions between
> 669: * two pairs. Effectively, given x values it returns x^3
> combinations.
Very worth noting that the pairs are used for the numbers for invalidation and
change listeners.
Also, there are duplicate pairs with the same pairs before and after, so it
seems to be repeating the same state. For example, the sequence
[1, 2]
[1, 0]
[1, 50]
repeats several times. Not sure if this is on purpose, but looks redundant.
modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java
line 723:
> 721: }
> 722:
> 723: sealed interface Record {
Maybe a doc/comment like "Records listener additions, removals, and value
changes so that they can then be checked to be correct."?
Can also be `private`.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1081#pullrequestreview-2679851206
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1992282573
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1992627970
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1992432871
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134145247
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134118437
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134148752
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134147713
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134151713
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134151771
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1992578749
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134116370
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134101554
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134118842
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134086519
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1992613836
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r2134122360
