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

2020-10-10 Thread yosbits
On Fri, 9 Oct 2020 22:59:37 GMT, Kevin Rushforth wrote: >> @kevinrushforth >> >> To show an improvement in time efficiency, the >> We need to fix the other high-priority hotspots first. I have determined >> that the current proposed change in the >> Run-Length approach needs a lot of time to

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

2020-10-09 Thread Kevin Rushforth
On Fri, 9 Oct 2020 17:47:31 GMT, yosbits wrote: > * new BitSet(c.size()) > > Don't you notice this mistake? It certainly isn't an exact size, and probably not a very good guess in many cases (e.g., a small number of objects where one near the end is chosen), but it is at least a lower limit

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

2020-10-09 Thread yosbits
On Fri, 9 Oct 2020 16:51:05 GMT, yosbits wrote: >> I understand that this will improve the performance of this method in some >> cases (but not all as you correctly point >> out), but what I really meant by my questions was: When does this matter to >> an application's overall performance and

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

2020-10-09 Thread yosbits
On Fri, 9 Oct 2020 13:37:57 GMT, Kevin Rushforth wrote: >> The performance improvements of the first change were self-evident, but >> I agree that the current changes need to be explained. >> >> BitSet Features >> * When using BitSet, memory usage (N/8) is wasted in the case of removing >> the

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

2020-10-09 Thread Kevin Rushforth
On Fri, 9 Oct 2020 05:31:54 GMT, yosbits wrote: >> Before I review the actual proposed change, I have a pair of related >> high-level questions that I should have asked at >> the beginning of this review. >> 1. What is the expected performance gain, and under what conditions would >> you see

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

2020-10-08 Thread yosbits
On Fri, 9 Oct 2020 01:06:54 GMT, Kevin Rushforth wrote: >> It seems that many people are interested, so I pushed the change. >> I don't understand how to proceed with the review on Github correctly, so if >> I have anything to do, please comment. >> >> java >> for(int

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

2020-10-08 Thread Kevin Rushforth
On Wed, 7 Oct 2020 21:14:14 GMT, yosbits wrote: >> **The next implementation will probably have a good balance between space >> and time.** >> Unless you do something like delete the even or odd indexes >> The space efficiency is very high. >> >> Currently being tested. >> >> Java >>

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

2020-10-08 Thread yosbits
On Wed, 7 Oct 2020 18:34:23 GMT, yosbits wrote: >> I plan to push changes that remain compatible, respecting the judgment of >> the project leader, but I would like to point >> out the following: >> There seems to be a problem with the reproduction code as follows. >> >> * If there are

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

2020-10-07 Thread yosbits
On Wed, 7 Oct 2020 14:24:36 GMT, yosbits wrote: >> Hopefully not looking in the wrong version but: >> (1) When dealing with BitSets previously, maybe this was by design butI >> didn’t see any usage of BitSet’s >> “clear()” to remove items from the BitSet. Although given move to >> remove it

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

2020-10-07 Thread yosbits
On Wed, 7 Oct 2020 12:11:12 GMT, Eric Bresie wrote: >>> >>> >>> I'm preparing a change that won't break compatibility, so stay tuned. >>> The test seems to need to be added. >> >> sounds good :) Note, that I'm working on >> [JDK-8254040](https://bugs.openjdk.java.net/browse/JDK-8254040)

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

2020-10-07 Thread Eric Bresie
On Wed, 7 Oct 2020 12:06:09 GMT, Jeanette Winzenburg wrote: >> I'm preparing a change that won't break compatibility, so stay tuned. >> The test seems to need to be added. > >> >> >> I'm preparing a change that won't break compatibility, so stay tuned. >> The test seems to need to be added. >

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

2020-10-07 Thread Jeanette Winzenburg
On Wed, 7 Oct 2020 11:39:29 GMT, yosbits wrote: > > > I'm preparing a change that won't break compatibility, so stay tuned. > The test seems to need to be added. sounds good :) Note, that I'm working on [JDK-8254040](https://bugs.openjdk.java.net/browse/JDK-8254040) which will add

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

2020-10-07 Thread yosbits
On Wed, 7 Oct 2020 11:30:16 GMT, Jeanette Winzenburg wrote: >>> >>> >>> did anyone look into Java-Collection-Frameworks (ArrayList and friends or >>> Eclipse-Collections) how they handle this >>> situation effeciently? >> >> not me - but good idea, provided they support modifications to the

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

2020-10-07 Thread Jeanette Winzenburg
On Wed, 7 Oct 2020 10:39:02 GMT, Tom Schindl wrote: > > > did anyone look into Java-Collection-Frameworks (ArrayList and friends or > Eclipse-Collections) how they handle this > situation effeciently? not me - but good idea, provided they support modifications to the source list while

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

