On Sat, 1 Apr 2023 03:32:43 GMT, Michael Strauß <[email protected]> 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
