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 <[email protected]> > 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 >
