RFR: 8253086: Optimization of removeAll and retainAll of ObservableListWrapper

[yosbits]

On Sat, 19 Sep 2020 01:09:10 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8253086
>> 
>> ObservableListWrapper.java
>>  * public boolean removeAll(Collection<?> c)
>> * public boolean retainAll(Collection<?> c)
>> 
>> These two methods use BitSet, but it doesn't make sense.
>> By rewriting to the equivalent behavior that does not use BitSet, it is possible to reduce the CPU load in a wide range
>> of use cases.
>> The test is done with the following command.
>> 
>> * gradle base: test
>> * gradle controls: test
>
> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java line 177:
> 
>> 175:         beginChange();
>> 176:         boolean removed = false;
>> 177:         for (int i = size()-1; i>=0; i--) {
> 
> 1. Use spaces between operators.
> 2. Why reverse the iteration order?
> 
> Implementation discussion:
> * We can use `boolean removed = removeIf(c::contains);` instead of the loop. However, this method uses the iterator and
>   not random access, which *could* be less performant. The class implements `RandomAccess`, suggesting that this is the
>   case, however, I don't see why. It seems that the implementation can be any list, such as `LinkedList`, which is not
>   `RandomAccess`.
> * Why do we need to override the inherited behavior? The super implementation, which is the one from
>   `ModifiableObservableListBase`, does
>   ```
>         beginChange();
>         try {
>             boolean res = super.removeAll(c);
>             return res;
>         } finally {
>             endChange();
>         }
>   ```
>   (except that it does not check for empty collections at the start - should it?) The `super` here is
>   `AbstractCollection`, which does `Iterator`-based removal.
> 
> Same goes for the other method.

Rewriting backingList.removeIf
It is not used here because it requires doRemove processing.
This is the same as the original code.

The iteration order is reversed because index movement does not occur during deletion. This is the same as the original
code.

Reviewers don't seem to be familiar with this code.

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

PR: https://git.openjdk.java.net/jfx/pull/305