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