RFR: 8283063: Optimize Observable{List/Set/Map}Wrapper.retainAll/removeAll [v3]
John Hendrikx
jhendrikx at openjdk.org
Sat Apr 1 08:24:39 UTC 2023
On Sat, 1 Apr 2023 03:32:43 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 with a new target base due to a merge or a rebase. The pull request now contains five commits:
>
> - Merge branch 'master' into fixes/JDK-8283063
>
> # Conflicts:
> # modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java
> # modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java
> # modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSequentialListWrapper.java
> # modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java
> - address review comments
> - refactored removeAll/retainAll optimizations
> - Optimize removeAll/retainAll for Observable{List/Set/Map}Wrapper
> - Failing test
Super happy you also added IOOBE -- collection classes are such a corner stone of Java and the observable variants fulfill that role for JavaFX -- these classes should be really conservative with the inputs they accept as any faulty inputs are likely to be bugs.
Programmers have certain expectations when they see `Set`, `List` or `Map`, and expect their inputs to be validated; when those validations are suddenly not working because the implementation happens to be of the `Observable` kind, bugs will go unnoticed.
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java line 326:
> 324: if (backingMap.isEmpty()) {
> 325: return false;
> 326: }
Passing `null` is always an error, and I think here you still need to throw an NPE first when `c` is `null`, even if the backing map is empty. From `AbstractColection` for example:
public boolean retainAll(Collection<?> c) {
Objects.requireNonNull(c);
...
I realize this was already incorrect, so perhaps out of scope for your PR.
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java line 353:
> 351: @Override
> 352: public boolean removeAll(Collection<?> c) {
> 353: if (backingMap.isEmpty() || c.isEmpty()) {
This one should also be swapped if you're going for implicit checks. Also I think it really couldn't hurt to add a small comment there (`// implicit null check` for future maintainers -- that's how I see it often done in JDK collection code).
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java line 466:
> 464: @Override
> 465: public boolean removeAll(Collection<?> c) {
> 466: if (backingMap.isEmpty() || c.isEmpty()) {
This one should also be swapped if you're going for implicit checks.
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableMapWrapper.java line 490:
> 488: @Override
> 489: public boolean retainAll(Collection<?> c) {
> 490: if (backingMap.isEmpty()) {
Here the null check is missing, it should be before you check the backingMap as passing `null` is a bug in the caller code.
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSequentialListWrapper.java line 185:
> 183: return false;
> 184: }
> 185:
Shouldn't this always check the index?
Suggestion:
if (index < 0 || index > size()) {
throw new IndexOutOfBoundsException("Index: " + index);
}
if (c.isEmpty()) { // implicit null check
return false;
}
modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSetWrapper.java line 337:
> 335: clear();
> 336: return true;
> 337: } else if (backingSet.isEmpty()) {
minor: style thing that I personally dislike, using an `else` when other branches `throw` or `return` (IDE can flag those)
modules/javafx.base/src/main/java/javafx/collections/ModifiableObservableListBase.java line 125:
> 123: return false;
> 124: }
> 125:
Shouldn't this always check the index?
Suggestion:
if (index < 0 || index > size()) {
throw new IndexOutOfBoundsException("Index: " + index);
}
if (c.isEmpty()) { // implicit null check
return false;
}
modules/javafx.base/src/main/java/javafx/collections/ModifiableObservableListBase.java line 142:
> 140:
> 141: return;
> 142: }
I'm really happy you also added the index range checks, not only does it conform to the contract better, it will help expose bugs in caller code (or even the JavaFX code as Observable* classes are used in many places, and who knows what bugs might lurk in some of the more complicated classes).
I'm not quite sure how this check is suppose to work though; does this need a comment ?
Suggestion:
if (fromIndex == toIndex) { // distinguish between the clear and remove(int) call
if (fromIndex < 0 || fromIndex > size()) {
throw new IndexOutOfBoundsException("Index: " + fromIndex);
}
return;
}
I think that would be a bit too cryptic for me...
modules/javafx.base/src/main/java/javafx/collections/ModifiableObservableListBase.java line 360:
> 358: return false;
> 359: }
> 360:
Shouldn't this always check the index, not only when the collection passed in is empty?
Otherwise code like: `singleElementSubList.addAll(15, List.of("a"))` would be allowed?
Suggestion:
if (index < 0 || index > sublist.size()) {
throw new IndexOutOfBoundsException("Index: " + index);
}
if (c.isEmpty()) { // implicit null check
return false;
}
modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java line 82:
> 80: public void testRemoveAllWithNullArgumentThrowsNPE() {
> 81: var map = new ObservableMapWrapper<>(new HashMap<>(Map.of("k0", "v0", "k1", "v1", "k2", "v2")));
> 82: assertThrows(NullPointerException.class, () -> map.entrySet().removeAll((Collection<?>) null));
This test fails I think when the map used is empty (no NPE), see other comments.
modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java line 97:
> 95: var content = Map.of("k0", "v0", "k1", "v1", "k2", "v2");
> 96: var map = new ObservableMapWrapper<>(new HashMap<>(content));
> 97: assertThrows(NullPointerException.class, () -> map.entrySet().retainAll((Collection<?>) null));
If I'm correct, with an empty map, this would not throw the NPE, see other comments.
modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableSequentialListWrapperTest.java line 60:
> 58:
> 59: @Test
> 60: public void testAddAllWithEmptyCollectionArgumentAndInvalidIndexThrowsIOOBE() {
How about a variant that doesn't pass an empty collection, `-1` should still fail, and for example `100` should also fail.
modules/javafx.base/src/test/java/test/javafx/collections/ModifiableObservableListBaseTest.java line 61:
> 59:
> 60: @Test
> 61: public void testAddAllWithEmptyCollectionArgumentAndInvalidIndexThrowsIOOBE() {
Could use variant here I think with a non empty collection, and see if it still throws IOOBE.
modules/javafx.base/src/test/java/test/javafx/collections/ModifiableObservableListBaseTest.java line 157:
> 155:
> 156: @Test
> 157: public void testAddAllWithEmptyCollectionArgumentAndInvalidIndexThrowsIOOBE() {
Could use variant here with non-empty collection, and see if it still throws IOOBE.
-------------
Changes requested by jhendrikx (Committer).
PR Review: https://git.openjdk.org/jfx/pull/751#pullrequestreview-1367824444
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155072066
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155072511
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155072566
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155072688
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155076010
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155073032
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155076300
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155074911
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155075825
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155073919
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155073715
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155076496
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155076585
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155076617
More information about the openjfx-dev
mailing list