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

[Michael Strauß]

On Thu, 30 Mar 2023 13:03:22 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/com/sun/javafx/collections/ObservableListWrapper.java line 172:
> 
>> 170:     @Override
>> 171:     public boolean removeAll(Collection<?> c) {
>> 172:         if (backingList.isEmpty() || c.isEmpty()) {
> 
> I think you should do an explicit `null` check here on `c` or swap the order of these arguments so it always throws an NPE here if `c` is `null` as per collection contract.  If you don't, it will do this implicit `null` check just after `beginChange`, and as I don't see a `try/finally` there to call `endChange`, it would mean the wrapper / changeListBuilder gets in a bad state.

I've opted for implicit checks by always dereferencing `c`.

> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java line 195:
> 
>> 193:     @Override
>> 194:     public boolean retainAll(Collection<?> c) {
>> 195:         if (backingList.isEmpty()) {
> 
> I think we need to check `c` for `null` here first to conform to the collection contract.

Done.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155050099
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155050071