RFR: 8367439: Bulk change notifications for ObservableSet and ObservableMap [v3]

[Michael Strauß]

On Thu, 23 Oct 2025 19:25:58 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
>> 
>>  - Merge branch 'master' into feature/bulk-listeners
>>  - remove unused variable
>>  - Don't repeatedly call backingSet.size()
>>  - Separate code paths for Change/IterableChange
>>  - Use MapListenerHelper in PlatformPreferences to support bulk change notifications
>>  - Factor out IterableSetChange/IterableMapChange implementations
>>  - add tests, documentation
>>  - Implementation of bulk change listeners for ObservableSet and ObservableMap
>
> 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` ?

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`.

> 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?

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.

> 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.

I've factored out the repeated call to `backingSet.size()`, and reordered the branches such that == 1 comes before > 1. This reads more cleanly now, and I think is also a bit better than a switch block.

> 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 {..}

Yes, changed this here and also in `MapListenerHelper`.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457888231
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457888283
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457895737
PR Review Comment: https://git.openjdk.org/jfx/pull/1885#discussion_r2457897995