RFR: 8253086: Optimization of removeAll and retainAll of ObservableListWrapper [v4]

[Kevin Rushforth]

On Tue, 22 Sep 2020 07:35:49 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
>
> yosbits has refreshed the contents of this pull request, and previous commits have been removed. The incremental views
> will show differences compared to the previous content of the PR.

The fix looks good to me. I left a few comments on the test, but it looks like a great start.

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java line 203:

> 201:         boolean retained = false;
> 202:         beginChange();
> 203:         for (int i = size()-1; i>=0; i--) {

Please fix the spacing. Per our coding guidelines, this should be:

    for (int i = size() - 1; i >= 0; i--) {

modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 38:

> 36: import java.util.Arrays;
> 37: import java.util.Collection;
> 38: import java.util.Collections;

Can you sort the imports?

modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 110:

> 108:         assertTrue(list.retainAll(Collections.EMPTY_SET));
> 109:         assertEquals(0, list.size());
> 110:     }

Can you add a test for `retainAll` (in a separate test method) that removes multiple elements in a single call?
Something like the following:

* set list to `(1, 2, 3, 4, 5)`
* remove `(2, 4)`
* verify that the list equals `(1, 3, 5)`
* remove `(1, 5)`
* verify that the list equals `(3)`

Similarly, a test for `retainAll` that removes multiple elements in a single call?

* set list to `(1, 2, 3, 4, 5)`
* retain `(1, 3, 5)`
* verify that the list equals `(1, 3, 5)`
* retain `(3)`
* verify that the list equals `(3)`

modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java line 78:

> 76:         assertEquals(3, list.size());
> 77:         assertTrue(list.removeAll(1));
> 78:         assertEquals(2, list.size());

Can you add a check that the correct element was removed (i.e., check that the list equals `(2, 3)`)?

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

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