8367439: Bulk change notifications for ObservableSet and ObservableMap #1885

mstr2 · 2025-09-07T00:12:31Z

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 operation), the remaining changes will also be delivered with repeated listener invocations.

Implementation

Bulk change retrieval is only possible if the collection implementation explicitly supports the new API. We do this for ObservableSetWrapper and ObservableMapWrapper, which accounts for most instances where observable collections are used.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
Change requires a CSR request matching fixVersion jfx26 to be approved (needs to be created)

Issue

JDK-8367439: Bulk change notifications for ObservableSet and ObservableMap (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1885/head:pull/1885
$ git checkout pull/1885

Update a local copy of the PR:
$ git checkout pull/1885
$ git pull https://git.openjdk.org/jfx.git pull/1885/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1885

View PR using the GUI difftool:
$ git pr show -t 1885

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1885.diff

Using Webrev

Link to Webrev Comment

…bleMap

bridgekeeper · 2025-09-07T00:13:19Z

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-09-07T00:13:46Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

andy-goryachev-oracle · 2025-09-08T15:24:25Z

modules/javafx.base/src/test/java/test/javafx/collections/ObservableMapTest.java

+        });
+
+        observer.assertMultipleCalls(call("one", "1", "2"), call("two", "2", "3"));
+    }


From the description:

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 operation), the remaining changes will also be delivered with repeated listener invocations.

should this be included in javadoc?

could we add a test for this scenario?

mstr2 · 2025-09-24T17:59:00Z

/reviewers 2 reviewer
/csr

openjdk · 2025-09-24T18:00:17Z

@mstr2
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

openjdk · 2025-09-24T18:00:46Z

@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8367439 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2025-09-24T18:04:31Z

Webrevs

bridgekeeper · 2025-10-22T20:03:57Z

@mstr2 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2025-10-22T23:11:11Z

I'll try to finish the review this week.
/keepalive

openjdk · 2025-10-22T23:11:42Z

@andy-goryachev-oracle The pull request is being re-evaluated and the inactivity timeout has been reset.

andy-goryachev-oracle · 2025-10-23T16:14:12Z

Before going into a full review, I'd like t ask this:

please enumerate all the bulk methods in Map and Set that support the new behavior in the description and possibly in the javadoc
do we have tests that cover all the bulk methods, exercising the following three scenarios:

next() is not called, received all changes individually (probably so, as it is the current behavior)
partial retrieval scenario where the remaining changes are received via individual events as described in javadoc and the description
all changes received via the new methods

mstr2 · 2025-10-23T17:00:12Z

Before going into a full review, I'd like t ask this:

please enumerate all the bulk methods in Map and Set that support the new behavior in the description and possibly in the javadoc

ObservableSetWrapper and ObservableMapWarpper support bulk change notifications for all bulk operations. Since these classes aren't public API, the only place where we could document this for users is in FXCollections.observableSet() and FXCollections.observableMap(). I wonder if we want to document it explicitly, since it is supported in the entire framework by default. The only specification surface is then the new method on SetChangeListener.Change and MapChangeListener.Change.

do we have tests that cover all the bulk methods, exercising the following three scenarios:

next() is not called, received all changes individually (probably so, as it is the current behavior)

The bulk operation tests are excercised both for bulk retrieval and individual retrieval. See ObservableSetWrapperTest.TestObservableSetWrapper.

partial retrieval scenario where the remaining changes are received via individual events as described in javadoc and the description

ObservableSetWrapper.partialChangeIterationCausesSubsequentListenerInvocation

all changes received via the new methods

Yes, see above.

…otifications

andy-goryachev-oracle · 2025-10-23T19:25:58Z

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java

