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

[Michael Strauß]

On Sat, 1 Apr 2023 08:00:33 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

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

The index check is only really necessary for the optimized code path, since calling `listIterator` later in the method would catch an invalid index. However, I've opted to always check the index at the beginning of the method.

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

Done.

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

Done.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155152703
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155153010
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155152937