RFR: 8367439: Bulk change notifications for ObservableSet and ObservableMap [v2]
Andy Goryachev
angorya at openjdk.org
Thu Oct 23 22:40:27 UTC 2025
On Thu, 23 Oct 2025 22:11:04 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:
>> While a `ListChangeListener` can receive notifications for bulk operations (`addAll`, `removeAll`, `clear`, etc.), `SetChangeListener` and `MapChangeListener` only receive notifications for individual add/replace/delete operations. For example, when mappings are added to an `ObservableMap` with `putAll()`, listeners will be invoked once for each individual mapping.
>>
>> Since there is no way for a `SetChangeListener`/`MapChangeListener` to know that more changes are coming, reacting to changes becomes difficult and potentially inefficient if an expensive operation (like reconfiguring the UI) is done for each individual change instead of once for a bulk change operation.
>>
>> I think we can improve the situation by adding a new method to `SetChangeListener.Change` and `MapChangeListener.Change`:
>>
>>
>> /**
>> * Gets the next change in a series of changes.
>> * <p>
>> * Repeatedly calling this method allows a listener to fetch all subsequent changes of a bulk
>> * map modification that would otherwise be reported as repeated invocations of the listener.
>> * If the listener only fetches some of the pending changes, the rest of the changes will be
>> * reported with subsequent listener invocations.
>> * <p>
>> * After this method has been called, the current {@code Change} instance is no longer valid and
>> * calling any method on it may result in undefined behavior. Callers must not make any assumptions
>> * about the identity of the {@code Change} instance returned by this method; even if the returned
>> * instance is the same as the current instance, it must be treated as a distinct change.
>> *
>> * @return the next change, or {@code null} if there are no more changes
>> */
>> public Change<E> next() { return null; }
>>
>>
>> This new method allows listener implementations to fetch all subsequent changes of a bulk operation, which can be implemented as follows:
>>
>>
>> set.addListener((SetChangeListener) change -> {
>> do {
>> // Inspect the change
>> if (change.wasAdded()) {
>> ...
>> } else if (change.wasRemoved() {
>> ...
>> }
>> } while ((change = change.next()) != null);
>> }
>>
>>
>> The implementation is fully backwards-compatible for listeners that are unaware of the new API. If the `next()` method is not called, then all subsequent changes are delivered as usual by repeated listener invocations.
>>
>> If a listener only fetches some changes of a bulk operation (but stops halfway through the op...
>
> Michael Strauß has updated the pull request incrementally with two additional commits since the last revision:
>
> - Use MapListenerHelper in PlatformPreferences to support bulk change notifications
> - Factor out IterableSetChange/IterableMapChange implementations
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java line 148:
> 146: change.nextAdded(key, newValue);
> 147: } else {
> 148: V oldValue = backingMap.get(key);
`containsKey()` followed by a `get()` - do you think it's possible to optimize this to avoid looking up twice? maybe use `get() == null` ?
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java line 795:
> 793: }
> 794:
> 795: private class SimpleChange extends MapChangeListener.Change<K,V> {
could this and `BulkChange` classes made static?
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java line 438:
> 436: callObservers(new SimpleAddChange(addedElement));
> 437: return true;
> 438: }
minor suggestion:
if (addedElement != null) {
..
} else if (addedList != null) {
..
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java line 517:
> 515: }
> 516:
> 517: if (removedList != null) {
same suggestion with `else`
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java line 532:
> 530: @Override
> 531: public void clear() {
> 532: if (backingSet.size() > 1) {
minor suggestion:
switch(backing.size()) {
case 0:
break;
case 1:
..
default:
..
}
though the compiler is probably going to optimize the second call to `size()` out.
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java line 166:
> 164: Thread.currentThread().getUncaughtExceptionHandler().uncaughtException(Thread.currentThread(), e);
> 165: }
> 166: } while (change instanceof IterableSetChange<? extends E> c && c.nextChange());
this is also minor: we are doing instanceof in a loop, perhaps we just need to add a second code path:
if(change instanceof IterableSetChange c) {
try {...}
} else {
try {..}
modules/javafx.base/src/main/java/javafx/collections/MapChangeListener.java line 107:
> 105: * Repeatedly calling this method allows a listener to fetch all subsequent changes of a bulk
> 106: * map modification that would otherwise be reported as repeated invocations of the listener.
> 107: * If the listener only fetches some of the pending changes, the rest of the changes will be
suggestion:
If the listener only fetches some of the pending changes **via the XX() method**
?
modules/javafx.base/src/main/java/javafx/collections/MapChangeListener.java line 110:
> 108: * reported with subsequent listener invocations.
> 109: * <p>
> 110: * After this method has been called, the current {@code Change} instance is no longer valid and
The language might be a bit confusing (it is to me, at least, where you say it might be the same instance but it is no longer valid? how is that even possible?).
Perhaps it could be rephrased to something like 'the Change instance returned by this method might be a different and must be used in subsequent operations' or something to that effect.
modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java line 49:
> 47: public void partialChangeIterationCausesSubsequentListenerInvocation() {
> 48: var trace = new ArrayList<String>();
> 49: var invocations = new int[1];
minor suggestion: an `AtomicInteger` is semantically better than an array
modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java line 78:
> 76: "d added at key k4",
> 77: "e added at key k5",
> 78: "f added at key k6"),
this is confusing (to me): I don't see the boundary created by the next() call (i.e. between the bulk and individual changes)
or am I missing something?
modules/javafx.base/src/test/java/test/javafx/collections/ObservableMapTest.java line 186:
> 184: @MethodSource("createParameters")
> 185: @SuppressWarnings("unchecked")
> 186: public void testReplaceAll(Callable<ObservableMap<String, String>> mapFactory) throws Exception {
should `ObservableSetTest` be modified as well?
modules/javafx.base/src/test/java/test/javafx/collections/ObservableMapTest.java line 189:
> 187: setUp(mapFactory);
> 188:
> 189: observableMap.replaceAll((key, value) -> switch (key) {
In addition to testing internal details in `ObservableXxxWrapperTests`, shouldn't we also test the bulk change functionality inside this test (i.e. testing the publicly accessible `Set`/`Map` variants)?
I mean, I would rather prioritize testing of the public APIs than some internal implementation.
modules/javafx.base/src/test/java/test/javafx/collections/ObservableMapTest.java line 195:
> 193: });
> 194:
> 195: observer.assertMultipleCalls(call("one", "1", "2"), call("two", "2", "3"));
Also, what I think might still be missing is the tests for all bulk methods (`clear`, `merge`, `putAll`, `replaceAll`) in 3 flavors:
1. the legacy (no `next()`, individual events)
2. partial, one `next()` call followed by individual events
3. real bulk processing (while last)
same for `Set`s.
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java line 78:
> 76: private final PreferenceProperties properties = new PreferenceProperties(this);
> 77:
> 78: private MapListenerHelper<String, Object> helper;
I wonder if this change should be undertaken as a separate PR, it's a totally unrelated functionality.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2456912999
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2456973646
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2456997708
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2456999627
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457007252
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457021029
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457052825
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457528078
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457592790
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457601876
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457633691
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457657384
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457688796
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457698289
More information about the openjfx-dev
mailing list