RFR: 8253086: Optimization of removeAll and retainAll of ObservableListWrapper

[Jeanette Winzenburg]

On Sat, 19 Sep 2020 05:18:11 GMT, yosbits <github.com+7517141+yososs at openjdk.org> wrote:

>> 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.
>
> Since the purpose of this issue is performance tuning,
> Rewriting with removeIf has a different purpose.
> 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.

wondering why the implementation with the BitSet was choosen at all? Certainly feels like a detour, so what are the
corner cases that I'm too blind to see?

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

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