Re: Alternatives for JDK-8185886: Improve scrolling performance of TableView and TreeTableView
That's a fair question. Hard to say, but my guess is that if we do the first, the incremental improvements of the second might not be worth the effort or risk. And if we go with the second, we would need another test case that hits the slow iteration problem when removing a listener from a large array. -- Kevin On 4/3/2020 12:15 PM, Scott Palmer wrote: Assuming testing and performance/memory analysis leads to the conclusion that the risks are worth it, would it make sense to do both? Would we get a greater benefit from the combined effects? Or is the incremental improvement of including the second fix (whichever it may be) no longer significant enough to bother with? Scott On Apr 3, 2020, at 2:15 PM, Kevin Rushforth wrote: We now have two pull requests under review that propose to solve the poor scrolling performance of TableView and TreeTableView, as tracked by JDK-8185886 [1] The first one, PR #108 [2], proposes a change in the bindings ExpressionHelper code relating to the cleaning up of listeners (changing the array-based implementation to a Map). It is a change in javafx.base to make the existing operations that TableView / TreeTableView do less expensive. The second one, PR #125 [3], proposes to address the problem by eliminating the need for cleaning up a large numbers of bindings. This approach changes the javafx.controls code used by TableView, and doesn't touch the binding code. It would be helpful to discuss which approach to take on this list, so we aren't independently reviewing both PRs. I don't yet have an opinion on which way to go, but I will note a couple pros / cons of each approach. PR #108 is both a more fundamental change and a simpler change. It changes the characteristics (memory footprint, performance) of a class that is used by far more that just TableView and TreeTableView. This is both a potential benefit and risk. If done in such a way that there are no regressions (functional, memory, or performance), it could benefit more than just the scrolling issue in question. By contrast, it has the potential to impact other use cases negatively, mainly from a performance or memory point of view, since the logic changes are relatively simple, and should be largely "behavior neutral". PR #125 is a more targeted change, impacting only the two controls in question, but is a more complicated change from a logic point of view. I am concerned primarily with any unintended behavioral changes. Both of them will need to be very well tested to ensure that there are no regressions. -- Kevin [1] https://bugs.openjdk.java.net/browse/JDK-8185886 [2] https://github.com/openjdk/jfx/pull/108 [3] https://github.com/openjdk/jfx/pull/125
Re: Alternatives for JDK-8185886: Improve scrolling performance of TableView and TreeTableView
Assuming testing and performance/memory analysis leads to the conclusion that the risks are worth it, would it make sense to do both? Would we get a greater benefit from the combined effects? Or is the incremental improvement of including the second fix (whichever it may be) no longer significant enough to bother with? Scott > On Apr 3, 2020, at 2:15 PM, Kevin Rushforth > wrote: > > We now have two pull requests under review that propose to solve the poor > scrolling performance of TableView and TreeTableView, as tracked by > JDK-8185886 [1] > > The first one, PR #108 [2], proposes a change in the bindings > ExpressionHelper code relating to the cleaning up of listeners (changing the > array-based implementation to a Map). It is a change in javafx.base to make > the existing operations that TableView / TreeTableView do less expensive. > > The second one, PR #125 [3], proposes to address the problem by eliminating > the need for cleaning up a large numbers of bindings. This approach changes > the javafx.controls code used by TableView, and doesn't touch the binding > code. > > It would be helpful to discuss which approach to take on this list, so we > aren't independently reviewing both PRs. > > I don't yet have an opinion on which way to go, but I will note a couple pros > / cons of each approach. > > PR #108 is both a more fundamental change and a simpler change. It changes > the characteristics (memory footprint, performance) of a class that is used > by far more that just TableView and TreeTableView. This is both a potential > benefit and risk. If done in such a way that there are no regressions > (functional, memory, or performance), it could benefit more than just the > scrolling issue in question. By contrast, it has the potential to impact > other use cases negatively, mainly from a performance or memory point of > view, since the logic changes are relatively simple, and should be largely > "behavior neutral". > > PR #125 is a more targeted change, impacting only the two controls in > question, but is a more complicated change from a logic point of view. I am > concerned primarily with any unintended behavioral changes. > > Both of them will need to be very well tested to ensure that there are no > regressions. > > -- Kevin > > [1] https://bugs.openjdk.java.net/browse/JDK-8185886 > [2] https://github.com/openjdk/jfx/pull/108 > [3] https://github.com/openjdk/jfx/pull/125 >
Alternatives for JDK-8185886: Improve scrolling performance of TableView and TreeTableView
We now have two pull requests under review that propose to solve the poor scrolling performance of TableView and TreeTableView, as tracked by JDK-8185886 [1] The first one, PR #108 [2], proposes a change in the bindings ExpressionHelper code relating to the cleaning up of listeners (changing the array-based implementation to a Map). It is a change in javafx.base to make the existing operations that TableView / TreeTableView do less expensive. The second one, PR #125 [3], proposes to address the problem by eliminating the need for cleaning up a large numbers of bindings. This approach changes the javafx.controls code used by TableView, and doesn't touch the binding code. It would be helpful to discuss which approach to take on this list, so we aren't independently reviewing both PRs. I don't yet have an opinion on which way to go, but I will note a couple pros / cons of each approach. PR #108 is both a more fundamental change and a simpler change. It changes the characteristics (memory footprint, performance) of a class that is used by far more that just TableView and TreeTableView. This is both a potential benefit and risk. If done in such a way that there are no regressions (functional, memory, or performance), it could benefit more than just the scrolling issue in question. By contrast, it has the potential to impact other use cases negatively, mainly from a performance or memory point of view, since the logic changes are relatively simple, and should be largely "behavior neutral". PR #125 is a more targeted change, impacting only the two controls in question, but is a more complicated change from a logic point of view. I am concerned primarily with any unintended behavioral changes. Both of them will need to be very well tested to ensure that there are no regressions. -- Kevin [1] https://bugs.openjdk.java.net/browse/JDK-8185886 [2] https://github.com/openjdk/jfx/pull/108 [3] https://github.com/openjdk/jfx/pull/125