2020-10-07 Thread Tom Schindl
On Wed, 7 Oct 2020 09:38:40 GMT, Jeanette Winzenburg wrote: >> The error occurs as specified in getSelectedItems(). It seems to be correct >> to write the following >> >> `listView.getItems().removeAll(new HashSet<>(selectedItems)) >> ` >> >> (or ArrayList) >> >> It could be interpreted

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

2020-10-07 Thread Jeanette Winzenburg
On Wed, 7 Oct 2020 08:07:52 GMT, yosbits wrote: > > > The error occurs as specified in getSelectedItems(). no, both spec and implementation (at least as far as its relation to this issue) is correct. > It seems to be correct to write the following > > `listView.getItems().removeAll(new

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

2020-10-07 Thread yosbits
On Tue, 6 Oct 2020 16:36:44 GMT, Kevin Rushforth wrote: >> 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. > > As mentioned in a reply to a comment by

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

2020-10-06 Thread Nir Lisker
On Mon, 5 Oct 2020 19:22:41 GMT, Kevin Rushforth wrote: >>> The listView test is passing for the bitSet and for the back-to-front >>> approach. Can we imagine a context where the >>> broken selectedItems impl would add wreckage to the latter? >> >> To answer my own question: yes. A failing

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

2020-10-06 Thread Kevin Rushforth
On Tue, 22 Sep 2020 07:35:49 GMT, yosbits 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

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

2020-10-05 Thread Kevin Rushforth
On Sun, 4 Oct 2020 09:24:16 GMT, Jeanette Winzenburg wrote: >> the problem was (and still is) in MultipleSelectionModelBase: >> >> selectedItems = new >> SelectedItemsReadOnlyObservableList(selectedIndices, () -> >> getItemCount()) { >> @Override protected T

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

2020-10-04 Thread Jeanette Winzenburg
On Sat, 3 Oct 2020 11:13:28 GMT, Jeanette Winzenburg wrote: > The listView test is passing for the bitSet and for the back-to-front > approach. Can we imagine a context where the > broken selectedItems impl would add wreckage to the latter? To answer my own question: yes. A failing test with

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

2020-10-03 Thread yosbits
On Fri, 2 Oct 2020 16:29:56 GMT, Kevin Rushforth wrote: >> The fix looks good to me. I left a few comments on the test, but it looks >> like a great start. > > One meta-comment: > >> @yososs yososs force-pushed the >>

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

2020-10-03 Thread Kevin Rushforth
On Sat, 3 Oct 2020 10:09:40 GMT, Jeanette Winzenburg wrote: >> 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());

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

2020-10-03 Thread Jeanette Winzenburg
On Fri, 2 Oct 2020 16:23:23 GMT, Kevin Rushforth wrote: >> 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. > >

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

2020-10-03 Thread Jeanette Winzenburg
On Fri, 2 Oct 2020 18:20:18 GMT, Kevin Rushforth wrote: >>> the reason BitSet was introduced was to ensure that the elements are >>> removed from this List in reverse order (prior to >>> that fix, they were removed in forward order with the loop index being >>> messed up). >> >> But why do

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

2020-10-02 Thread Kevin Rushforth
On Fri, 2 Oct 2020 17:16:03 GMT, Nir Lisker wrote: >> I looked at the fix for >> [JDK-8093144](https://bugs.openjdk.java.net/browse/JDK-8093144), and the >> reason BitSet was >> introduced was to ensure that the elements are removed from this List in >> reverse order (prior to that fix, they

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

2020-10-02 Thread Nir Lisker
On Fri, 2 Oct 2020 16:01:20 GMT, Kevin Rushforth wrote: > the reason BitSet was introduced was to ensure that the elements are removed > from this List in reverse order (prior to > that fix, they were removed in forward order with the loop index being messed > up). But why do they need to be

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

2020-10-02 Thread Kevin Rushforth
On Fri, 2 Oct 2020 16:27:09 GMT, Kevin Rushforth wrote: >> 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

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

2020-10-02 Thread Kevin Rushforth
On Tue, 22 Sep 2020 07:35:49 GMT, yosbits 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

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

2020-10-02 Thread Kevin Rushforth
On Tue, 22 Sep 2020 01:58:48 GMT, yosbits wrote: >> digging a bit: the BitSet was introduced with >> https://bugs.openjdk.java.net/browse/JDK-8093144 to solve some problem >> with selectionModels - don't know whether those still hold (there had been >> extensive changes to selection since

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

2020-09-22 Thread yosbits
> 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