RFR: 8283063: Optimize Observable{List/Set/Map}Wrapper.retainAll/removeAll [v4]

[Nir Lisker]

On Sat, 1 Apr 2023 18:14:11 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> `Observable{List/Set/Map}Wrapper.retainAll/removeAll` can be optimized for some edge cases.
>> 
>> 1. `removeAll(c)`:
>> This is a no-op if 'c' is empty.
>> For `ObservableListWrapper`, returning early skips an object allocation. For `ObservableSetWrapper` and `ObservableMapWrapper`, returning early prevents an enumeration of the entire collection.
>> 
>> 2. `retainAll(c)`:
>> This is a no-op if the backing collection is empty, or equivalent to `clear()` if `c` is empty.
>> 
>> I've added some tests to verify the optimized behavior for each of the three classes.
>
> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
> 
>   addressed review comments, added tests

Looks good. Left a few minor comments.

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSequentialListWrapper.java line 178:

> 176:     @Override
> 177:     public boolean addAll(int index, Collection<? extends E> c) {
> 178:         if (index < 0 || index > size()) {

If you want, there are methods in `Objects` that do range checks, like `Objects.checkIndex`.

Same for the other files.

modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 41:

> 39: 
> 40:     @Nested
> 41:     class RemoveAllTest {

Empty line after class declaration.

Same for other places.

modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 57:

> 55:             });
> 56: 
> 57:             list.removeAll(Collections.<String>emptyList());

You can use `List.of()` instead of `Collections.emptyList()` if you want here and in other places.

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

> 63: 
> 64:         @Test
> 65:         public void testValueSetNullArgumentThrowsNPE() {

The values collection is not a `Set`, but the intention is clear. Same in other places.

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

> 106: 
> 107:             var map2 = new ObservableMapWrapper<>(new HashMap<>(Map.of("k0", "v0", "k1", "v1", "k2", "v2")));
> 108:             assertThrows(NullPointerException.class, () -> map2.entrySet().retainAll((Collection<?>) null));

Would advise to rename `map1` to `emptyMap` and `map2` to `notEmptyMap` or the like.

Same in other places.

modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableSequentialListWrapperTest.java line 55:

> 53:         };
> 54: 
> 55:         assertDoesNotThrow(() -> list.addAll(Collections.emptyList()));

The `addAll` method here does not belong to `ObservableSequentialListWrapper`, but to `ModifiableObservableListBase`, which is tested in another file. Not sure if this method test helps.

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

PR Review: https://git.openjdk.org/jfx/pull/751#pullrequestreview-1367987645
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155208556
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155217217
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155217387
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155219827
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155220370
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155219034