+                if (!backingMap.containsKey(key)) {
+                    change.nextAdded(key, newValue);
+                } else {
+                    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 ?

This is possible if we know that the map doesn't contain null mappings. But since ObservableMapWrapper is a wrapper around an arbitray Map, we need to account for null mappings. As a consequence of that, we don't know if map.get(key) == null means that no mapping is present, or if a mapping is present but its value is null.

andy-goryachev-oracle · 2025-10-23T19:38:15Z

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java

+        return builder.append(" at key ").append(change.getKey()).toString();
+    }
+
+    private class SimpleChange extends MapChangeListener.Change<K,V> {


could this and BulkChange classes made static?

Yes, I can do that for the existing Change implementations, but it's not directly related to bulk change notifications. Maybe another PR would be better.

since you are already here... saves a pointer maybe

"since you are already here" is not usually sufficient justification for fixing an unrelated bug. So it probably makes sense to do for any new class added, but I'd leave the existing ones alone.

I thought it is a new class. But sure, I am ok to keep the code as is.

andy-goryachev-oracle · 2025-10-23T19:44:32Z

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java

+        if (addedElement != null) {
+            callObservers(new SimpleAddChange(addedElement));
+            return true;
+        }


minor suggestion:

if (addedElement != null) { .. } else if (addedList != null) { ..

andy-goryachev-oracle · 2025-10-23T19:44:56Z

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java

+            return true;
+        }
+
+        if (removedList != null) {


same suggestion with else

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java

andy-goryachev-oracle · 2025-10-23T22:16:09Z

modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java

+                "c added at key k3",
+                "d added at key k4",
+                "e added at key k5",
+                "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?

This test demonstrates that the sequence of changes is the same, no matter the number of change listener invocations. For this purpose, we assert that the number of changes is as expected (6), but the number of invocations is only 3. In this sense, not seeing a boundary between the invocations is the point.

Ok, so if I were to write a test per the public API, two things should be tested: the changes and the bulk aspect. So perhaps the toString() or the test's toString() can be modified to explicitly encode the bulk aspect. In other words, the test might look like this:

expect([k3=c, k4=d],[k5=e],[k6=f])

if the test calls next on k3 and k4, then individual events come for k5, k6.
This way you test both API functions in one test.

andy-goryachev-oracle · 2025-10-23T22:23:56Z

modules/javafx.base/src/test/java/test/javafx/collections/ObservableMapTest.java

+    @ParameterizedTest
+    @MethodSource("createParameters")
+    @SuppressWarnings("unchecked")
+    public void testReplaceAll(Callable<ObservableMap<String, String>> mapFactory) throws Exception {


should ObservableSetTest be modified as well?

andy-goryachev-oracle · 2025-10-23T22:29:19Z

modules/javafx.base/src/test/java/test/javafx/collections/ObservableMapTest.java

+    public void testReplaceAll(Callable<ObservableMap<String, String>> mapFactory) throws Exception {
+        setUp(mapFactory);
+
+        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.

andy-goryachev-oracle · 2025-10-23T22:34:38Z

modules/javafx.base/src/test/java/test/javafx/collections/ObservableMapTest.java

+            default -> value;
+        });
+
+        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:

the legacy (no next(), individual events)

partial, one next() call followed by individual events

real bulk processing (while last)

same for Sets.

andy-goryachev-oracle · 2025-10-23T22:37:00Z

...avafx.graphics/src/main/java/com/sun/javafx/application/preferences/PlatformPreferences.java


-    private final List<InvalidationListener> invalidationListeners = new CopyOnWriteArrayList<>();
-    private final List<MapChangeListener<? super String, Object>> mapChangeListeners = new CopyOnWriteArrayList<>();
+    private MapListenerHelper<String, Object> helper;


I wonder if this change should be undertaken as a separate PR, it's a totally unrelated functionality.

andy-goryachev-oracle · 2025-10-23T22:47:27Z

oh, could you please merge the latest master branch?

mstr2 · 2025-10-24T00:30:10Z

There's one behavioral change with this enhancement that I want to point out.

Previously, bulk operations were just repeated invocations of their single-element counterpart, i.e. ObservableSetWrapper.addAll(Collection<E>) would just repeatedly call ObservableSetWrapper.add(E) and thereby trigger repeated listener invocations.

Now consider this method:

public interface SetChangeListener<E> {

    abstract class Change<E> {
        /**
         * An observable set that is associated with the change.
         * @return the source set
         */
        public ObservableSet<E> getSet() { ... }
    }
}

Previously, if a listener were to query the source set, it would always correspond to the current state of the set.
With the bulk change enhancement, the source set corresponds to the final state of the set (because all changes have already been applied in bulk).

andy-goryachev-oracle · 2025-10-24T18:13:59Z

There's one behavioral change with this enhancement that I want to point out.
we probably need to add this explanation to SetChangeListener.Change.getSet() javadoc. I assume the same goes for MapChangeListener.Change.getMap() ?

andy-goryachev-oracle · 2025-10-24T18:27:27Z

What about the ordering of bulk changes? Should it be addressed in javadoc for bulk changes (in next())?

The ordering might impact tests.

andy-goryachev-oracle · 2025-10-24T18:41:48Z

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java

+            E[] removed = (E[])new Object[size];
+            backingSet.toArray(removed);
+            backingSet.clear();
+            callObservers(new IterableSetChange.Remove<>(this, Arrays.asList(removed)));


suggestion:

ArrayList<E> removed = new ArrayList<>(backingSet); backingSet.clear(); callObservers(new IterableSetChange.Remove<>(this, removed));

Implementation of bulk change listeners for ObservableSet and Observa…

3779152

…bleMap

andy-goryachev-oracle reviewed Sep 8, 2025

View reviewed changes

add tests, documentation

747609b

mstr2 changed the title ~~Bulk change notifications for ObservableSet and ObservableMap~~ 8367439: Bulk change notifications for ObservableSet and ObservableMap Sep 11, 2025

mstr2 marked this pull request as ready for review September 24, 2025 17:58

openjdk bot added the rfr Ready for review label Sep 24, 2025

openjdk bot added the csr Need approved CSR to integrate pull request label Sep 24, 2025

kevinrushforth self-requested a review October 10, 2025 14:51

mstr2 added 2 commits October 24, 2025 00:06

Factor out IterableSetChange/IterableMapChange implementations

f44413e

Use MapListenerHelper in PlatformPreferences to support bulk change n…

6801fdc

…otifications

andy-goryachev-oracle reviewed Oct 23, 2025

View reviewed changes

mstr2 added 4 commits October 24, 2025 01:21

Separate code paths for Change/IterableChange

27d3a35

Don't repeatedly call backingSet.size()

b03b567

remove unused variable

21f813e

Merge branch 'master' into feature/bulk-listeners

6101be4

andy-goryachev-oracle reviewed Oct 24, 2025

View reviewed changes

Uh oh!

8367439: Bulk change notifications for ObservableSet and ObservableMap #1885

Are you sure you want to change the base?

8367439: Bulk change notifications for ObservableSet and ObservableMap #1885

Conversation

mstr2 commented Sep 7, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Sep 7, 2025

Uh oh!

openjdk bot commented Sep 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstr2 commented Sep 24, 2025

Uh oh!

openjdk bot commented Sep 24, 2025

Uh oh!

openjdk bot commented Sep 24, 2025

Uh oh!

mlbridge bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

bridgekeeper bot commented Oct 22, 2025

Uh oh!

andy-goryachev-oracle commented Oct 22, 2025

Uh oh!

openjdk bot commented Oct 22, 2025

Uh oh!

andy-goryachev-oracle commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstr2 commented Oct 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Oct 23, 2025

Uh oh!

mstr2 commented Oct 24, 2025

Uh oh!

andy-goryachev-oracle commented Oct 24, 2025

Uh oh!

andy-goryachev-oracle commented Oct 24, 2025

Uh oh!

mstr2 commented Sep 7, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 24, 2025 •

edited

Loading

andy-goryachev-oracle commented Oct 23, 2025 •

edited

Loading