RFR: 8253086: Optimization of removeAll and retainAll of ObservableListWrapper

[Nir Lisker]

On Mon, 14 Sep 2020 09:57:26 GMT, yosbits <github.com+7517141+yososs 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

Remove the unused `BitSet` import.

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.

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

Changes requested by nlisker (Reviewer).

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