Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v4]

[Kevin Rushforth]

On Thu, 3 Sep 2020 07:44:55 GMT, yosbits 
 wrote:

>>> 
>>> 
>>> When the startup time is measured by eye, the impression changes depending 
>>> on the individual difference.
>> 
>> my eye is a precision instrument :) Seriously, who would do such a thingy? 
>> Obviously, it must be measured, for zeroth
>> approximation doing so crudely by comparing currentMillis before/after 
>> showing is good enough to "see" the tendency.
>>> The effect of runLater affects your experience.
>> 
>> that's why you shouldn't do it when trying to gain insight into timing 
>> issues ;)
>> 
>>> 
>>> However, I succeeded in further improving performance by eliminating 
>>> another bottleneck in TreeTableView. Of course, it
>>> also includes improvements in startup time.
>> 
>> cool :)
>
> Column virtualization causes shortness of breath when scrolling a large 
> stroke horizontally.
> This does not happen when stroking on the scrollbar. Looks like a potential 
> problem with VirtualFlow.
> 
> If you are worried about shortness of breath, the following code will help 
> reduce the problem.
> 
> 
>  Java
> 　private static final int OVERLAP_MARGIN = 500;
> 
> private static boolean isOverlap(double start, double end, double start2, 
> double end2){
>   start = Math.max(start-OVERLAP_MARGIN, start2);
>   end = Math.min(end+OVERLAP_MARGIN, end2);
> return (start<=end2 && end >= start2);
> }

@yososs Per [this 
message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-September/027534.html)
 on the
openjfx-dev mailing list, I have closed 
[JDK-8185886](https://bugs.openjdk.java.net/browse/JDK-8185886) as a duplicate,
and suggested another existing JBS issue for this PR to use. Please change the 
title to:

8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal 
direction

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Kevin Rushforth]

On Thu, 27 Aug 2020 00:07:21 GMT, Kevin Rushforth  wrote:

>> So far, there are two alternative fixes for the bad performance issue while 
>> scrolling TableView/TreeTableViews:
>> 
>> - this one #108, that tries to improve the performance of excessive number 
>> of `removeListener` calls
>> - the WIP #185 that avoids registering two listeners (on Scene and on 
>> Window) for each and every Node.
>> 
>> For the case presented, where new items are constantly added to the 
>> TableView, the trace of calls that reach
>> `com.sun.javafx.binding.ExpressionHelper.removeListener()` is something like 
>> this:
>> ![TraceOpenJFX16ea1](https://user-images.githubusercontent.com/2043230/91322208-c2ba9900-e7bf-11ea-8918-cdbecd457cf2.png)
>>  
>> As can be seen, all those calls are triggered by the change of the number of 
>> cells in
>> [VirtualFlow::setCellCount](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L804).
>> Whenever the cell count changes there is this
>> [call](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L843):
>> sheetChildren.clear();
>> 
>> This happens every time the number of items in the `TableView` changes, as 
>> the `VirtualFlow` keeps track of the number
>> of virtual cells (`cellCount` is the total number of items) while the number 
>> of actual cells or number of visible nodes
>> used is given by `sheetChildren.size()`.  This means that all the actual 
>> cells (nodes) that are used by the
>> `VirtualFlow` are disposed and recreated all over again every time the 
>> number of items changes, triggering all those
>> calls to unregister those nodes from the scene that ultimately lead to 
>> remove the listeners with
>> `ExpressionHelper.removeListener`.  However, this seems unnecessary, as the 
>> number of actual cells/nodes doesn't change
>> that much, and causes this very bad performance.  On a quick test over the 
>> sample posted, just removing that line gives
>> a noticeable improvement in performance..
>> There is a concern though due to the
>> [comment](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L839):
>> // Fix for RT-13965: Without this line of code, the number of items in
>> // the sheet would constantly grow, leaking memory for the life of the
>> // application. This was especially apparent when the total number of
>> // cells changes - regardless of whether it became bigger or smaller.
>> sheetChildren.clear();
>> 
>> There are some methods in VirtualFlow that already manage the lifecycle of 
>> this list of nodes (clear, remove cells, add
>> cells, ...), so I don't think that is the case anymore for that comment: the 
>> number of items in the sheet doesn't
>> constantly grow and there is no memory leak.  Running the attached sample 
>> for a long time, and profiling with VisualVM,
>> shows improved performance (considerable drop in CPU usage), and no issues 
>> regarding memory usage.
>> So I'd like to propose this third alternative, which would affect only 
>> VirtualFlow and the controls that use it, but
>> wouldn't have any impact in the rest of controls as the other two options 
>> (as `ExpressionHelper` or `Node` listeners
>> wouldn't be modified).  Thoughts and feedback are welcome.
>
>> So I'd like to propose this third alternative, which would affect only 
>> VirtualFlow and the controls that use it, but
>> wouldn't have any impact in the rest of controls as the other two options 
>> (as ExpressionHelper or Node listeners
>> wouldn't be modified).
> 
> Given PR #185, which was mentioned above, (it isn't out for review yet, but I 
> want to evaluate it), this would be a 4th
> approach.
> As long as this really doesn't introduce a leak, it seems promising.
> 
> I note that these are not mutually exclusive.
> 
> We should discuss this on the list and not just in one or more of of the 3 
> (soon to be 4) open pull requests.

@dannygonzalez Per [this 
message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-September/027534.html)
 on
the openjfx-dev mailing list, I have filed a new JBS issue for this PR to use. 
Please change the title to:

8252936: Optimize removal of listeners from ExpressionHelper.Generic

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v4]

[yosbits]

On Wed, 2 Sep 2020 10:46:48 GMT, Jeanette Winzenburg  
wrote:

>> When the startup time is measured by eye, the impression changes depending 
>> on the individual difference.
>> The effect of runLater affects your experience.
>> 
>> However, I succeeded in further improving performance by eliminating another 
>> bottleneck in TreeTableView. Of course, it
>> also includes improvements in startup time.
>> I plan to commit at a later date.
>
>> 
>> 
>> When the startup time is measured by eye, the impression changes depending 
>> on the individual difference.
> 
> my eye is a precision instrument :) Seriously, who would do such a thingy? 
> Obviously, it must be measured, for zeroth
> approximation doing so crudely by comparing currentMillis before/after 
> showing is good enough to "see" the tendency.
>> The effect of runLater affects your experience.
> 
> that's why you shouldn't do it when trying to gain insight into timing issues 
> ;)
> 
>> 
>> However, I succeeded in further improving performance by eliminating another 
>> bottleneck in TreeTableView. Of course, it
>> also includes improvements in startup time.
> 
> cool :)

Column virtualization causes shortness of breath when scrolling a large stroke 
horizontally.
This does not happen when stroking on the scrollbar. Looks like a potential 
problem with VirtualFlow.

If you are worried about shortness of breath, the following code will help 
reduce the problem.


 Java
　private static final int OVERLAP_MARGIN = 500;

private static boolean isOverlap(double start, double end, double start2, 
double end2){
start = Math.max(start-OVERLAP_MARGIN, start2);
end = Math.min(end+OVERLAP_MARGIN, end2);
return (start<=end2 && end >= start2);
}

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v4]

[yosbits]

> If there are many columns, the current TableView will stall scrolling. 
> Resolving this performance issue requires column
> virtualization. Virtualization mode is enabled when the row height is fixed 
> by the following method.
> `tableView.setFixedCellSize(height)`
> 
> This proposal includes a fix because the current code does not correctly 
> implement column virtualization.
> 
> The improvement of this proposal can be seen in the following test program.
> 
>  Java
> import java.util.Arrays;
> import java.util.Collections;
> 
> import javafx.animation.AnimationTimer;
> import javafx.application.Application;
> import javafx.beans.property.SimpleStringProperty;
> import javafx.collections.ObservableList;
> import javafx.scene.Scene;
> import javafx.scene.control.Button;
> import javafx.scene.control.TableColumn;
> import javafx.scene.control.TableView;
> import javafx.scene.layout.BorderPane;
> import javafx.scene.layout.HBox;
> import javafx.stage.Stage;
> 
> public class BigTableViewTest2 extends Application {
>   private static final boolean USE_WIDTH_FIXED_SIZE = false;
>   private static final boolean USE_HEIGHT_FIXED_SIZE = true;
> //private static final int COL_COUNT=30;
> //private static final int COL_COUNT=300;
> //private static final int COL_COUNT=600;
>   private static final int COL_COUNT = 1000;
>   private static final int ROW_COUNT = 1000;
> 
>   @Override
>   public void start(final Stage primaryStage) throws Exception {
>   final TableView tableView = new TableView<>();
> 
> //tableView.setTableMenuButtonVisible(true); //too heavy
>   if (USE_HEIGHT_FIXED_SIZE) {
>   tableView.setFixedCellSize(24);
>   }
> 
>   final ObservableList> columns = 
> tableView.getColumns();
>   for (int i = 0; i < COL_COUNT; i++) {
>   final TableColumn column = new 
> TableColumn<>("Col" + i);
>   final int colIndex = i;
>   column.setCellValueFactory((cell) -> new 
> SimpleStringProperty(cell.getValue()[colIndex]));
>   columns.add(column);
>   if (USE_WIDTH_FIXED_SIZE) {
>   column.setPrefWidth(60);
>   column.setMaxWidth(60);
>   column.setMinWidth(60);
>   }
>   }
> 
>   final Button load = new Button("load");
>   load.setOnAction(e -> {
>   final ObservableList items = 
> tableView.getItems();
>   items.clear();
>   for (int i = 0; i < ROW_COUNT; i++) {
>   final String[] rec = new String[COL_COUNT];
>   for (int j = 0; j < rec.length; j++) {
>   rec[j] = i + ":" + j;
>   }
>   items.add(rec);
>   }
>   });
> 
>   final Button reverse = new Button("reverse columns");
>   reverse.setOnAction(e -> {
>   final TableColumn[] itemsArray = 
> columns.toArray(new TableColumn[0]);
>   Collections.reverse(Arrays.asList(itemsArray));
>   tableView.getColumns().clear();
>   
> tableView.getColumns().addAll(Arrays.asList(itemsArray));
>   });
> 
>   final Button hide = new Button("hide % 10");
>   hide.setOnAction(e -> {
>   for (int i = 0, n = columns.size(); i < n; i++) {
>   if (i % 10 == 0) {
>   columns.get(i).setVisible(false);
>   }
>   }
>   });
> 
>   final BorderPane root = new BorderPane(tableView);
>   root.setTop(new HBox(8, load, reverse, hide));
> 
>   final Scene scene = new Scene(root, 800, 800);
>   primaryStage.setScene(scene);
>   primaryStage.show();
>   this.prepareTimeline(scene);
>   }
> 
>   public static void main(final String[] args) {
>   Application.launch(args);
>   }
> 
>   private void prepareTimeline(final Scene scene) {
>   new AnimationTimer() {
>   @Override
>   public void handle(final long now) {
>   final double fps = 
> com.sun.javafx.perf.PerformanceTracker.getSceneTracker(scene).getInstantFPS();
>   ((Stage) scene.getWindow()).setTitle("FPS:" + 
> (int) fps);
>   }
>   }.start();
>   }
> }
> 
>  Java
> import javafx.animation.AnimationTimer;
> import javafx.application.Application;
> import javafx.application.Platform;
> import 

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v3]

[Jeanette Winzenburg]

On Tue, 1 Sep 2020 20:57:58 GMT, yosbits 
 wrote:

> 
> 
> When the startup time is measured by eye, the impression changes depending on 
> the individual difference.

my eye is a precision instrument :) Seriously, who would do such a thingy? 
Obviously, it must be measured, for zeroth
approximation doing so crudely by comparing currentMillis before/after showing 
is good enough to "see" the tendency.

> The effect of runLater affects your experience.

that's why you shouldn't do it when trying to gain insight into timing issues ;)

> 
> However, I succeeded in further improving performance by eliminating another 
> bottleneck in TreeTableView. Of course, it
> also includes improvements in startup time.

cool :)

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v3]

[yosbits]

On Tue, 1 Sep 2020 14:12:15 GMT, Jeanette Winzenburg  
wrote:

>> Since there was a problem that the cell was not updated when the window was 
>> maximized, I added a fix to
>> TreeTableViewSkin.java.
>> Added test code (BigTreeTableViewTest) for TreeTableView to the first post.
>> 
>> @kleopatra
>> Does the test code I added (BigTreeTableViewTest.java) also have side 
>> effects?
>
>> 
>> 
>> Since there was a problem that the cell was not updated when the window was 
>> maximized, I added a fix to
>> TreeTableViewSkin.java.
> 
> that's just the added listener to hbar properties (same as in TableViewSkin), 
> or anything else changed?
> 
>> Added test code (BigTreeTableViewTest) for TreeTableView to the first post.
>> 
>> @kleopatra
>> Does the test code I added (BigTreeTableViewTest.java) also have side 
>> effects?
> 
> yeah, but why do you ask me - you could easily see for yourself :) And don't 
> understand why you wrap the hierarchy
> setup in runlater (which smears a bit over the delay by filling the treeTable 
> content at an unspecific time "later")

When the startup time is measured by eye, the impression changes depending on 
the individual difference.
The effect of runLater affects your experience.

However, I succeeded in further improving performance by eliminating another 
bottleneck in TreeTableView. Of course, it
also includes improvements in startup time.

I plan to commit at a later date.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v3]

[Jeanette Winzenburg]

On Mon, 31 Aug 2020 19:16:31 GMT, yosbits 
 wrote:

> 
> 
> Since there was a problem that the cell was not updated when the window was 
> maximized, I added a fix to
> TreeTableViewSkin.java.

that's just the added listener to hbar properties (same as in TableViewSkin), 
or anything else changed?

> Added test code (BigTreeTableViewTest) for TreeTableView to the first post.
> 
> @kleopatra
> Does the test code I added (BigTreeTableViewTest.java) also have side effects?

yeah, but why do you ask me - you could easily see for yourself :) And don't 
understand why you wrap the hierarchy
setup in runlater (which smears a bit over the delay by filling the treeTable 
content at an unspecific time "later")

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v3]

[yosbits]

On Sun, 30 Aug 2020 13:20:49 GMT, Jeanette Winzenburg  
wrote:

>> When setting the cell height to a fixed value with setFixedCellSize(), 
>> column virtualization can improve performance.
>> 
>> As the next step, I think it is possible to try to enable column 
>> virtualization regardless of whether
>> setFixedCellSize() is set or not. Before doing this step, the direct access 
>> validation of the internal structure of the
>> test code needs to be changed via the public API.
>
> this indeed improves scrolling performance considerably (from extremely 
> lagging to smooth following the mouse when
> dragging the thumb of the vertical scrollbar).
> As a side-effect, start-up time of TreeTableView seems to increase 
> considerably (at least by a factor of 3 to 4,
> probably not linear in # of columns) - see f.i. the [example in
> JDK-8166956](https://bugs.openjdk.java.net/browse/JDK-8166956). Any idea why 
> that might happen?

Since there was a problem that the cell was not updated when the window was 
maximized, I added a fix to
TreeTableViewSkin.java.

Added test code (BigTreeTableViewTest) for TreeTableView to the first post.

@kleopatra
Does the test code I added (BigTreeTableViewTest.java) also have side effects?

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v3]

[yosbits]

> If there are many columns, the current TableView will stall scrolling. 
> Resolving this performance issue requires column
> virtualization. Virtualization mode is enabled when the row height is fixed 
> by the following method.
> `tableView.setFixedCellSize(height)`
> 
> This proposal includes a fix because the current code does not correctly 
> implement column virtualization.
> 
> The improvement of this proposal can be seen in the following test program.
> 
>  Java
> import java.util.Arrays;
> import java.util.Collections;
> 
> import javafx.animation.AnimationTimer;
> import javafx.application.Application;
> import javafx.beans.property.SimpleStringProperty;
> import javafx.collections.ObservableList;
> import javafx.scene.Scene;
> import javafx.scene.control.Button;
> import javafx.scene.control.TableColumn;
> import javafx.scene.control.TableView;
> import javafx.scene.layout.BorderPane;
> import javafx.scene.layout.HBox;
> import javafx.stage.Stage;
> 
> public class BigTableViewTest2 extends Application {
>   private static final boolean USE_WIDTH_FIXED_SIZE = false;
>   private static final boolean USE_HEIGHT_FIXED_SIZE = true;
> //private static final int COL_COUNT=30;
> //private static final int COL_COUNT=300;
> //private static final int COL_COUNT=600;
>   private static final int COL_COUNT = 1000;
>   private static final int ROW_COUNT = 1000;
> 
>   @Override
>   public void start(final Stage primaryStage) throws Exception {
>   final TableView tableView = new TableView<>();
> 
> //tableView.setTableMenuButtonVisible(true); //too heavy
>   if (USE_HEIGHT_FIXED_SIZE) {
>   tableView.setFixedCellSize(24);
>   }
> 
>   final ObservableList> columns = 
> tableView.getColumns();
>   for (int i = 0; i < COL_COUNT; i++) {
>   final TableColumn column = new 
> TableColumn<>("Col" + i);
>   final int colIndex = i;
>   column.setCellValueFactory((cell) -> new 
> SimpleStringProperty(cell.getValue()[colIndex]));
>   columns.add(column);
>   if (USE_WIDTH_FIXED_SIZE) {
>   column.setPrefWidth(60);
>   column.setMaxWidth(60);
>   column.setMinWidth(60);
>   }
>   }
> 
>   final Button load = new Button("load");
>   load.setOnAction(e -> {
>   final ObservableList items = 
> tableView.getItems();
>   items.clear();
>   for (int i = 0; i < ROW_COUNT; i++) {
>   final String[] rec = new String[COL_COUNT];
>   for (int j = 0; j < rec.length; j++) {
>   rec[j] = i + ":" + j;
>   }
>   items.add(rec);
>   }
>   });
> 
>   final Button reverse = new Button("reverse columns");
>   reverse.setOnAction(e -> {
>   final TableColumn[] itemsArray = 
> columns.toArray(new TableColumn[0]);
>   Collections.reverse(Arrays.asList(itemsArray));
>   tableView.getColumns().clear();
>   
> tableView.getColumns().addAll(Arrays.asList(itemsArray));
>   });
> 
>   final Button hide = new Button("hide % 10");
>   hide.setOnAction(e -> {
>   for (int i = 0, n = columns.size(); i < n; i++) {
>   if (i % 10 == 0) {
>   columns.get(i).setVisible(false);
>   }
>   }
>   });
> 
>   final BorderPane root = new BorderPane(tableView);
>   root.setTop(new HBox(8, load, reverse, hide));
> 
>   final Scene scene = new Scene(root, 800, 800);
>   primaryStage.setScene(scene);
>   primaryStage.show();
>   this.prepareTimeline(scene);
>   }
> 
>   public static void main(final String[] args) {
>   Application.launch(args);
>   }
> 
>   private void prepareTimeline(final Scene scene) {
>   new AnimationTimer() {
>   @Override
>   public void handle(final long now) {
>   final double fps = 
> com.sun.javafx.perf.PerformanceTracker.getSceneTracker(scene).getInstantFPS();
>   ((Stage) scene.getWindow()).setTitle("FPS:" + 
> (int) fps);
>   }
>   }.start();
>   }
> }

yosbits has updated the pull request with a new target base due to a merge or a 
rebase. The incremental webrev excludes
the unrelated changes brought in by the 

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v2]

[Jeanette Winzenburg]

On Thu, 27 Aug 2020 06:37:53 GMT, yosbits 
 wrote:

>> 19fabf2eafcb02b519d39a1b0a9dad5b9209db64
>> 
>> * Constructor creates multiple cell nodes, but the
>> Fixed a wasteful problem of creating all cells and deleting out-of-display 
>> cells because the fixedCellSize attribute
>> was not initialized in the constructor..
>> * Reduce the load on ExpressionHelper.removeListener by removing cells 
>> outside of the display range from the back of the
>>   scrolling operation.
>
> When setting the cell height to a fixed value with setFixedCellSize(), column 
> virtualization can improve performance.
> 
> As the next step, I think it is possible to try to enable column 
> virtualization regardless of whether
> setFixedCellSize() is set or not. Before doing this step, the direct access 
> validation of the internal structure of the
> test code needs to be changed via the public API.

this indeed improves scrolling performance considerably (from extremely lagging 
to smooth following the mouse when
dragging the thumb of the vertical scrollbar).

As a side-effect, start-up time of TreeTableView seems to increase considerably 
(at least by a factor of 3 to 4,
probably not linear in # of columns) - see f.i. the [example in
JDK-8166956](https://bugs.openjdk.java.net/browse/JDK-8166956). Any idea why 
that might happen?

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v2]

[yosbits]

On Sat, 18 Apr 2020 17:40:30 GMT, yosbits 
 wrote:

>> My name is "Naohiro Yoshimoto"
>> I have received an approved email on 2019-8-3.
>
> 19fabf2eafcb02b519d39a1b0a9dad5b9209db64
> 
> * Constructor creates multiple cell nodes, but the
> Fixed a wasteful problem of creating all cells and deleting out-of-display 
> cells because the fixedCellSize attribute
> was not initialized in the constructor..
> * Reduce the load on ExpressionHelper.removeListener by removing cells 
> outside of the display range from the back of the
>   scrolling operation.

When setting the cell height to a fixed value with setFixedCellSize(), column 
virtualization can improve performance.

As the next step, I think it is possible to try to enable column virtualization 
regardless of whether
setFixedCellSize() is set or not. Before doing this step, the direct access 
validation of the internal structure of the
test code needs to be changed via the public API.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Kevin Rushforth]

On Wed, 26 Aug 2020 15:40:57 GMT, Jose Pereda  wrote:

> So I'd like to propose this third alternative, which would affect only 
> VirtualFlow and the controls that use it, but
> wouldn't have any impact in the rest of controls as the other two options (as 
> ExpressionHelper or Node listeners
> wouldn't be modified).

Given PR #185, which was mentioned above, (it isn't out for review yet, but I 
want to evaluate it), this would be a 4th
approach.

As long as this really doesn't introduce a leak, it seems promising.

I note that these are not mutually exclusive.

We should discuss this on the list and not just in one or more of of the 3 
(soon to be 4) open pull requests.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Wed, 26 Aug 2020 17:44:20 GMT, yosbits 
 wrote:

>> So far, there are two alternative fixes for the bad performance issue while 
>> scrolling TableView/TreeTableViews:
>> 
>> - this one #108, that tries to improve the performance of excessive number 
>> of `removeListener` calls
>> - the WIP #185 that avoids registering two listeners (on Scene and on 
>> Window) for each and every Node.
>> 
>> For the case presented, where new items are constantly added to the 
>> TableView, the trace of calls that reach
>> `com.sun.javafx.binding.ExpressionHelper.removeListener()` is something like 
>> this:
>> ![TraceOpenJFX16ea1](https://user-images.githubusercontent.com/2043230/91322208-c2ba9900-e7bf-11ea-8918-cdbecd457cf2.png)
>>  
>> As can be seen, all those calls are triggered by the change of the number of 
>> cells in
>> [VirtualFlow::setCellCount](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L804).
>> Whenever the cell count changes there is this
>> [call](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L843):
>> sheetChildren.clear();
>> 
>> This happens every time the number of items in the `TableView` changes, as 
>> the `VirtualFlow` keeps track of the number
>> of virtual cells (`cellCount` is the total number of items) while the number 
>> of actual cells or number of visible nodes
>> used is given by `sheetChildren.size()`.  This means that all the actual 
>> cells (nodes) that are used by the
>> `VirtualFlow` are disposed and recreated all over again every time the 
>> number of items changes, triggering all those
>> calls to unregister those nodes from the scene that ultimately lead to 
>> remove the listeners with
>> `ExpressionHelper.removeListener`.  However, this seems unnecessary, as the 
>> number of actual cells/nodes doesn't change
>> that much, and causes this very bad performance.  On a quick test over the 
>> sample posted, just removing that line gives
>> a noticeable improvement in performance..
>> There is a concern though due to the
>> [comment](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L839):
>> // Fix for RT-13965: Without this line of code, the number of items in
>> // the sheet would constantly grow, leaking memory for the life of the
>> // application. This was especially apparent when the total number of
>> // cells changes - regardless of whether it became bigger or smaller.
>> sheetChildren.clear();
>> 
>> There are some methods in VirtualFlow that already manage the lifecycle of 
>> this list of nodes (clear, remove cells, add
>> cells, ...), so I don't think that is the case anymore for that comment: the 
>> number of items in the sheet doesn't
>> constantly grow and there is no memory leak.  Running the attached sample 
>> for a long time, and profiling with VisualVM,
>> shows improved performance (considerable drop in CPU usage), and no issues 
>> regarding memory usage.
>> So I'd like to propose this third alternative, which would affect only 
>> VirtualFlow and the controls that use it, but
>> wouldn't have any impact in the rest of controls as the other two options 
>> (as `ExpressionHelper` or `Node` listeners
>> wouldn't be modified).  Thoughts and feedback are welcome.
>
> I confirmed the sample code (JavaFX Sluggish),
> This is not scroll performance
> It seems to reproduce the additional performance issue.
> Therefore, it is not considered appropriate as a fix for JDK-8185886.
> I know you are reproducing another performance issue, but
> I'm proposing to fix scrolling performance issues in #125.

The https://github.com/openjdk/jfx/pull/185 is a full fix, not a WIP.  It 
avoids registering the listeners on Scene and
Window and moves the only uses of those listeners to their respective controls, 
PopupWindow  and ProgressIndicatorSkin
(the property involved is internal, so there is no risk of affecting 3rd 
parties).

Please have a closer look.

I updated the title to make it more clear it is no longer a WIP, unless someone 
has review comments.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[yosbits]

On Wed, 26 Aug 2020 15:40:57 GMT, Jose Pereda  wrote:

>> I have attached a code sample. If you use OpenJFX 16-ea+1 and run visual VM 
>> and look at the hotspots in the JavaFX
>> thread, you can see that about 45% of the time in the JavaFX thread is spent 
>> in removeListener calls.
>> Note: In CPU settings of VisualVM, I removed all packages from the "Do not 
>> profile packages section".
>> 
>> [JavaFXSluggish.java.zip](https://github.com/openjdk/jfx/files/5130298/JavaFXSluggish.java.zip)
>
> So far, there are two alternative fixes for the bad performance issue while 
> scrolling TableView/TreeTableViews:
> 
> - this one #108, that tries to improve the performance of excessive number of 
> `removeListener` calls
> - the WIP #185 that avoids registering two listeners (on Scene and on Window) 
> for each and every Node.
> 
> For the case presented, where new items are constantly added to the 
> TableView, the trace of calls that reach
> `com.sun.javafx.binding.ExpressionHelper.removeListener()` is something like 
> this:
> ![TraceOpenJFX16ea1](https://user-images.githubusercontent.com/2043230/91322208-c2ba9900-e7bf-11ea-8918-cdbecd457cf2.png)
>  
> As can be seen, all those calls are triggered by the change of the number of 
> cells in
> [VirtualFlow::setCellCount](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L804).
> Whenever the cell count changes there is this
> [call](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L843):
> sheetChildren.clear();
> 
> This happens every time the number of items in the `TableView` changes, as 
> the `VirtualFlow` keeps track of the number
> of virtual cells (`cellCount` is the total number of items) while the number 
> of actual cells or number of visible nodes
> used is given by `sheetChildren.size()`.  This means that all the actual 
> cells (nodes) that are used by the
> `VirtualFlow` are disposed and recreated all over again every time the number 
> of items changes, triggering all those
> calls to unregister those nodes from the scene that ultimately lead to remove 
> the listeners with
> `ExpressionHelper.removeListener`.  However, this seems unnecessary, as the 
> number of actual cells/nodes doesn't change
> that much, and causes this very bad performance.  On a quick test over the 
> sample posted, just removing that line gives
> a noticeable improvement in performance..
> There is a concern though due to the
> [comment](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L839):
> // Fix for RT-13965: Without this line of code, the number of items in
> // the sheet would constantly grow, leaking memory for the life of the
> // application. This was especially apparent when the total number of
> // cells changes - regardless of whether it became bigger or smaller.
> sheetChildren.clear();
> 
> There are some methods in VirtualFlow that already manage the lifecycle of 
> this list of nodes (clear, remove cells, add
> cells, ...), so I don't think that is the case anymore for that comment: the 
> number of items in the sheet doesn't
> constantly grow and there is no memory leak.  Running the attached sample for 
> a long time, and profiling with VisualVM,
> shows improved performance (considerable drop in CPU usage), and no issues 
> regarding memory usage.
> So I'd like to propose this third alternative, which would affect only 
> VirtualFlow and the controls that use it, but
> wouldn't have any impact in the rest of controls as the other two options (as 
> `ExpressionHelper` or `Node` listeners
> wouldn't be modified).  Thoughts and feedback are welcome.

I confirmed the sample code (JavaFX Sluggish),
This is not scroll performance
It seems to reproduce the additional performance issue.
Therefore, it is not considered appropriate as a fix for JDK-8185886.
I know you are reproducing another performance issue, but
I'm proposing to fix scrolling performance issues in #125.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Jose Pereda]

On Wed, 26 Aug 2020 14:08:37 GMT, dannygonzalez 
 wrote:

>> @hjohn, agreed regards the issues of adding a listener to each node.
>> 
>> Would it be worth doing the additional work of updating PopupWindow and 
>> ProgressIndicatorSkin to add their own
>> listeners to make this a pull request that can be reviewed officially?
>> I await any further comments from @kevinrushforth et al.
>
> I have attached a code sample. If you use OpenJFX 16-ea+1 and run visual VM 
> and look at the hotspots in the JavaFX
> thread, you can see that about 45% of the time in the JavaFX thread is spent 
> in removeListener calls.
> Note: In CPU settings of VisualVM, I removed all packages from the "Do not 
> profile packages section".
> 
> [JavaFXSluggish.java.zip](https://github.com/openjdk/jfx/files/5130298/JavaFXSluggish.java.zip)

So far, there are two alternative fixes for the bad performance issue while 
scrolling TableView/TreeTableViews:

- this one #108, that tries to improve the performance of excessive number of 
`removeListener` calls
- the WIP #185 that avoids registering two listeners (on Scene and on Window) 
for each and every Node.

For the case presented, where new items are constantly added to the TableView, 
the trace of calls that reach
`com.sun.javafx.binding.ExpressionHelper.removeListener()` is something like 
this:

![TraceOpenJFX16ea1](https://user-images.githubusercontent.com/2043230/91322208-c2ba9900-e7bf-11ea-8918-cdbecd457cf2.png)
 
As can be seen, all those calls are triggered by the change of the number of 
cells in
[VirtualFlow::setCellCount](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L804).

Whenever the cell count changes there is this
[call](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L843):

sheetChildren.clear();

This happens every time the number of items in the `TableView` changes, as the 
`VirtualFlow` keeps track of the number
of virtual cells (`cellCount` is the total number of items) while the number of 
actual cells or number of visible nodes
used is given by `sheetChildren.size()`.

This means that all the actual cells (nodes) that are used by the `VirtualFlow` 
are disposed and recreated all over
again every time the number of items changes, triggering all those calls to 
unregister those nodes from the scene that
ultimately lead to remove the listeners with `ExpressionHelper.removeListener`.

However, this seems unnecessary, as the number of actual cells/nodes doesn't 
change that much, and causes this very bad
performance.

On a quick test over the sample posted, just removing that line gives a 
noticeable improvement in performance..

There is a concern though due to the
[comment](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L839):

// Fix for RT-13965: Without this line of code, the number of items in
// the sheet would constantly grow, leaking memory for the life of the
// application. This was especially apparent when the total number of
// cells changes - regardless of whether it became bigger or smaller.
sheetChildren.clear();

There are some methods in VirtualFlow that already manage the lifecycle of this 
list of nodes (clear, remove cells, add
cells, ...), so I don't think that is the case anymore for that comment: the 
number of items in the sheet doesn't
constantly grow and there is no memory leak.

Running the attached sample for a long time, and profiling with VisualVM, shows 
improved performance (considerable drop
in CPU usage), and no issues regarding memory usage.

So I'd like to propose this third alternative, which would affect only 
VirtualFlow and the controls that use it, but
wouldn't have any impact in the rest of controls as the other two options (as 
`ExpressionHelper` or `Node` listeners
wouldn't be modified).

Thoughts and feedback are welcome.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Fri, 17 Apr 2020 10:46:51 GMT, dannygonzalez 
 wrote:

>> The problem is that there are usually many nodes, but only very few scenes 
>> and windows, so registering a listener for
>> each node on a scene or window is pretty bad design (also consider the 
>> amount of notifications that a scene change
>> would trigger in such scenarios).  As far as I can see, this is the only 
>> such listener and only needed for two very
>> limited cases, and its addition in the past may have slipped through the 
>> cracks.  Adding a performance unit test that
>> specifically checks add/remove performance of nodes may prevent such future 
>> regressions.
>
> @hjohn, agreed regards the issues of adding a listener to each node.
> 
> Would it be worth doing the additional work of updating PopupWindow and 
> ProgressIndicatorSkin to add their own
> listeners to make this a pull request that can be reviewed officially?
> I await any further comments from @kevinrushforth et al.

I have attached a code sample. If you use OpenJFX 16-ea+1 and run visual VM and 
look at the hotspots in the JavaFX
thread, you can see that about 45% of the time in the JavaFX thread is spent in 
removeListener calls.

Note: In CPU settings of VisualVM, I removed all packages from the "Do not 
profile packages section".

[JavaFXSluggish.java.zip](https://github.com/openjdk/jfx/files/5130298/JavaFXSluggish.java.zip)

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Fri, 17 Apr 2020 10:22:15 GMT, John Hendrikx  wrote:

>> @hjohn Thanks for looking into this. It looks like your changes do improve 
>> the issue with the JavaFX thread being
>> swamped with listener de-registrations. Looking at the JavaFX thread in 
>> VisualVM, the removeListener call has dropped
>> off the hotspots in the same way it did with my pull request.  I wasn't 
>> fully confident of making changes to the Node
>> hierarchy to remove listeners hence why I approached the issue from the 
>> other direction i.e. the obvious bottleneck
>> which was the listener de-registration slowness.  I do worry however that 
>> any changes down the road which add listeners
>> to the Node hierarchy again without fully understanding the implications 
>> would lead us to the same point we are now
>> where the slowness of listener de-registrations becomes an issue again. 
>> There are no tests that catch this scenario.  I
>> feel that ideally both solutions are needed but am happy to bow to the more 
>> experienced OpenJFX devs opinions here as I
>> know my changes may be more fundamental and hence risky.
>
> The problem is that there are usually many nodes, but only very few scenes 
> and windows, so registering a listener for
> each node on a scene or window is pretty bad design (also consider the amount 
> of notifications that a scene change
> would trigger in such scenarios).  As far as I can see, this is the only such 
> listener and only needed for two very
> limited cases, and its addition in the past may have slipped through the 
> cracks.  Adding a performance unit test that
> specifically checks add/remove performance of nodes may prevent such future 
> regressions.

@hjohn, agreed regards the issues of adding a listener to each node.

Would it be worth doing the additional work of updating PopupWindow and 
ProgressIndicatorSkin to add their own
listeners to make this a pull request that can be reviewed officially?

I await any further comments from @kevinrushforth et al.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Fri, 17 Apr 2020 08:48:35 GMT, dannygonzalez 
 wrote:

>> @dannygonzalez I added a proof of concept here if you want to play with it: 
>> https://github.com/openjdk/jfx/pull/185
>
> @hjohn Thanks for looking into this. It looks like your changes do improve 
> the issue with the JavaFX thread being
> swamped with listener de-registrations. Looking at the JavaFX thread in 
> VisualVM, the removeListener call has dropped
> off the hotspots in the same way it did with my pull request.  I wasn't fully 
> confident of making changes to the Node
> hierarchy to remove listeners hence why I approached the issue from the other 
> direction i.e. the obvious bottleneck
> which was the listener de-registration slowness.  I do worry however that any 
> changes down the road which add listeners
> to the Node hierarchy again without fully understanding the implications 
> would lead us to the same point we are now
> where the slowness of listener de-registrations becomes an issue again. There 
> are no tests that catch this scenario.  I
> feel that ideally both solutions are needed but am happy to bow to the more 
> experienced OpenJFX devs opinions here as I
> know my changes may be more fundamental and hence risky.

The problem is that there are usually many nodes, but only very few scenes and 
windows, so registering a listener for
each node on a scene or window is pretty bad design (also consider the amount 
of notifications that a scene change
would trigger in such scenarios).  As far as I can see, this is the only such 
listener and only needed for two very
limited cases, and its addition in the past may have slipped through the cracks.

Adding a performance unit test that specifically checks add/remove performance 
of nodes may prevent such future
regressions.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Fri, 17 Apr 2020 08:07:08 GMT, John Hendrikx  wrote:

>> @hjohn
>> 
>>> 2. There is a significant increase in memory use. Where before each 
>>> listener occupied an entry in an array, now each
>>> listener is wrapped by Map.Entry (the Integer instance used per entry can 
>>> be disregarded). I estimate around 4-8x more
>>> heap will be consumed (the numbers are small however, still well below 1 MB 
>>> for 20.000 entries). If this is an issue, a
>>> further level could be introduced in the listener implementation hierarchy 
>>> (singleton -> array -> map).
>> 
>> There was discussion about lazy initialisation of the LinkedHashMap when 
>> needed and/or have some sort of strategy where
>> we could use arrays or lists if the number of listeners are less than some 
>> threshold (i.e. introducing another level to
>> the hierarchy as you mentioned). This was mentioned here also:
>> https://github.com/openjdk/jfx/pull/108#issuecomment-590838942
>
> @dannygonzalez I added a proof of concept here if you want to play with it: 
> https://github.com/openjdk/jfx/pull/185

@hjohn Thanks for looking into this. It looks like your changes do improve the 
issue with the JavaFX thread being
swamped with listener de-registrations. Looking at the JavaFX thread in 
VisualVM, the removeListener call has dropped
off the hotspots in the same way it did with my pull request.

I wasn't fully confident of making changes to the Node hierarchy to remove 
listeners hence why I approached the issue
from the other direction i.e. the obvious bottleneck which was the listener 
de-registration slowness.

I do worry however that any changes down the road which add listeners to the 
Node hierarchy again without fully
understanding the implications would lead us to the same point we are now where 
the slowness of listener
de-registrations becomes an issue again. There are no tests that catch this 
scenario.  I feel that ideally both
solutions are needed but am happy to bow to the more experienced OpenJFX devs 
opinions here as I know my changes may be
more fundamental and hence risky.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Fri, 17 Apr 2020 07:22:20 GMT, dannygonzalez 
 wrote:

>> @hjon
>> 
>>> 3. Even though the current version of this pull request takes care to 
>>> notify duplicate listeners the correct amount of
>>> times, it does not notify them in the correct order with respect to other 
>>> listeners. If one registers listeners (a, b,
>>> a) the notification order will be (a, a, b).
>> 
>> Unless I'm missing something I don't think this is the case. I used a 
>> LinkedHashMap which preserved the order of
>> notifications. Actually some unit tests failed if the notifications weren't 
>> carried out in the same order as
>> registration which was the case when I used a HashMap. See here:
>> https://github.com/openjdk/jfx/pull/108#issuecomment-590883183
>
> @hjohn
> 
>> 2. There is a significant increase in memory use. Where before each listener 
>> occupied an entry in an array, now each
>> listener is wrapped by Map.Entry (the Integer instance used per entry can be 
>> disregarded). I estimate around 4-8x more
>> heap will be consumed (the numbers are small however, still well below 1 MB 
>> for 20.000 entries). If this is an issue, a
>> further level could be introduced in the listener implementation hierarchy 
>> (singleton -> array -> map).
> 
> There was discussion about lazy initialisation of the LinkedHashMap when 
> needed and/or have some sort of strategy where
> we could use arrays or lists if the number of listeners are less than some 
> threshold (i.e. introducing another level to
> the hierarchy as you mentioned). This was mentioned here also:
> https://github.com/openjdk/jfx/pull/108#issuecomment-590838942

@dannygonzalez I added a proof of concept here if you want to play with it: 
https://github.com/openjdk/jfx/pull/185

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Fri, 17 Apr 2020 07:22:20 GMT, dannygonzalez 
 wrote:

>> @hjon
>> 
>>> 3. Even though the current version of this pull request takes care to 
>>> notify duplicate listeners the correct amount of
>>> times, it does not notify them in the correct order with respect to other 
>>> listeners. If one registers listeners (a, b,
>>> a) the notification order will be (a, a, b).
>> 
>> Unless I'm missing something I don't think this is the case. I used a 
>> LinkedHashMap which preserved the order of
>> notifications. Actually some unit tests failed if the notifications weren't 
>> carried out in the same order as
>> registration which was the case when I used a HashMap. See here:
>> https://github.com/openjdk/jfx/pull/108#issuecomment-590883183
>
> @hjohn
> 
>> 2. There is a significant increase in memory use. Where before each listener 
>> occupied an entry in an array, now each
>> listener is wrapped by Map.Entry (the Integer instance used per entry can be 
>> disregarded). I estimate around 4-8x more
>> heap will be consumed (the numbers are small however, still well below 1 MB 
>> for 20.000 entries). If this is an issue, a
>> further level could be introduced in the listener implementation hierarchy 
>> (singleton -> array -> map).
> 
> There was discussion about lazy initialisation of the LinkedHashMap when 
> needed and/or have some sort of strategy where
> we could use arrays or lists if the number of listeners are less than some 
> threshold (i.e. introducing another level to
> the hierarchy as you mentioned). This was mentioned here also:
> https://github.com/openjdk/jfx/pull/108#issuecomment-590838942

I've implemented an alternative solution: Removing the listeners on 
`Window.showingProperty` and `Scene.windowProperty`
completely.  They are in fact only used in two places: `PopupWindow` in order 
to remove itself if the `Window` is no
longer showing, and `ProgressIndicatorSkin`.  These two can be easily replaced 
with their own listeners for these
properties instead of burdening **all** nodes with these listeners only to 
support these two classes.

I left the `isTreeShowing` method in, and implemented it simply as 
`isTreeVisible() && isWindowShowing()` as that's
really the only difference between "visible" and "showing" apparently.

Here is the test result with 20.000 nested StackPanes with only this change in:

- Add all 45 ms
- Remove all 25 ms

I think this might be a good solution as it completely avoids these listeners.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Fri, 17 Apr 2020 07:08:01 GMT, dannygonzalez 
 wrote:

>> I've tested this pull request locally a few times, and the performance 
>> improvement is quite significant.   A test with
>> 20.000 nested stack panes resulted in these average times:
>> - Add all 51 ms
>> - Remove all 25 ms
>> 
>> Versus the unmodified code:
>> 
>> - Add all 34 ms
>> - Remove all 135 ms
>> 
>> However, there are some significant functional changes as well that might 
>> impact current users:
>> 
>> 1. The new code ensures that all listeners are notified even if the list is 
>> modified during iteration by always making
>> a **copy** when an event is fired.  The old code only did so when it was 
>> **actually** modified during iteration.  This
>> can be mitigated by making the copy in the code that modifies the list (as 
>> the original did) using the `locked` flag to
>> check whether an iteration was in progress.2. There is a significant 
>> increase in memory use.  Where before each
>> listener occupied an entry in an array, now each listener is wrapped by 
>> `Map.Entry` (the Integer instance used per
>> entry can be disregarded).  I estimate around 4-8x more heap will be 
>> consumed (the numbers are small however, still
>> well below 1 MB for 20.000 entries).  If this is an issue, a further level 
>> could be introduced in the listener
>> implementation hierarchy (singleton -> array -> map).  3. Even though the 
>> current version of this pull request takes
>> care to notify duplicate listeners the correct amount of times, it does not 
>> notify them in the correct order with
>> respect to other listeners.  If one registers listeners (a, b, a) the 
>> notification order will be (a, a, b).  The last
>> point is not easily solved and could potentially cause problems.  Finally I 
>> think this solution, although it performs
>> well is not the full solution.  A doubling or quadrupling of nodes would 
>> again run into serious limitations.  I think
>> this commit 
>> https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd
>>  should not have introduced
>> another listener for each Node on the Window class.  A better solution would 
>> be to only have the Scene listen to Window
>> and have Scene provide a new combined status property that Node could use 
>> for its purposes.  Even better however would
>> be to change the properties involved to make use of the hierarchy naturally 
>> present in Nodes, having child nodes listen
>> to their parent, and the top level node listen to the scene.  This would 
>> reduce the amount of listeners on a single
>> property in Scene and Window immensely, instead spreading those listeners 
>> over the Node hierarchy, keeping listener
>> lists much  shorter, which should scale a lot better.
>
> @hjon
> 
>> 3. Even though the current version of this pull request takes care to notify 
>> duplicate listeners the correct amount of
>> times, it does not notify them in the correct order with respect to other 
>> listeners. If one registers listeners (a, b,
>> a) the notification order will be (a, a, b).
> 
> Unless I'm missing something I don't think this is the case. I used a 
> LinkedHashMap which preserved the order of
> notifications. Actually some unit tests failed if the notifications weren't 
> carried out in the same order as
> registration which was the case when I used a HashMap. See here:
> https://github.com/openjdk/jfx/pull/108#issuecomment-590883183

@hjohn

> 2. There is a significant increase in memory use. Where before each listener 
> occupied an entry in an array, now each
> listener is wrapped by Map.Entry (the Integer instance used per entry can be 
> disregarded). I estimate around 4-8x more
> heap will be consumed (the numbers are small however, still well below 1 MB 
> for 20.000 entries). If this is an issue, a
> further level could be introduced in the listener implementation hierarchy 
> (singleton -> array -> map).

There was discussion about lazy initialisation of the LinkedHashMap when needed 
and/or have some sort of strategy where
we could use arrays or lists if the number of listeners are less than some 
threshold (i.e. introducing another level to
the hierarchy as you mentioned). This was mentioned here also:
https://github.com/openjdk/jfx/pull/108#issuecomment-590838942

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 16 Apr 2020 16:15:20 GMT, John Hendrikx  wrote:

>> @hjohn I have 12136 change listeners when debugging our application as you 
>> suggested.
>> 
>> Please note that I see the issue when the TableView is having items added to 
>> it. If you just have a static TableView I
>> do not see the issue.
>> It is only when you add items to the TableView which causes a myriad of 
>> listeners to be deregistered and registered.
>> The Visual VM snapshot I attached above was taken as our application was 
>> adding items to the TableView.
>
> I've tested this pull request locally a few times, and the performance 
> improvement is quite significant.   A test with
> 20.000 nested stack panes resulted in these average times:
> - Add all 51 ms
> - Remove all 25 ms
> 
> Versus the unmodified code:
> 
> - Add all 34 ms
> - Remove all 135 ms
> 
> However, there are some significant functional changes as well that might 
> impact current users:
> 
> 1. The new code ensures that all listeners are notified even if the list is 
> modified during iteration by always making
> a **copy** when an event is fired.  The old code only did so when it was 
> **actually** modified during iteration.  This
> can be mitigated by making the copy in the code that modifies the list (as 
> the original did) using the `locked` flag to
> check whether an iteration was in progress.2. There is a significant 
> increase in memory use.  Where before each
> listener occupied an entry in an array, now each listener is wrapped by 
> `Map.Entry` (the Integer instance used per
> entry can be disregarded).  I estimate around 4-8x more heap will be consumed 
> (the numbers are small however, still
> well below 1 MB for 20.000 entries).  If this is an issue, a further level 
> could be introduced in the listener
> implementation hierarchy (singleton -> array -> map).  3. Even though the 
> current version of this pull request takes
> care to notify duplicate listeners the correct amount of times, it does not 
> notify them in the correct order with
> respect to other listeners.  If one registers listeners (a, b, a) the 
> notification order will be (a, a, b).  The last
> point is not easily solved and could potentially cause problems.  Finally I 
> think this solution, although it performs
> well is not the full solution.  A doubling or quadrupling of nodes would 
> again run into serious limitations.  I think
> this commit 
> https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd
>  should not have introduced
> another listener for each Node on the Window class.  A better solution would 
> be to only have the Scene listen to Window
> and have Scene provide a new combined status property that Node could use for 
> its purposes.  Even better however would
> be to change the properties involved to make use of the hierarchy naturally 
> present in Nodes, having child nodes listen
> to their parent, and the top level node listen to the scene.  This would 
> reduce the amount of listeners on a single
> property in Scene and Window immensely, instead spreading those listeners 
> over the Node hierarchy, keeping listener
> lists much  shorter, which should scale a lot better.

@hjon

> 3. Even though the current version of this pull request takes care to notify 
> duplicate listeners the correct amount of
> times, it does not notify them in the correct order with respect to other 
> listeners. If one registers listeners (a, b,
> a) the notification order will be (a, a, b).

Unless I'm missing something I don't think this is the case. I used a 
LinkedHashMap which preserved the order of
notifications. Actually some unit tests failed if the notifications weren't 
carried out in the same order as
registration which was the case when I used a HashMap. See here:
https://github.com/openjdk/jfx/pull/108#issuecomment-590883183

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Thu, 16 Apr 2020 09:45:20 GMT, dannygonzalez 
 wrote:

>> @dannygonzalez  Could you perhaps debug your application and take a look at 
>> how large the following array is: a random
>> node -> `scene` -> `value` -> `window` -> `readOnlyProperty` -> `helper` -> 
>> `changeListeners`.  I just tested this with
>> a custom control displaying 200 cells on screen at once (each cell 
>> consisting of about 30 nodes itself), and I saw
>> about 2 change listeners registered on this single Scene Window 
>> property.  However, this custom control is not
>> creating/destroying cells beyond the initial allocation, so there wouldn't 
>> be any registering and unregistering going
>> on, scrolling was still smooth >30 fps.
>
> @hjohn I have 12136 change listeners when debugging our application as you 
> suggested.
> 
> Please note that I see the issue when the TableView is having items added to 
> it. If you just have a static TableView I
> do not see the issue.
> It is only when you add items to the TableView which causes a myriad of 
> listeners to be deregistered and registered.
> The Visual VM snapshot I attached above was taken as our application was 
> adding items to the TableView.

I've tested this pull request locally a few times, and the performance 
improvement is quite significant.   A test with
20.000 nested stack panes resulted in these average times:

- Add all 51 ms
- Remove all 25 ms

Versus the unmodified code:

- Add all 34 ms
- Remove all 135 ms

However, there are some significant functional changes as well that might 
impact current users:

1. The new code ensures that all listeners are notified even if the list is 
modified during iteration by always making
a **copy** when an event is fired.  The old code only did so when it was 
**actually** modified during iteration.  This
can be mitigated by making the copy in the code that modifies the list (as the 
original did) using the `locked` flag to
check whether an iteration was in progress.

2. There is a significant increase in memory use.  Where before each listener 
occupied an entry in an array, now each
listener is wrapped by `Map.Entry` (the Integer instance used per entry can be 
disregarded).  I estimate around 4-8x
more heap will be consumed (the numbers are small however, still well below 1 
MB for 20.000 entries).  If this is an
issue, a further level could be introduced in the listener implementation 
hierarchy (singleton -> array -> map).

3. Even though the current version of this pull request takes care to notify 
duplicate listeners the correct amount of
times, it does not notify them in the correct order with respect to other 
listeners.  If one registers listeners (a, b,
a) the notification order will be (a, a, b).

The last point is not easily solved and could potentially cause problems.

Finally I think this solution, although it performs well is not the full 
solution.  A doubling or quadrupling of nodes
would again run into serious limitations.  I think this commit
https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd 
should not have introduced another
listener for each Node on the Window class.  A better solution would be to only 
have the Scene listen to Window and
have Scene provide a new combined status property that Node could use for its 
purposes.

Even better however would be to change the properties involved to make use of 
the hierarchy naturally present in Nodes,
having child nodes listen to their parent, and the top level node listen to the 
scene.  This would reduce the amount of
listeners on a single property in Scene and Window immensely, instead spreading 
those listeners over the Node
hierarchy, keeping listener lists much  shorter, which should scale a lot 
better.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 16 Apr 2020 08:46:26 GMT, John Hendrikx  wrote:

>> If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the 
>> ExpressionHelper.removeListener is using
>> 61% of the JavaFX thread whilst running our application.
>> [snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip)
>> 
>> If you show only the JavaFX Application thread, press the "HotSpot" and 
>> "Reverse Calls" button you can take a look to
>> see which classes are calling the removeListener function.
>> ![Screenshot 2020-04-16 at 09 16
>> 11](https://user-images.githubusercontent.com/6702882/79432046-2f788800-7fc3-11ea-930a-98fed0190628.png)
>
> @dannygonzalez  Could you perhaps debug your application and take a look at 
> how large the following array is: a random
> node -> `scene` -> `value` -> `window` -> `readOnlyProperty` -> `helper` -> 
> `changeListeners`.  I just tested this with
> a custom control displaying 200 cells on screen at once (each cell consisting 
> of about 30 nodes itself), and I saw
> about 2 change listeners registered on this single Scene Window property. 
>  However, this custom control is not
> creating/destroying cells beyond the initial allocation, so there wouldn't be 
> any registering and unregistering going
> on, scrolling was still smooth >30 fps.

@hjohn I have 12136 change listeners when debugging our application as you 
suggested.

Please note that I see the issue when the TableView is having items added to 
it. If you just have a static TableView I
do not see the issue.

It is only when you add items to the TableView which causes a myriad of 
listeners to be deregistered and registered.
The Visual VM snapshot I attached above was taken as our application was adding 
items to the TableView.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Thu, 16 Apr 2020 08:24:16 GMT, dannygonzalez 
 wrote:

>> The listeners added by `Node` are apparently internally required for 
>> internal properties `TreeShowing` and
>> `TreeVisible`, and are used to take various decisions like whether to 
>> play/pause animations.  There is also a couple of
>> listeners registering on these properties in turn (in `PopupWindow`, 
>> `SwingNode`, `WebView` and `MediaView`).  A lot of
>> the checks for visibility / showing could easily be done by using the 
>> `Scene` property and checking visibility /
>> showing status from there.  No need for so many listeners.  The other 
>> classes mentioned might register their own
>> listener, instead of having `Node` do it for them (and thus impacting 
>> *every* node).  Alternatively, `Node` may lazily
>> register the listeners for Scene.Window and Window.Showing only when needed 
>> (which from what I can see is for pretty
>> specific classes only, not classes that you'd see a lot in a TableView...)
>
> If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the 
> ExpressionHelper.removeListener is using
> 61% of the JavaFX thread whilst running our application.
> [snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip)
> 
> If you show only the JavaFX Application thread, press the "HotSpot" and 
> "Reverse Calls" button you can take a look to
> see which classes are calling the removeListener function.
> ![Screenshot 2020-04-16 at 09 16
> 11](https://user-images.githubusercontent.com/6702882/79432046-2f788800-7fc3-11ea-930a-98fed0190628.png)

@dannygonzalez  Could you perhaps debug your application and take a look at how 
large the following array is: a random
node -> `scene` -> `value` -> `window` -> `readOnlyProperty` -> `helper` -> 
`changeListeners`.  I just tested this with
a custom control displaying 200 cells on screen at once (each cell consisting 
of about 30 nodes itself), and I saw
about 2 change listeners registered on this single Scene Window property.

However, this custom control is not creating/destroying cells beyond the 
initial allocation, so there wouldn't be any
registering and unregistering going on, scrolling was still smooth >30 fps.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 16 Apr 2020 07:34:40 GMT, John Hendrikx  wrote:

>> Looking at the commit  
>> https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd
>>  it seems that the long listener lists are actually part of the `Scene`'s 
>> `Window` property and the `Window`'s `Showing`
>>  property.  Each `Node` registers itself on those and so the listener lists 
>> for those properties would scale with the
>>  number of nodes.
>> 
>> A test case showing this problem would really be great as then the patch can 
>> also be verified to solve the problem, but
>> I suppose it could be reproduced simply by having a large number of Nodes in 
>> a scene.  @dannygonzalez could you give us
>> an idea how many Nodes we're talking about?  1000? 10.000?  It also means 
>> there might be other options, do Nodes really
>> need to add these listeners and for which functionality and are there 
>> alternatives?  It would also be possible to
>> target only these specific properties with an optimized listener list to 
>> reduce the impact of this change.
>
> The listeners added by `Node` are apparently internally required for internal 
> properties `TreeShowing` and
> `TreeVisible`, and are used to take various decisions like whether to 
> play/pause animations.  There is also a couple of
> listeners registering on these properties in turn (in `PopupWindow`, 
> `SwingNode`, `WebView` and `MediaView`).  A lot of
> the checks for visibility / showing could easily be done by using the `Scene` 
> property and checking visibility /
> showing status from there.  No need for so many listeners.  The other classes 
> mentioned might register their own
> listener, instead of having `Node` do it for them (and thus impacting *every* 
> node).  Alternatively, `Node` may lazily
> register the listeners for Scene.Window and Window.Showing only when needed 
> (which from what I can see is for pretty
> specific classes only, not classes that you'd see a lot in a TableView...)

If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the 
ExpressionHelper.removeListener is using
61% of the JavaFX thread whilst running our application.

[snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip)

If you show only the JavaFX Application thread, press the "HotSpot" and 
"Reverse Calls" button you can take a look to
see which classes are calling the removeListener function.

![Screenshot 2020-04-16 at 09 16
11](https://user-images.githubusercontent.com/6702882/79432046-2f788800-7fc3-11ea-930a-98fed0190628.png)

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Thu, 16 Apr 2020 07:11:58 GMT, John Hendrikx  wrote:

>> The patch proposed here does not share the case where the listener deletion 
>> performance becomes a bottleneck.
>> 
>> I think that it is necessary to reproduce it by testing first, but
>> 
>> If you just focus on improving listener removal performance,
>> 
>> If the lifespan of a large number of registered listeners is biased,
>> It seems like the next simple change can improve delete performance without 
>> changing the data structure.
>> 
>> * Change the search from the front of the list to the search from the back.
>> 
>> This will reduce the number of long-life listeners matching.
>
> Looking at the commit  
> https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd
>  it seems that the long listener lists are actually part of the `Scene`'s 
> `Window` property and the `Window`'s `Showing`
>  property.  Each `Node` registers itself on those and so the listener lists 
> for those properties would scale with the
>  number of nodes.
> 
> A test case showing this problem would really be great as then the patch can 
> also be verified to solve the problem, but
> I suppose it could be reproduced simply by having a large number of Nodes in 
> a scene.  @dannygonzalez could you give us
> an idea how many Nodes we're talking about?  1000? 10.000?  It also means 
> there might be other options, do Nodes really
> need to add these listeners and for which functionality and are there 
> alternatives?  It would also be possible to
> target only these specific properties with an optimized listener list to 
> reduce the impact of this change.

The listeners added by `Node` are apparently internally required for internal 
properties `TreeShowing` and
`TreeVisible`, and are used to take various decisions like whether to 
play/pause animations.  There is also a couple of
listeners registering on these properties in turn (in `PopupWindow`, 
`SwingNode`, `WebView` and `MediaView`).

A lot of the checks for visibility / showing could easily be done by using the 
`Scene` property and checking visibility
/ showing status from there.  No need for so many listeners.  The other classes 
mentioned might register their own
listener, instead of having `Node` do it for them (and thus impacting *every* 
node).

Alternatively, `Node` may lazily register the listeners for Scene.Window and 
Window.Showing only when needed (which
from what I can see is for pretty specific classes only, not classes that you'd 
see a lot in a TableView...)

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Thu, 16 Apr 2020 06:34:39 GMT, yosbits 
 wrote:

>> @hjohn
>> I haven't quantified exactly where all the listeners are coming from but the 
>> Node class for example has various
>> listeners on it.
>> The following changeset  (which added an extra listener on the Node class) 
>> impacted TableView performance significantly
>> (although it was still bad before this change in my previous tests):
>>> commit e21606d3a1b73cd4b44383babc750a4b4721edfd
>>> Author: Florian Kirmaier mailto:fk at sandec.de>>
>>> Date:   Tue Jan 22 09:08:36 2019 -0800
>>> 
>>> 8216377: Fix memoryleak for initial nodes of Window
>>> 8207837: Indeterminate ProgressBar does not animate if content is added 
>>> after scene is set on window
>>> 
>>> Add or remove the windowShowingChangedListener when the scene changes
>> 
>> As you can imagine, a TableView with many columns and rows can be made up of 
>> many Node classes. The impact of having
>> multiple listeners deregistering on the Node when new rows are being added 
>> to a TableView can be a significant
>> performance hit on the JavaFX thread.   I don't have the time or knowledge 
>> to investigate why these listeners are all
>> needed especially around the Node class. I went directly to the source of 
>> the problem which was the linear search to
>> deregister each listener.  I have been running my proposed fix in our JavaFX 
>> application for the past few months and it
>> has saved it from being unusable due to the JavaFx thread being swamped.
>
> The patch proposed here does not share the case where the listener deletion 
> performance becomes a bottleneck.
> 
> I think that it is necessary to reproduce it by testing first, but
> 
> If you just focus on improving listener removal performance,
> 
> If the lifespan of a large number of registered listeners is biased,
> It seems like the next simple change can improve delete performance without 
> changing the data structure.
> 
> * Change the search from the front of the list to the search from the back.
> 
> This will reduce the number of long-life listeners matching.

Looking at the commit  
https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd
 it seems that the long listener lists are actually part of the `Scene`'s 
`Window` property and the `Window`'s `Showing`
 property.  Each `Node` registers itself on those and so the listener lists for 
those properties would scale with the
 number of nodes.

A test case showing this problem would really be great as then the patch can 
also be verified to solve the problem, but
I suppose it could be reproduced simply by having a large number of Nodes in a 
scene.  @dannygonzalez could you give us
an idea how many Nodes we're talking about?  1000? 10.000?

It also means there might be other options, do Nodes really need to add these 
listeners and for which functionality and
are there alternatives?  It would also be possible to target only these 
specific properties with an optimized listener
list to reduce the impact of this change.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[yosbits]

On Wed, 15 Apr 2020 07:11:27 GMT, dannygonzalez 
 wrote:

>> @dannygonzalez You mentioned "There are multiple change listeners on a Node 
>> for example, so you can imagine with the
>> number of nodes in a TableView this is going to be a problem if the 
>> unregistering is slow.".
>> Your changes in this PR seem to be focused on improving performance of 
>> unregistering listeners when there are thousands
>> of listeners listening on changes or invalidations of the **same** property. 
>>  Is this actually the case?  Is there a
>> single property in TableView or TreeTableView where such a high amount of 
>> listeners are present?  If so, I think the
>> problem should be solved there.  As it is, I donot think this PR will help 
>> unregistration performance of listeners when
>> the listeners are  spread out over dozens of properties, and although high 
>> in total number, the total listeners per
>> property would be low and their listener lists would be short.  Performance 
>> of short lists (<50 entries) is almost
>> unbeatable, so it is relevant for this PR to know if there are any 
>> properties with long listener lists.
>
> @hjohn
> I haven't quantified exactly where all the listeners are coming from but the 
> Node class for example has various
> listeners on it.
> The following changeset  (which added an extra listener on the Node class) 
> impacted TableView performance significantly
> (although it was still bad before this change in my previous tests):
>> commit e21606d3a1b73cd4b44383babc750a4b4721edfd
>> Author: Florian Kirmaier mailto:fk at sandec.de>>
>> Date:   Tue Jan 22 09:08:36 2019 -0800
>> 
>> 8216377: Fix memoryleak for initial nodes of Window
>> 8207837: Indeterminate ProgressBar does not animate if content is added 
>> after scene is set on window
>> 
>> Add or remove the windowShowingChangedListener when the scene changes
> 
> As you can imagine, a TableView with many columns and rows can be made up of 
> many Node classes. The impact of having
> multiple listeners deregistering on the Node when new rows are being added to 
> a TableView can be a significant
> performance hit on the JavaFX thread.   I don't have the time or knowledge to 
> investigate why these listeners are all
> needed especially around the Node class. I went directly to the source of the 
> problem which was the linear search to
> deregister each listener.  I have been running my proposed fix in our JavaFX 
> application for the past few months and it
> has saved it from being unusable due to the JavaFx thread being swamped.

If the lifespan of a large number of registered listeners is biased,
It seems like the next simple change can improve delete performance without 
changing the data structure.

* Change the search from the front of the list to the search from the back.

This will reduce the number of long-life listeners matching.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 14 Apr 2020 12:20:10 GMT, John Hendrikx  wrote:

>> In a minimal test I wrote (not a microbenchmark that removes listeners), I 
>> tried this PR code, but did not reproduce
>> the performance improvement. I have attached a test program in my PR(#125)
>
> @dannygonzalez You mentioned "There are multiple change listeners on a Node 
> for example, so you can imagine with the
> number of nodes in a TableView this is going to be a problem if the 
> unregistering is slow.".
> Your changes in this PR seem to be focused on improving performance of 
> unregistering listeners when there are thousands
> of listeners listening on changes or invalidations of the **same** property.  
> Is this actually the case?  Is there a
> single property in TableView or TreeTableView where such a high amount of 
> listeners are present?  If so, I think the
> problem should be solved there.  As it is, I donot think this PR will help 
> unregistration performance of listeners when
> the listeners are  spread out over dozens of properties, and although high in 
> total number, the total listeners per
> property would be low and their listener lists would be short.  Performance 
> of short lists (<50 entries) is almost
> unbeatable, so it is relevant for this PR to know if there are any properties 
> with long listener lists.

@hjohn
I haven't quantified exactly where all the listeners are coming from but the 
Node class for example has various
listeners on it.

The following changeset  (which added an extra listener on the Node class) 
impacted TableView performance significantly
(although it was still bad before this change in my previous tests):

> commit e21606d3a1b73cd4b44383babc750a4b4721edfd
> Author: Florian Kirmaier mailto:fk at sandec.de>>
> Date:   Tue Jan 22 09:08:36 2019 -0800
> 
> 8216377: Fix memoryleak for initial nodes of Window
> 8207837: Indeterminate ProgressBar does not animate if content is added 
> after scene is set on window
> 
> Add or remove the windowShowingChangedListener when the scene changes

As you can imagine, a TableView with many columns and rows can be made up of 
many Node classes. The impact of having
multiple listeners deregistering on the Node when new rows are being added to a 
TableView can be a significant
performance hit on the JavaFX thread.

I don't have the time or knowledge to investigate why these listeners are all 
needed especially around the Node class.
I went directly to the source of the problem which was the linear search to 
deregister each listener.

I have been running my proposed fix in our JavaFX application for the past few 
months and it has saved it from being
unusable due to the JavaFx thread being swamped.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

On Tue, 10 Mar 2020 06:08:25 GMT, yosbits 
 wrote:

>>> technically true - but the implementation was linear with a fixed sequence 
>>> since-the-beginning-of-java-desktop-time
>>> (and sometimes, for good ol' beans properties, even exposed as api to 
>>> access the array of listeners). So technically,
>>> we could go the path of explicitely spec'ing that the order is unspecified. 
>>> Pretty sure that doing so and implementing
>>> it will break tons of application code that's subtly relying on fifo 
>>> notification (f.i. register before or after skin
>>> has its own is a simple wide-spread trick) .. which all did it wrong and 
>>> might deserve it ;-) But then if even internal
>>> code does it ..
>> 
>> So we can choose to specify it as ordered.
>> 
>> These are the 2 options I mentioned:
>> 1. Not specify the behavior and change the implementation in favor of 
>> performance. This could break applications as
>> you've mentioned. 2. Specify that the order is preserved and keep the 
>> ordered implementation behavior. This will allow
>> applications to rely on the behavior safely.
>> Right now we're losing on both sides. We keep a promise and we don't tell 
>> anyone about it. The only reason for this is
>> if we intend to change the behavior in the future, in which case we should 
>> add a doc comment saying that the order is
>> unspecified and is planned to change in the future, so there will be at 
>> least some warning.  Once we choose what to do
>> here it will make sense to continue to review the code with that decision in 
>> mind.
>
> In a minimal test I wrote (not a microbenchmark that removes listeners), I 
> tried this PR code, but did not reproduce
> the performance improvement. I have attached a test program in my PR(#125)

@dannygonzalez You mentioned "There are multiple change listeners on a Node for 
example, so you can imagine with the
number of nodes in a TableView this is going to be a problem if the 
unregistering is slow.".

Your changes in this PR seem to be focused on improving performance of 
unregistering listeners when there are thousands
of listeners listening on changes or invalidations of the **same** property.  
Is this actually the case?  Is there a
single property in TableView or TreeTableView where such a high amount of 
listeners are present?  If so, I think the
problem should be solved there.

As it is, I donot think this PR will help unregistration performance of 
listeners when the listeners are  spread out
over dozens of properties, and although high in total number, the total 
listeners per property would be low and their
listener lists would be short.  Performance of short lists (<50 entries) is 
almost unbeatable, so it is relevant for
this PR to know if there are any properties with long listener lists.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Dalibor Topic]

On Mon, 9 Mar 2020 17:00:06 GMT, Nir Lisker  wrote:

>> I took the liberty of pointing out some formatting errors
>
> The title of your PR should match the title of the JBS issue. Moreover, #108 
> already tackles this issue with a
> different approach and 2 PRs on the same issue can't be intetgrated. Since 
> I'm not familiar with this code, I can't
> advise on who's solution is more suitable. I suggest you discuss both of the 
> approaches on the mailing list. I saw many
> JBS issues around this performance issue, probably one of them fits (or a new 
> one can be created).

Hi,
I couldn't find you listed at 
https://www.oracle.com/technetwork/community/oca-486395.html . Please send me 
an e-mail
at dalibor.to...@oracle.com with information about your OCA, so that I can look 
it up.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Martin Polakovic]

On Mon, 24 Feb 2020 07:39:43 GMT, yosbits 
 wrote:

> If there are many columns, the current TableView will stall scrolling. 
> Resolving this performance issue requires column
> virtualization. Virtualization mode is enabled when the row height is fixed 
> by the following method.
> `tableView.setFixedCellSize(height)`
> 
> This proposal includes a fix because the current code does not correctly 
> implement column virtualization.
> 
> The improvement of this proposal can be seen in the following test program.
> 
>  Java
> import javafx.animation.AnimationTimer;
> import javafx.application.Application;
> import javafx.beans.property.SimpleStringProperty;
> import javafx.collections.ObservableList;
> import javafx.scene.Scene;
> import javafx.scene.control.TableColumn;
> import javafx.scene.control.TableView;
> import javafx.scene.layout.BorderPane;
> import javafx.stage.Stage;
> 
> public class BigTableViewTest2 extends Application {
> 
>   private static final boolean USE_WIDTH_FIXED_SIZE = true;
>   private static final boolean USE_HEIGHT_FIXED_SIZE = true;
> //private static final int COL_COUNT=300;
>   private static final int COL_COUNT=600;
> //private static final int COL_COUNT=1000;
>   private static final int ROW_COUNT=1000;
> 
>   @Override
>   public void start(Stage primaryStage) throws Exception {
>   final TableView tableView = new TableView<>();
> 
>   final ObservableList> columns = 
> tableView.getColumns();
>   for(int i=0; i   TableColumn column = new 
> TableColumn<>("Col"+i);
>   final int colIndex=i;
>   column.setCellValueFactory((cell)->new 
> SimpleStringProperty(cell.getValue()[colIndex]));
>   columns.add(column);
>   if(USE_WIDTH_FIXED_SIZE) {
>   column.setPrefWidth(60);
>   column.setMaxWidth(60);
>   column.setMinWidth(60);
>   }
>   }
> 
>   ObservableList items = tableView.getItems();
>   for(int i=0; i   String[] rec = new String[COL_COUNT];
>   for(int j=0; j   rec[j] = i+":"+j;
>   }
>   items.add(rec);
>   }
>   BorderPane root = new BorderPane(tableView);
>   if(USE_HEIGHT_FIXED_SIZE) {
>   tableView.setFixedCellSize(24);
>   }
> 
>   Scene scene = new Scene(root, 800, 800);
>   primaryStage.setScene(scene);
>   primaryStage.show();
>   prepareTimeline(scene);
> 
>   }
> 
>   public static void main(String[] args) {
>   Application.launch(args);
>   }
> 
> private void prepareTimeline(Scene scene) {
> new AnimationTimer() {
> @Override public void handle(long now) {
> double fps =  
> com.sun.javafx.perf.PerformanceTracker.getSceneTracker(scene).getInstantFPS();
> ((Stage)scene.getWindow()).setTitle("FPS:"+(int)fps);
> }
> }.start();
> }
> 
> }

I took the liberty of pointing out some formatting errors

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
 line 492:

> 491: tableCell.requestLayout();
> 492: }else if(fixedCellSizeEnabled && 
> lastVisibleColumnIndex 493: break;

`}else` formatting

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
 line 387:

> 386: }
> 387: }else{
> 388: // we only add/remove to the scenegraph if the fixed 
> cell

`}else` formatting

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
 line 373:

> 372: lastVisibleColumnIndex = column;
> 373: }else if( firstVisibleColumnIndex != -1 ) {
> 374: break;

`}else` formatting

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
 line 347:

> 346: final Insets padding = getSkinnable().getPadding();
> 347: final double vfWidth = virtualFlow == null ? 
> 0.0:virtualFlow.getWidth();
> 348: final double headerWidth = vfWidth - (padding.getLeft() + 
> padding.getRight());

`0.0:` formatting

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
 line 414:

> 413: width = snapSizeX(tableColumn.getWidth());
> 414: if (isVisible || isOverlap(tableCell.getLayoutX(), 
> tableCell.getLayoutX()+width, scrollX, headerWidth
> + scrollX)) { 415: // if the style origin is null then the 
> property has not been

`)+w` formatting

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[yosbits]

On Thu, 2 Apr 2020 17:38:00 GMT, Dalibor Topic  wrote:

>> The title of your PR should match the title of the JBS issue. Moreover, #108 
>> already tackles this issue with a
>> different approach and 2 PRs on the same issue can't be intetgrated. Since 
>> I'm not familiar with this code, I can't
>> advise on who's solution is more suitable. I suggest you discuss both of the 
>> approaches on the mailing list. I saw many
>> JBS issues around this performance issue, probably one of them fits (or a 
>> new one can be created).
>
> Hi,
> I couldn't find you listed at 
> https://www.oracle.com/technetwork/community/oca-486395.html . Please send me 
> an e-mail
> at dalibor.to...@oracle.com with information about your OCA, so that I can 
> look it up.

My name is "Naohiro Yoshimoto"
I have received an approved email on 2019-8-3.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Thu, 27 Feb 2020 21:29:27 GMT, Martin Polakovic 
 wrote:

>> If there are many columns, the current TableView will stall scrolling. 
>> Resolving this performance issue requires column
>> virtualization. Virtualization mode is enabled when the row height is fixed 
>> by the following method.
>> `tableView.setFixedCellSize(height)`
>> 
>> This proposal includes a fix because the current code does not correctly 
>> implement column virtualization.
>> 
>> The improvement of this proposal can be seen in the following test program.
>> 
>>  Java
>> import javafx.animation.AnimationTimer;
>> import javafx.application.Application;
>> import javafx.beans.property.SimpleStringProperty;
>> import javafx.collections.ObservableList;
>> import javafx.scene.Scene;
>> import javafx.scene.control.TableColumn;
>> import javafx.scene.control.TableView;
>> import javafx.scene.layout.BorderPane;
>> import javafx.stage.Stage;
>> 
>> public class BigTableViewTest2 extends Application {
>> 
>>  private static final boolean USE_WIDTH_FIXED_SIZE = true;
>>  private static final boolean USE_HEIGHT_FIXED_SIZE = true;
>> //   private static final int COL_COUNT=300;
>>  private static final int COL_COUNT=600;
>> //   private static final int COL_COUNT=1000;
>>  private static final int ROW_COUNT=1000;
>> 
>>  @Override
>>  public void start(Stage primaryStage) throws Exception {
>>  final TableView tableView = new TableView<>();
>> 
>>  final ObservableList> columns = 
>> tableView.getColumns();
>>  for(int i=0; i>  TableColumn column = new 
>> TableColumn<>("Col"+i);
>>  final int colIndex=i;
>>  column.setCellValueFactory((cell)->new 
>> SimpleStringProperty(cell.getValue()[colIndex]));
>>  columns.add(column);
>>  if(USE_WIDTH_FIXED_SIZE) {
>>  column.setPrefWidth(60);
>>  column.setMaxWidth(60);
>>  column.setMinWidth(60);
>>  }
>>  }
>> 
>>  ObservableList items = tableView.getItems();
>>  for(int i=0; i>  String[] rec = new String[COL_COUNT];
>>  for(int j=0; j>  rec[j] = i+":"+j;
>>  }
>>  items.add(rec);
>>  }
>>  BorderPane root = new BorderPane(tableView);
>>  if(USE_HEIGHT_FIXED_SIZE) {
>>  tableView.setFixedCellSize(24);
>>  }
>> 
>>  Scene scene = new Scene(root, 800, 800);
>>  primaryStage.setScene(scene);
>>  primaryStage.show();
>>  prepareTimeline(scene);
>> 
>>  }
>> 
>>  public static void main(String[] args) {
>>  Application.launch(args);
>>  }
>> 
>> private void prepareTimeline(Scene scene) {
>> new AnimationTimer() {
>> @Override public void handle(long now) {
>> double fps =  
>> com.sun.javafx.perf.PerformanceTracker.getSceneTracker(scene).getInstantFPS();
>> ((Stage)scene.getWindow()).setTitle("FPS:"+(int)fps);
>> }
>> }.start();
>> }
>> 
>> }
>
> I took the liberty of pointing out some formatting errors

The title of your PR should match the title of the JBS issue. Moreover, #108 
already tackles this issue with a
different approach and 2 PRs on the same issue can't be intetgrated. Since I'm 
not familiar with this code, I can't
advise on who's solution is more suitable. I suggest you discuss both of the 
approaches on the mailing list. I saw many
JBS issues around this performance issue, probably one of them fits (or a new 
one can be created).

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[yosbits]

On Mon, 24 Feb 2020 07:39:43 GMT, yosbits 
 wrote:

> If there are many columns, the current TableView will stall scrolling. 
> Resolving this performance issue requires column
> virtualization. Virtualization mode is enabled when the row height is fixed 
> by the following method.
> `tableView.setFixedCellSize(height)`
> 
> This proposal includes a fix because the current code does not correctly 
> implement column virtualization.
> 
> The improvement of this proposal can be seen in the following test program.
> 
>  Java
> import javafx.animation.AnimationTimer;
> import javafx.application.Application;
> import javafx.beans.property.SimpleStringProperty;
> import javafx.collections.ObservableList;
> import javafx.scene.Scene;
> import javafx.scene.control.TableColumn;
> import javafx.scene.control.TableView;
> import javafx.scene.layout.BorderPane;
> import javafx.stage.Stage;
> 
> public class BigTableViewTest2 extends Application {
> 
>   private static final boolean USE_WIDTH_FIXED_SIZE = true;
>   private static final boolean USE_HEIGHT_FIXED_SIZE = true;
> //private static final int COL_COUNT=300;
>   private static final int COL_COUNT=600;
> //private static final int COL_COUNT=1000;
>   private static final int ROW_COUNT=1000;
> 
>   @Override
>   public void start(Stage primaryStage) throws Exception {
>   final TableView tableView = new TableView<>();
> 
>   final ObservableList> columns = 
> tableView.getColumns();
>   for(int i=0; i   TableColumn column = new 
> TableColumn<>("Col"+i);
>   final int colIndex=i;
>   column.setCellValueFactory((cell)->new 
> SimpleStringProperty(cell.getValue()[colIndex]));
>   columns.add(column);
>   if(USE_WIDTH_FIXED_SIZE) {
>   column.setPrefWidth(60);
>   column.setMaxWidth(60);
>   column.setMinWidth(60);
>   }
>   }
> 
>   ObservableList items = tableView.getItems();
>   for(int i=0; i   String[] rec = new String[COL_COUNT];
>   for(int j=0; j   rec[j] = i+":"+j;
>   }
>   items.add(rec);
>   }
>   BorderPane root = new BorderPane(tableView);
>   if(USE_HEIGHT_FIXED_SIZE) {
>   tableView.setFixedCellSize(24);
>   }
> 
>   Scene scene = new Scene(root, 800, 800);
>   primaryStage.setScene(scene);
>   primaryStage.show();
>   prepareTimeline(scene);
> 
>   }
> 
>   public static void main(String[] args) {
>   Application.launch(args);
>   }
> 
> private void prepareTimeline(Scene scene) {
> new AnimationTimer() {
> @Override public void handle(long now) {
> double fps =  
> com.sun.javafx.perf.PerformanceTracker.getSceneTracker(scene).getInstantFPS();
> ((Stage)scene.getWindow()).setTitle("FPS:"+(int)fps);
> }
> }.start();
> }
> 
> }

I'm signed to OCA and already a contributor.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[yosbits]

On Mon, 2 Mar 2020 11:47:03 GMT, Nir Lisker  wrote:

>>> 
>>> 
>>> I think that a starting point would be to decide on a spec for the listener 
>>> notification order: is it specified by
>>> their registration order, or that is it unspecified, in which case we can 
>>> take advantage of this for better
>>> performance. Leaving is unspecified and restricting ourselves to having it 
>>> ordered is losing on both sides.
>> 
>> technically true - but the implementation was linear with a fixed sequence 
>> since-the-beginning-of-java-desktop-time
>> (and sometimes, for good ol' beans properties, even exposed as api to access 
>> the array of listeners). So technically,
>> we could go the path of explicitely spec'ing that the order is unspecified. 
>> Pretty sure that doing so and implementing
>> it will break tons of application code that's subtly relying on fifo 
>> notification (f.i. register before or after skin
>> has its own is a simple wide-spread trick) .. which all did it wrong and 
>> might deserve it ;-) But then if even internal
>> code does it ..
>
>> technically true - but the implementation was linear with a fixed sequence 
>> since-the-beginning-of-java-desktop-time
>> (and sometimes, for good ol' beans properties, even exposed as api to access 
>> the array of listeners). So technically,
>> we could go the path of explicitely spec'ing that the order is unspecified. 
>> Pretty sure that doing so and implementing
>> it will break tons of application code that's subtly relying on fifo 
>> notification (f.i. register before or after skin
>> has its own is a simple wide-spread trick) .. which all did it wrong and 
>> might deserve it ;-) But then if even internal
>> code does it ..
> 
> So we can choose to specify it as ordered.
> 
> These are the 2 options I mentioned:
> 1. Not specify the behavior and change the implementation in favor of 
> performance. This could break applications as
> you've mentioned. 2. Specify that the order is preserved and keep the ordered 
> implementation behavior. This will allow
> applications to rely on the behavior safely.
> Right now we're losing on both sides. We keep a promise and we don't tell 
> anyone about it. The only reason for this is
> if we intend to change the behavior in the future, in which case we should 
> add a doc comment saying that the order is
> unspecified and is planned to change in the future, so there will be at least 
> some warning.  Once we choose what to do
> here it will make sense to continue to review the code with that decision in 
> mind.

In a minimal test I wrote (not a microbenchmark that removes listeners), I 
tried this PR code, but did not reproduce
the performance improvement. I have attached a test program in my PR(#125)

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Mon, 2 Mar 2020 08:31:47 GMT, dannygonzalez 
 wrote:

>> In use cases where a large number of listeners are being discarded, it may 
>> be better to first consider changing the design to receive event 
>> notifications on nodes whose listener registrations are not frequently 
>> discarded.
>
>> Hmm .. personally, I would consider changing such a basic (and probably 
>> highly optimized) implementation used all over the framework is a very high 
>> risk change that at the worst outcome would detoriate internal and external 
>> code. So before going that lane, I would try - as you probably have, just me 
>> not seeing it :) - to tackle the problem from the other end:
>> 
>> > I know that in our application, the **thousands of listeners** 
>> > unregistering when using a TableView was stalling the JavaFX thread.
>> 
>> .. sounds like a lot. Any idea, where exactly they come into play?
> 
> I did start to look at why there were so many change listeners unregistering 
> but felt that would take a deeper understanding of the architecture and 
> design decisions of JavaFX scene graph and I didn't have that time to invest. 
> I do know that there are thousands of them unregistering in a TableView and 
> unregistering them is a bottleneck for the JavaFX thread.
> 
> There are multiple change listeners on a Node for example, so you can imagine 
> with the number of nodes in a TableView this is going to be a problem if the 
> unregistering is slow.
> 
> To get our application usable I profiled the code and saw this unregistering 
> as a major bottleneck, hence why I looked at this more obvious solution.
> 
> I'm happy to look at the problem from the other angle and happy to listen to 
> suggestions from people with more experience of the design and architecture 
> but tackling the problem from the perspective of re-architecting the 
> behaviour of listeners in the scene graph would be more work than I could 
> feasibly take on.

> technically true - but the implementation was linear with a fixed sequence 
> since-the-beginning-of-java-desktop-time (and sometimes, for good ol' beans 
> properties, even exposed as api to access the array of listeners). So 
> technically, we could go the path of explicitely spec'ing that the order is 
> unspecified. Pretty sure that doing so and implementing it will break tons of 
> application code that's subtly relying on fifo notification (f.i. register 
> before or after skin has its own is a simple wide-spread trick) .. which all 
> did it wrong and might deserve it ;-) But then if even internal code does it 
> ..

So we can choose to specify it as ordered.

These are the 2 options I mentioned:
1. Not specify the behavior and change the implementation in favor of 
performance. This could break applications as you've mentioned.
2. Specify that the order is preserved and keep the ordered implementation 
behavior. This will allow applications to rely on the behavior safely.

Right now we're losing on both sides. We keep a promise and we don't tell 
anyone about it. The only reason for this is if we intend to change the 
behavior in the future, in which case we should add a doc comment saying that 
the order is unspecified and is planned to change in the future, so there will 
be at least some warning.

Once we choose what to do here it will make sense to continue to review the 
code with that decision in mind.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 27 Feb 2020 03:17:14 GMT, yosbits 
 wrote:

>>> 
>>> 
>>> I think that a starting point would be to decide on a spec for the listener 
>>> notification order: is it specified by their registration order, or that is 
>>> it unspecified, in which case we can take advantage of this for better 
>>> performance. Leaving is unspecified and restricting ourselves to having it 
>>> ordered is losing on both sides.
>> 
>> technically true - but the implementation was linear with a fixed sequence 
>> since-the-beginning-of-java-desktop-time (and sometimes, for good ol' beans 
>> properties, even exposed as api to access the array of listeners). So 
>> technically, we could go the path of explicitely spec'ing that the order is 
>> unspecified. Pretty sure that doing so and implementing it will break tons 
>> of application code that's subtly relying on fifo notification (f.i. 
>> register before or after skin has its own is a simple wide-spread trick) .. 
>> which all did it wrong and might deserve it ;-) But then if even internal 
>> code does it ..
>
> In use cases where a large number of listeners are being discarded, it may be 
> better to first consider changing the design to receive event notifications 
> on nodes whose listener registrations are not frequently discarded.

> Hmm .. personally, I would consider changing such a basic (and probably 
> highly optimized) implementation used all over the framework is a very high 
> risk change that at the worst outcome would detoriate internal and external 
> code. So before going that lane, I would try - as you probably have, just me 
> not seeing it :) - to tackle the problem from the other end:
> 
> > I know that in our application, the **thousands of listeners** 
> > unregistering when using a TableView was stalling the JavaFX thread.
> 
> .. sounds like a lot. Any idea, where exactly they come into play?

I did start to look at why there were so many change listeners unregistering 
but felt that would take a deeper understanding of the architecture and design 
decisions of JavaFX scene graph and I didn't have that time to invest. 
I do know that there are thousands of them unregistering in a TableView and 
unregistering them is a bottleneck for the JavaFX thread.

There are multiple change listeners on a Node for example, so you can imagine 
with the number of nodes in a TableView this is going to be a problem if the 
unregistering is slow.

To get our application usable I profiled the code and saw this unregistering as 
a major bottleneck, hence why I looked at this more obvious solution.

I'm happy to look at the problem from the other angle and happy to listen to 
suggestions from people with more experience of the design and architecture but 
tackling the problem from the perspective of re-architecting the behaviour of 
listeners in the scene graph would be more work than I could feasibly take on.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[yosbits]

On Wed, 26 Feb 2020 10:07:02 GMT, Jeanette Winzenburg  
wrote:

>> Hmm .. personally, I would consider changing such a basic (and probably 
>> highly optimized) implementation used all over the framework is a very high 
>> risk change that at the worst outcome would detoriate internal and external 
>> code. So before going that lane, I would try - as you probably have, just me 
>> not seeing it :) - to tackle the problem from the other end:
>> 
>>> I know that in our application, the **thousands of listeners** 
>>> unregistering when using a TableView was stalling the JavaFX thread.
>>> 
>> 
>> .. sounds like a lot. Any idea, where exactly they come into play?
> 
>> 
>> 
>> I think that a starting point would be to decide on a spec for the listener 
>> notification order: is it specified by their registration order, or that is 
>> it unspecified, in which case we can take advantage of this for better 
>> performance. Leaving is unspecified and restricting ourselves to having it 
>> ordered is losing on both sides.
> 
> technically true - but the implementation was linear with a fixed sequence 
> since-the-beginning-of-java-desktop-time (and sometimes, for good ol' beans 
> properties, even exposed as api to access the array of listeners). So 
> technically, we could go the path of explicitely spec'ing that the order is 
> unspecified. Pretty sure that doing so and implementing it will break tons of 
> application code that's subtly relying on fifo notification (f.i. register 
> before or after skin has its own is a simple wide-spread trick) .. which all 
> did it wrong and might deserve it ;-) But then if even internal code does it 
> ..

In use cases where a large number of listeners are being discarded, it may be 
better to first consider changing the design to receive event notifications on 
nodes whose listener registrations are not frequently discarded.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Jeanette Winzenburg]

On Wed, 26 Feb 2020 09:49:04 GMT, Jeanette Winzenburg  
wrote:

>> Even though the order of notifications is unspecified, the following tests 
>> fail if we use a HashMap vs LinkedHashMap i.e. the notifications are not in 
>> order of registration:
>> 
>> test.javafx.scene.control.TableViewTest > 
>> ensureSelectionIsCorrectWhenItemsChange FAILED
>> java.lang.AssertionError: expected:<0> but was:<-1>
>> at org.junit.Assert.fail(Assert.java:91)
>> at org.junit.Assert.failNotEquals(Assert.java:645)
>> at org.junit.Assert.assertEquals(Assert.java:126)
>> at org.junit.Assert.assertEquals(Assert.java:470)
>> at org.junit.Assert.assertEquals(Assert.java:454)
>> at 
>> test.javafx.scene.control.TableViewTest.ensureSelectionIsCorrectWhenItemsChange(TableViewTest.java:333)
>> 
>> test.javafx.scene.control.TreeTableViewTest > 
>> test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
>> java.lang.AssertionError: 
>> at org.junit.Assert.fail(Assert.java:91)
>> at org.junit.Assert.assertTrue(Assert.java:43)
>> at org.junit.Assert.assertTrue(Assert.java:54)
>> at 
>> test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeTableViewTest.java:2042)
>> 
>> test.javafx.scene.control.TreeViewTest > test_rt27185 FAILED
>> java.lang.AssertionError: expected: but 
>> was:
>> at org.junit.Assert.fail(Assert.java:91)
>> at org.junit.Assert.failNotEquals(Assert.java:645)
>> at org.junit.Assert.assertEquals(Assert.java:126)
>> at org.junit.Assert.assertEquals(Assert.java:145)
>> at 
>> test.javafx.scene.control.TreeViewTest.test_rt27185(TreeViewTest.java:603)
>> 
>> test.javafx.scene.control.TreeViewTest > 
>> test_rt27180_collapseBranch_laterSiblingAndChildrenSelected FAILED
>> java.lang.AssertionError: 
>> at org.junit.Assert.fail(Assert.java:91)
>> at org.junit.Assert.assertTrue(Assert.java:43)
>> at org.junit.Assert.assertTrue(Assert.java:54)
>> at 
>> test.javafx.scene.control.TreeViewTest.test_rt27180_collapseBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:973)
>> 
>> test.javafx.scene.control.TreeViewTest > 
>> test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
>> java.lang.AssertionError: 
>> at org.junit.Assert.fail(Assert.java:91)
>> at org.junit.Assert.assertTrue(Assert.java:43)
>> at org.junit.Assert.assertTrue(Assert.java:54)
>> at 
>> test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeViewTest.java:992)
>> 
>> 
>> These would need to be investigated to see why they assume this order.
> 
> Hmm .. personally, I would consider changing such a basic (and probably 
> highly optimized) implementation used all over the framework is a very high 
> risk change that at the worst outcome would detoriate internal and external 
> code. So before going that lane, I would try - as you probably have, just me 
> not seeing it :) - to tackle the problem from the other end:
> 
>> I know that in our application, the **thousands of listeners** unregistering 
>> when using a TableView was stalling the JavaFX thread.
>> 
> 
> .. sounds like a lot. Any idea, where exactly they come into play?

> 
> 
> I think that a starting point would be to decide on a spec for the listener 
> notification order: is it specified by their registration order, or that is 
> it unspecified, in which case we can take advantage of this for better 
> performance. Leaving is unspecified and restricting ourselves to having it 
> ordered is losing on both sides.

technically true - but the implementation was linear with a fixed sequence 
since-the-beginning-of-java-desktop-time (and sometimes, for good ol' beans 
properties, even exposed as api to access the array of listeners). So 
technically, we could go the path of explicitely spec'ing that the order is 
unspecified. Pretty sure that doing so and implementing it will break tons of 
application code that's subtly relying on fifo notification (f.i. register 
before or after skin has its own is a simple wide-spread trick) .. which all 
did it wrong and might deserve it ;-) But then if even internal code does it ..

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Jeanette Winzenburg]

On Tue, 25 Feb 2020 14:06:23 GMT, dannygonzalez 
 wrote:

>> I think that a starting point would be to decide on a spec for the listener 
>> notification order: is it specified by their registration order, or that is 
>> it unspecified, in which case we can take advantage of this for better 
>> performance. Leaving is unspecified and restricting ourselves to having it 
>> ordered is losing on both sides.
> 
> Even though the order of notifications is unspecified, the following tests 
> fail if we use a HashMap vs LinkedHashMap i.e. the notifications are not in 
> order of registration:
> 
> test.javafx.scene.control.TableViewTest > 
> ensureSelectionIsCorrectWhenItemsChange FAILED
> java.lang.AssertionError: expected:<0> but was:<-1>
> at org.junit.Assert.fail(Assert.java:91)
> at org.junit.Assert.failNotEquals(Assert.java:645)
> at org.junit.Assert.assertEquals(Assert.java:126)
> at org.junit.Assert.assertEquals(Assert.java:470)
> at org.junit.Assert.assertEquals(Assert.java:454)
> at 
> test.javafx.scene.control.TableViewTest.ensureSelectionIsCorrectWhenItemsChange(TableViewTest.java:333)
> 
> test.javafx.scene.control.TreeTableViewTest > 
> test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
> java.lang.AssertionError: 
> at org.junit.Assert.fail(Assert.java:91)
> at org.junit.Assert.assertTrue(Assert.java:43)
> at org.junit.Assert.assertTrue(Assert.java:54)
> at 
> test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeTableViewTest.java:2042)
> 
> test.javafx.scene.control.TreeViewTest > test_rt27185 FAILED
> java.lang.AssertionError: expected: but 
> was:
> at org.junit.Assert.fail(Assert.java:91)
> at org.junit.Assert.failNotEquals(Assert.java:645)
> at org.junit.Assert.assertEquals(Assert.java:126)
> at org.junit.Assert.assertEquals(Assert.java:145)
> at 
> test.javafx.scene.control.TreeViewTest.test_rt27185(TreeViewTest.java:603)
> 
> test.javafx.scene.control.TreeViewTest > 
> test_rt27180_collapseBranch_laterSiblingAndChildrenSelected FAILED
> java.lang.AssertionError: 
> at org.junit.Assert.fail(Assert.java:91)
> at org.junit.Assert.assertTrue(Assert.java:43)
> at org.junit.Assert.assertTrue(Assert.java:54)
> at 
> test.javafx.scene.control.TreeViewTest.test_rt27180_collapseBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:973)
> 
> test.javafx.scene.control.TreeViewTest > 
> test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
> java.lang.AssertionError: 
> at org.junit.Assert.fail(Assert.java:91)
> at org.junit.Assert.assertTrue(Assert.java:43)
> at org.junit.Assert.assertTrue(Assert.java:54)
> at 
> test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeViewTest.java:992)
> 
> 
> These would need to be investigated to see why they assume this order.

Hmm .. personally, I would consider changing such a basic (and probably highly 
optimized) implementation used all over the framework is a very high risk 
change that at the worst outcome would detoriate internal and external code. So 
before going that lane, I would try - as you probably have, just me not seeing 
it :) - to tackle the problem from the other end:

> I know that in our application, the **thousands of listeners** unregistering 
> when using a TableView was stalling the JavaFX thread.
> 

.. sounds like a lot. Any idea, where exactly they come into play?

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 13:55:50 GMT, Nir Lisker  wrote:

>> Replying to @nlisker and @kevinrushforth general comments about the memory 
>> consumption of using a LinkedHashMap:
>> 
>> I agree that at the very least these should be lazy initialised when they 
>> are needed and that we should initially allocate a small capacity and load 
>> factor.
>> 
>> We could also have some sort of strategy where we could use arrays or lists 
>> if the number of listeners are less than some threshold (i.e. keeping the 
>> implementation with a similar overhead to the original) and then switch to 
>> the HashMap when it exceeds this threshold. This would make the code more 
>> complicated and I wonder if this is the worth the extra complexity.
>>  
>> I know that in our application, the thousands of listeners unregistering 
>> when using a TableView was stalling the JavaFX thread. 
>> 
>> I am happy to submit code that lazy initialises and minimises initial 
>> capacity and load factor as suggested or happy to take direction and 
>> suggestions from anyone with a deeper understanding of the code than myself.
> 
> I think that a starting point would be to decide on a spec for the listener 
> notification order: is it specified by their registration order, or that is 
> it unspecified, in which case we can take advantage of this for better 
> performance. Leaving is unspecified and restricting ourselves to having it 
> ordered is losing on both sides.

Even though the order of notifications is unspecified, the following tests fail 
if we use a HashMap vs LinkedHashMap i.e. the notifications are not in order of 
registration:

test.javafx.scene.control.TableViewTest > 
ensureSelectionIsCorrectWhenItemsChange FAILED
java.lang.AssertionError: expected:<0> but was:<-1>
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:470)
at org.junit.Assert.assertEquals(Assert.java:454)
at 
test.javafx.scene.control.TableViewTest.ensureSelectionIsCorrectWhenItemsChange(TableViewTest.java:333)

test.javafx.scene.control.TreeTableViewTest > 
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeTableViewTest.java:2042)

test.javafx.scene.control.TreeViewTest > test_rt27185 FAILED
java.lang.AssertionError: expected: but 
was:
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:145)
at 
test.javafx.scene.control.TreeViewTest.test_rt27185(TreeViewTest.java:603)

test.javafx.scene.control.TreeViewTest > 
test_rt27180_collapseBranch_laterSiblingAndChildrenSelected FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeViewTest.test_rt27180_collapseBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:973)

test.javafx.scene.control.TreeViewTest > 
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeViewTest.java:992)


These would need to be investigated to see why they assume this order.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Tue, 25 Feb 2020 12:18:34 GMT, dannygonzalez 
 wrote:

>> I haven't done a detailed review yet, but I worry about the memory 
>> consumption and performance of using a Map for the simple cases where there 
>> is only a single (or very small number) of listeners. These methods are used 
>> in many more places than just the TableView / TreeTableView implementation.
> 
> Replying to @nlisker and @kevinrushforth general comments about the memory 
> consumption of using a LinkedHashMap:
> 
> I agree that at the very least these should be lazy initialised when they are 
> needed and that we should initially allocate a small capacity and load factor.
> 
> We could also have some sort of strategy where we could use arrays or lists 
> if the number of listeners are less than some threshold (i.e. keeping the 
> implementation with a similar overhead to the original) and then switch to 
> the HashMap when it exceeds this threshold. This would make the code more 
> complicated and I wonder if this is the worth the extra complexity.
>  
> I know that in our application, the thousands of listeners unregistering when 
> using a TableView was stalling the JavaFX thread. 
> 
> I am happy to submit code that lazy initialises and minimises initial 
> capacity and load factor as suggested or happy to take direction and 
> suggestions from anyone with a deeper understanding of the code than myself.

I think that a starting point would be to decide on a spec for the listener 
notification order: is it specified by their registration order, or that is it 
unspecified, in which case we can take advantage of this for better 
performance. Leaving is unspecified and restricting ourselves to having it 
ordered is losing on both sides.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 00:43:34 GMT, Kevin Rushforth  wrote:

>> Sorry for the interruption, send a PR that corrects the same problem.
> 
> I haven't done a detailed review yet, but I worry about the memory 
> consumption and performance of using a Map for the simple cases where there 
> is only a single (or very small number) of listeners. These methods are used 
> in many more places than just the TableView / TreeTableView implementation.

Replying to @nlisker and @kevinrushforth general comments about the memory 
consumption of using a LinkedHashMap:

I agree that at the very least these should be lazy initialised when they are 
needed and that we should initially allocate a small capacity and load factor.

We could also have some sort of strategy where we could use arrays or lists if 
the number of listeners are less than some threshold (i.e. keeping the 
implementation with a similar overhead to the original) and then switch to the 
HashMap when it exceeds this threshold. This would make the code more 
complicated and I wonder if this is the worth the extra complexity.
 
I know that in our application, the thousands of listeners unregistering when 
using a TableView was stalling the JavaFX thread. 

I am happy to submit code that lazy initialises and minimises initial capacity 
and load factor as suggested or happy to take direction and suggestions from 
anyone with a deeper understanding of the code than myself.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 00:45:06 GMT, Kevin Rushforth  wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>>  line 194:
>> 
>>> 193: private Map 
>>> invalidationListeners = new LinkedHashMap<>();
>>> 194: private Map, Integer> 
>>> changeListeners = new LinkedHashMap<>();
>>> 195: private T currentValue;
>> 
>> Two comments on this:
>> 
>> 1. The old implementation initialized these lazily, so we might want to 
>> preserve that behavior. I think it is reasonable since in most cases an 
>> observable won't have both types of listeners attached to it.
>> 2. Since we're dealing wither performance optimizations here, we should 
>> think about what the `initialCapcity` and `loadFactor` of these maps should 
>> be, as it can greatly increase performance when dealing with a large amount 
>> of entries.
> 
> Adding to this, we need to ensure that the simple case of a few listeners 
> doesn't suffer memory bloat. It may make sense to initially allocate a Map 
> with a small capacity and load factor, and then reallocate it if someone adds 
> more than a certain number of listeners.

Agree with the lazy initialisation and the initial setting of capacity and load 
factor to reduce memory footprint unless it's needed.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Kevin Rushforth]

On Mon, 24 Feb 2020 23:45:03 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 194:
> 
>> 193: private Map 
>> invalidationListeners = new LinkedHashMap<>();
>> 194: private Map, Integer> changeListeners 
>> = new LinkedHashMap<>();
>> 195: private T currentValue;
> 
> Two comments on this:
> 
> 1. The old implementation initialized these lazily, so we might want to 
> preserve that behavior. I think it is reasonable since in most cases an 
> observable won't have both types of listeners attached to it.
> 2. Since we're dealing wither performance optimizations here, we should think 
> about what the `initialCapcity` and `loadFactor` of these maps should be, as 
> it can greatly increase performance when dealing with a large amount of 
> entries.

Adding to this, we need to ensure that the simple case of a few listeners 
doesn't suffer memory bloat. It may make sense to initially allocate a Map with 
a small capacity and load factor, and then reallocate it if someone adds more 
than a certain number of listeners.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Kevin Rushforth]

On Sat, 22 Feb 2020 08:42:52 GMT, yosbits 
 wrote:

>> @kevinrushforth just a note to say there are other ExpressionHelper classes 
>> (i.e. MapExpressionHelper, SetExpressionHelper and ListExpressionHelper) 
>> that also use arrays and suffer from the linear search issue when removing 
>> listeners. 
>> 
>> These however didn't appear in the critical path of the JavaFXThread and 
>> didn't come up in my profiling of TableView.
>> 
>> If this pull request is accepted, my opinion is that they should probably 
>> all move to using the same pattern as here, which is to use Maps instead of 
>> Arrays for their listener lists so that all these classes are uniform.
>> 
>> Thanks
> 
> Sorry for the interruption, send a PR that corrects the same problem.

I haven't done a detailed review yet, but I worry about the memory consumption 
and performance of using a Map for the simple cases where there is only a 
single (or very small number) of listeners. These methods are used in many more 
places than just the TableView / TreeTableView implementation.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Thu, 6 Feb 2020 16:13:33 GMT, dannygonzalez 
 wrote:

> https://bugs.openjdk.java.net/browse/JDK-8185886
> 
> Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays 
> to improve listener removal speed.
> 
> This was due to the removal of listeners in TableView taking up to 50% of CPU 
> time on the JavaFX Application thread when scrolling/adding rows to 
> TableViews.
> 
> This may alleviate some of the issues seen here:
> 
> TableView has a horrific performance with many columns #409
> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
> 
> JDK-8088394 : Huge memory consumption in TableView with too many columns
> JDK-8166956: JavaFX TreeTableView slow scroll performance
> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
> horizontal direction
> 
> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
> columns
> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html

modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java 
line 194:

> 193: private Map invalidationListeners 
> = new LinkedHashMap<>();
> 194: private Map, Integer> changeListeners 
> = new LinkedHashMap<>();
> 195: private T currentValue;

Two comments on this:

1. The old implementation initialized these lazily, so we might want to 
preserve that behavior. I think it is reasonable since in most cases an 
observable won't have both types of listeners attached to it.
2. Since we're dealing wither performance optimizations here, we should think 
about what the `initialCapcity` and `loadFactor` of these maps should be, as it 
can greatly increase performance when dealing with a large amount of entries.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Mon, 24 Feb 2020 11:04:26 GMT, dannygonzalez 
 wrote:

>> Just wanted to keep a similar behaviour as before using the same calculation 
>> based originally on the size of the listeners array list and now based on 
>> the size of the map. So in theory the weak listeners should be trimmed at 
>> the same rate as before.
>> Happy to hear alternatives.
> 
> Also bear in mind that the trimming of weak listeners is extremely slow as it 
> has to iterate through each listener performing the weak listener test. We 
> can't call this every time we add a listener as this would lock up the JavaFX 
> thread completely (I tried it). 
> I assume this is why the original calculation was used where it backs of the 
> rate the weak listener trimming code was called as the array list grew.

I honestly don't quite understand the old cleanup behavior of `(oldCapacity * 
3)/2 + 1`. Why is it grown by x1.5? In your tests, can you try to change the 
cleanup threshold to higher and lower values and see what differences you get?

At the very least, the initial values of the counters should be set according 
to the specific constructor used.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 11:32:25 GMT, Nir Lisker  wrote:

>> Nope, actually:
>> `listeners.keySet().removeIf(p::test) `
>> 
>> is not the same as:
>> `listeners.entrySet().removeIf(e -> p.test(e.getKey()));`
>> 
>> We need to test against the entrySet.key not the entrySet itself.
> 
>> We need to test against the entrySet.key not the entrySet itself.
> 
> I suggested to test against the elements in `keySet()`, which are the same as 
> the ones in `entrySet().getKey()`.

Gotcha, sorry I missed that.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Mon, 24 Feb 2020 11:09:30 GMT, dannygonzalez 
 wrote:

>> Agreed, will change.
> 
> Nope, actually:
> `listeners.keySet().removeIf(p::test) `
> 
> is not the same as:
> `listeners.entrySet().removeIf(e -> p.test(e.getKey()));`
> 
> We need to test against the entrySet.key not the entrySet itself.

> We need to test against the entrySet.key not the entrySet itself.

I suggested to test against the elements in `keySet()`, which are the same as 
the ones in `entrySet().getKey()`.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 08:11:40 GMT, dannygonzalez 
 wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java
>>  line 64:
>> 
>>> 63: 
>>> 64: listeners.entrySet().removeIf(e -> p.test(e.getKey()));
>>> 65: }
>> 
>> This can be `listeners.keySet().removeIf(p::test);`.
> 
> Agreed, will change.

Nope, actually:
`listeners.keySet().removeIf(p::test) `

is not the same as:
`listeners.entrySet().removeIf(e -> p.test(e.getKey()));`

We need to test against the entrySet.key not the entrySet itself.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 10:16:55 GMT, dannygonzalez 
 wrote:

>>> So we need to keep the original behaviour from before where a count was 
>>> checked on every addListener call and the weak references were purged from 
>>> the array using the original calculation for this count.
>> 
>> The GC'd weak listeners do need to be purged, but why is the original 
>> behavior of the counters still applicable?
> 
> Just wanted to keep a similar behaviour as before using the same calculation 
> based originally on the size of the listeners array list and now based on the 
> size of the map. So in theory the weak listeners should be trimmed at the 
> same rate as before.
> Happy to hear alternatives.

Also bear in mind that the trimming of weak listeners is extremely slow as it 
has to iterate through each listener performing the weak listener test. We 
can't call this every time we add a listener as this would lock up the JavaFX 
thread completely (I tried it). 
I assume this is why the original calculation was used where it backs of the 
rate the weak listener trimming code was called as the array list grew.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 09:59:28 GMT, Nir Lisker  wrote:

>> As far as I know a LinkedHashMap doesn't automatically remove weak listener 
>> entries. The listener maps can be holding weak listeners as well as normal 
>> listeners.
>> So we need to keep the original behaviour from before where a count was 
>> checked on every addListener call and the weak references were purged from 
>> the array using the original calculation for this count.
>> Otherwise the map would never garbage collect these weak references.
>> 
>> The initial value of 2 for these counts was just the starting count although 
>> this is not necessarily strictly accurate. To be completely accurate then we 
>> would have to set the appropriate count in each constructor as follows:
>> 
>> i.e. in the Constructor with 2 InvalidationListeners:
>> weakChangeListenerGcCount = 0
>> weakInvalidationListenerGcCount = 2
>> 
>> in the Constructor with 2 ChangeListeners:
>> weakChangeListenerGcCount = 2
>> weakInvalidationListenerGcCount = 0
>> 
>> in the Constructor with 1 InvalidationListener and 1 ChangeListener:
>> weakChangeListenerGcCount = 1
>> weakInvalidationListenerGcCount = 1
>> 
>> Now, I could have used a WeakHashMap to store the listeners where it would 
>> automatically purge weak listeners but this doesn't maintain insertion 
>> order. Even though the specification doesn't mandate that listeners should 
>> be called back in the order they are registered, the unit tests failed when 
>> I didn't maintain order.
>> 
>> I am happy to remove this weak listener purge code (as it would make the 
>> code much simpler) but then we wouldn't automatically remove the weak 
>> listeners, but this may not be deemed a problem anyway?
> 
>> So we need to keep the original behaviour from before where a count was 
>> checked on every addListener call and the weak references were purged from 
>> the array using the original calculation for this count.
> 
> The GC'd weak listeners do need to be purged, but why is the original 
> behavior of the counters still applicable?

Just wanted to keep a similar behaviour as before using the same calculation 
based originally on the size of the listeners array list and now based on the 
size of the map. So in theory the weak listeners should be trimmed at the same 
rate as before.
Happy to hear alternatives.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Mon, 24 Feb 2020 09:14:51 GMT, dannygonzalez 
 wrote:

>> I agree, I kept these in as I wasn't sure if there was a need to manually 
>> force the garbage collection of weak listeners at the same rate as the 
>> original implementation.
>> Removing this would make sense to me also.
>> 
>> Updated my thoughts on this, see my comments below.
> 
> As far as I know a LinkedHashMap doesn't automatically remove weak listener 
> entries. The listener maps can be holding weak listeners as well as normal 
> listeners.
> So we need to keep the original behaviour from before where a count was 
> checked on every addListener call and the weak references were purged from 
> the array using the original calculation for this count.
> Otherwise the map would never garbage collect these weak references.
> 
> The initial value of 2 for these counts was just the starting count although 
> this is not necessarily strictly accurate. To be completely accurate then we 
> would have to set the appropriate count in each constructor as follows:
> 
> i.e. in the Constructor with 2 InvalidationListeners:
> weakChangeListenerGcCount = 0
> weakInvalidationListenerGcCount = 2
> 
> in the Constructor with 2 ChangeListeners:
> weakChangeListenerGcCount = 2
> weakInvalidationListenerGcCount = 0
> 
> in the Constructor with 1 InvalidationListener and 1 ChangeListener:
> weakChangeListenerGcCount = 1
> weakInvalidationListenerGcCount = 1
> 
> Now, I could have used a WeakHashMap to store the listeners where it would 
> automatically purge weak listeners but this doesn't maintain insertion order. 
> Even though the specification doesn't mandate that listeners should be called 
> back in the order they are registered, the unit tests failed when I didn't 
> maintain order.
> 
> I am happy to remove this weak listener purge code (as it would make the code 
> much simpler) but then we wouldn't automatically remove the weak listeners, 
> but this may not be deemed a problem anyway?

> So we need to keep the original behaviour from before where a count was 
> checked on every addListener call and the weak references were purged from 
> the array using the original calculation for this count.

The GC'd weak listeners do need to be purged, but why is the original behavior 
of the counters still applicable?

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 08:14:02 GMT, dannygonzalez 
 wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>>  line 197:
>> 
>>> 196: private int weakChangeListenerGcCount = 2;
>>> 197: private int weakInvalidationListenerGcCount = 2;
>>> 198: 
>> 
>> Why are these set to 2 and why do you need them at all? The previous 
>> implementation needed to grow and shrink the array so it had to keep these, 
>> but `Map` takes care of this for you.
> 
> I agree, I kept these in as I wasn't sure if there was a need to manually 
> force the garbage collection of weak listeners at the same rate as the 
> original implementation.
> Removing this would make sense to me also.
> 
> Updated my thoughts on this, see my comments below.

As far as I know a LinkedHashMap doesn't automatically remove weak listener 
entries. The listener maps can be holding weak listeners as well as normal 
listeners.
So we need to keep the original behaviour from before where a count was checked 
on every addListener call and the weak references were purged from the array 
using the original calculation for this count.
Otherwise the map would never garbage collect these weak references.

The initial value of 2 for these counts was just the starting count although 
this is not necessarily strictly accurate. To be completely accurate then we 
would have to set the appropriate count in each constructor as follows:

i.e. in the Constructor with 2 InvalidationListeners:
weakChangeListenerGcCount = 0
weakInvalidationListenerGcCount = 2

in the Constructor with 2 ChangeListeners:
weakChangeListenerGcCount = 2
weakInvalidationListenerGcCount = 0

in the Constructor with 1 InvalidationListener and 1 ChangeListener:
weakChangeListenerGcCount = 1
weakInvalidationListenerGcCount = 1

Now, I could have used a WeakHashMap to store the listeners where it would 
automatically purge weak listeners but this doesn't maintain insertion order. 
Even though the specification doesn't mandate that listeners should be called 
back in the order they are registered, the unit tests failed when I didn't 
maintain order.

I am happy to remove this weak listener purge code (as it would make the code 
much simpler) but then we wouldn't automatically remove the weak listeners, but 
this may not be deemed a problem anyway?

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Sun, 23 Feb 2020 01:05:25 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 283:
> 
>> 282: final Map 
>> curInvalidationList = new LinkedHashMap<>(invalidationListeners);
>> 283: final Map, Integer> curChangeList 
>> = new LinkedHashMap<>(changeListeners);
>> 284: 
> 
> You only need the entry set, so you don't need to copy the map, just the set.

Thanks, yes the EntrySet would make more sense here. I'll fix that up.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Sun, 23 Feb 2020 02:54:55 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 285:
> 
>> 284: 
>> 285: curInvalidationList.entrySet().forEach(entry -> 
>> fireInvalidationListeners(entry));
>> 286: 
> 
> The lambda can be converted to a method reference.

Agreed,  I'll fix.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Sun, 23 Feb 2020 03:09:28 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java
>  line 64:
> 
>> 63: 
>> 64: listeners.entrySet().removeIf(e -> p.test(e.getKey()));
>> 65: }
> 
> This can be `listeners.keySet().removeIf(p::test);`.

Agreed, will change.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Sun, 23 Feb 2020 04:20:55 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 197:
> 
>> 196: private int weakChangeListenerGcCount = 2;
>> 197: private int weakInvalidationListenerGcCount = 2;
>> 198: 
> 
> Why are these set to 2 and why do you need them at all? The previous 
> implementation needed to grow and shrink the array so it had to keep these, 
> but `Map` takes care of this for you.

I agree, I kept these in as I wasn't sure if there was a need to manually force 
the garbage collection of weak listeners at the same rate as the original 
implementation.
Removing this would make sense to me also.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Thu, 6 Feb 2020 16:13:33 GMT, dannygonzalez 
 wrote:

> https://bugs.openjdk.java.net/browse/JDK-8185886
> 
> Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays 
> to improve listener removal speed.
> 
> This was due to the removal of listeners in TableView taking up to 50% of CPU 
> time on the JavaFX Application thread when scrolling/adding rows to 
> TableViews.
> 
> This may alleviate some of the issues seen here:
> 
> TableView has a horrific performance with many columns #409
> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
> 
> JDK-8088394 : Huge memory consumption in TableView with too many columns
> JDK-8166956: JavaFX TreeTableView slow scroll performance
> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
> horizontal direction
> 
> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
> columns
> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html

modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java 
line 283:

> 282: final Map curInvalidationList 
> = new LinkedHashMap<>(invalidationListeners);
> 283: final Map, Integer> curChangeList 
> = new LinkedHashMap<>(changeListeners);
> 284: 

You only need the entry set, so you don't need to copy the map, just the set.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java 
line 285:

> 284: 
> 285: curInvalidationList.entrySet().forEach(entry -> 
> fireInvalidationListeners(entry));
> 286: 

The lambda can be converted to a method reference.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java
 line 64:

> 63: 
> 64: listeners.entrySet().removeIf(e -> p.test(e.getKey()));
> 65: }

This can be `listeners.keySet().removeIf(p::test);`.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java 
line 197:

> 196: private int weakChangeListenerGcCount = 2;
> 197: private int weakInvalidationListenerGcCount = 2;
> 198: 

Why are these set to 2 and why do you need them at all? The previous 
implementation needed to grow and shrink the array so it had to keep these, but 
`Map` takes care of this for you.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[yosbits]

On Tue, 18 Feb 2020 09:02:36 GMT, dannygonzalez 
 wrote:

>> @dannygonzalez the reason for the `jcheck` failure is that you have commits 
>> with two different email addresses in your branch. At this point, it's 
>> probably best to squash the commits into a single commit with `git rebase -i 
>> master` (presuming that your local `master` is up to date), and then do a 
>> force-push.
> 
> @kevinrushforth just a note to say there are other ExpressionHelper classes 
> (i.e. MapExpressionHelper, SetExpressionHelper and ListExpressionHelper) that 
> also use arrays and suffer from the linear search issue when removing 
> listeners. 
> 
> These however didn't appear in the critical path of the JavaFXThread and 
> didn't come up in my profiling of TableView.
> 
> If this pull request is accepted, my opinion is that they should probably all 
> move to using the same pattern as here, which is to use Maps instead of 
> Arrays for their listener lists so that all these classes are uniform.
> 
> Thanks

Sorry for the interruption, send a PR that corrects the same problem.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Jeanette Winzenburg]

On Wed, 12 Feb 2020 12:43:58 GMT, dannygonzalez 
 wrote:

>>> 
>>> Although that does seem odd behaviour to me. Obviously as the original 
>>> implementation was using an array I can see how the implementation drove 
>>> that specification.
>>> 
>> 
>> whatever drove it (had been so since the beginning ot java desktop, at least 
>> since the days of swing), there is no way to change it, is it?
>> 
>>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>>> all passed. It would be nice if there was a specific test case for this 
>>> behaviour.
>>> 
>> 
>> yeah, the test coverage is ... not optimal :)
>> 
>>> I would need to store a registration count for each listener to satisfy 
>>> this requirement.
>> 
>> a count plus some marker as to where it was added: 
>> 
>>addListener(firstL)
>>addListener(secondL)
>>addListener(firstL)
>> 
>> must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
>> which brings us back to .. an array?
> 
> The listeners are called back in the order they were registered in my 
> implementation although I didn’t see this requirement in the spec unless I 
> missed something.
> 
> The performance unregistering thousands of listeners by TableView from the 
> array is killing the performance of our JavaFX application. It takes up to 
> 60% of JavaFX Application thread CPU and there are various bug reports around 
> this same TableView performance issue.
> The set implementation has lowered this to below 1%.
> 
> This pull request may be too fundamental a change to go into mainline. We 
> however have to build our local OpenJFX with this fix or our application is 
> unusable.
> 
> I’m happy to receive any suggestions.
> 
> Danny
> 
> 
> On 12 Feb 2020, at 12:22, Jeanette Winzenburg 
> mailto:notificati...@github.com>> wrote:
> 
> 
> Although that does seem odd behaviour to me. Obviously as the original 
> implementation was using an array I can see how the implementation drove that 
> specification.
> 
> whatever drove it (had been so since the beginning ot java desktop, at least 
> since the days of swing), there is no way to change it, is it?
> 
> Non of the JavaFx unit tests test for that specific case as the unit tests 
> all passed. It would be nice if there was a specific test case for this 
> behaviour.
> 
> yeah, the test coverage is ... not optimal :)
> 
> I would need to store a registration count for each listener to satisfy this 
> requirement.
> 
> a count plus some marker as to where it was added:
> 
> addListener(firstL)
> addListener(secondL)
> addListener(firstL)
> 
> must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
> which brings us back to .. an array?
> 
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on 
> GitHub,
>  or 
> unsubscribe.

> 
> The listeners are called back in the order they were registered in my 
> implementation although I didn’t see this requirement in the spec unless I 
> missed something. 

yeah, you are right can't find that spec on sequence right now - maybe I 
dreamed it :)

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Wed, 12 Feb 2020 12:22:15 GMT, Jeanette Winzenburg  
wrote:

>>> hmm ... wouldn't the change violate spec of adding listeners:
>>> 
>>> > If the same listener is added more than once, then it will be notified 
>>> > more than once.
>> 
>> True, I hadn't read that spec in ObservableValueBase. 
>> Although that does seem odd behaviour to me. Obviously as the original 
>> implementation was using an array I can see how the implementation drove 
>> that specification.
>> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>> all passed. It would be nice if there was a specific test case for this 
>> behaviour.
>> 
>> I would need to store a registration count for each listener to satisfy this 
>> requirement.
> 
>> 
>> Although that does seem odd behaviour to me. Obviously as the original 
>> implementation was using an array I can see how the implementation drove 
>> that specification.
>> 
> 
> whatever drove it (had been so since the beginning ot java desktop, at least 
> since the days of swing), there is no way to change it, is it?
> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>> all passed. It would be nice if there was a specific test case for this 
>> behaviour.
>> 
> 
> yeah, the test coverage is ... not optimal :)
> 
>> I would need to store a registration count for each listener to satisfy this 
>> requirement.
> 
> a count plus some marker as to where it was added: 
> 
>addListener(firstL)
>addListener(secondL)
>addListener(firstL)
> 
> must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
> which brings us back to .. an array?

The listeners are called back in the order they were registered in my 
implementation although I didn’t see this requirement in the spec unless I 
missed something.

The performance unregistering thousands of listeners by TableView from the 
array is killing the performance of our JavaFX application. It takes up to 60% 
of JavaFX Application thread CPU and there are various bug reports around this 
same TableView performance issue.
The set implementation has lowered this to below 1%.

This pull request may be too fundamental a change to go into mainline. We 
however have to build our local OpenJFX with this fix or our application is 
unusable.

I’m happy to receive any suggestions.

Danny


On 12 Feb 2020, at 12:22, Jeanette Winzenburg 
mailto:notificati...@github.com>> wrote:


Although that does seem odd behaviour to me. Obviously as the original 
implementation was using an array I can see how the implementation drove that 
specification.

whatever drove it (had been so since the beginning ot java desktop, at least 
since the days of swing), there is no way to change it, is it?

Non of the JavaFx unit tests test for that specific case as the unit tests all 
passed. It would be nice if there was a specific test case for this behaviour.

yeah, the test coverage is ... not optimal :)

I would need to store a registration count for each listener to satisfy this 
requirement.

a count plus some marker as to where it was added:

addListener(firstL)
addListener(secondL)
addListener(firstL)

must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
which brings us back to .. an array?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on 
GitHub,
 or 
unsubscribe.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Kevin Rushforth]

On Wed, 12 Feb 2020 13:06:00 GMT, Jeanette Winzenburg  
wrote:

>> The listeners are called back in the order they were registered in my 
>> implementation although I didn’t see this requirement in the spec unless I 
>> missed something.
>> 
>> The performance unregistering thousands of listeners by TableView from the 
>> array is killing the performance of our JavaFX application. It takes up to 
>> 60% of JavaFX Application thread CPU and there are various bug reports 
>> around this same TableView performance issue.
>> The set implementation has lowered this to below 1%.
>> 
>> This pull request may be too fundamental a change to go into mainline. We 
>> however have to build our local OpenJFX with this fix or our application is 
>> unusable.
>> 
>> I’m happy to receive any suggestions.
>> 
>> Danny
>> 
>> 
>> On 12 Feb 2020, at 12:22, Jeanette Winzenburg 
>> mailto:notificati...@github.com>> wrote:
>> 
>> 
>> Although that does seem odd behaviour to me. Obviously as the original 
>> implementation was using an array I can see how the implementation drove 
>> that specification.
>> 
>> whatever drove it (had been so since the beginning ot java desktop, at least 
>> since the days of swing), there is no way to change it, is it?
>> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>> all passed. It would be nice if there was a specific test case for this 
>> behaviour.
>> 
>> yeah, the test coverage is ... not optimal :)
>> 
>> I would need to store a registration count for each listener to satisfy this 
>> requirement.
>> 
>> a count plus some marker as to where it was added:
>> 
>> addListener(firstL)
>> addListener(secondL)
>> addListener(firstL)
>> 
>> must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
>> which brings us back to .. an array?
>> 
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on 
>> GitHub,
>>  or 
>> unsubscribe.
> 
>> 
>> The listeners are called back in the order they were registered in my 
>> implementation although I didn’t see this requirement in the spec unless I 
>> missed something. 
> 
> yeah, you are right can't find that spec on sequence right now - maybe I 
> dreamed it :)

@dannygonzalez the reason for the `jcheck` failure is that you have commits 
with two different email addresses in your branch. At this point, it's probably 
best to squash the commits into a single commit with `git rebase -i master` 
(presuming that your local `master` is up to date), and then do a force-push.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Jeanette Winzenburg]

On Thu, 6 Feb 2020 16:22:28 GMT, dannygonzalez 
 wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> /signed
> 
> I have signed the Oracle Contributor Agreement today. Have not received back 
> any confirmation yet though.

hmm ... wouldn't the change violate spec of adding listeners:

> If the same listener is added more than once, then it will be notified more 
> than once.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Wed, 12 Feb 2020 11:29:24 GMT, Jeanette Winzenburg  
wrote:

>> /signed
>> 
>> I have signed the Oracle Contributor Agreement today. Have not received back 
>> any confirmation yet though.
> 
> hmm ... wouldn't the change violate spec of adding listeners:
> 
>> If the same listener is added more than once, then it will be notified more 
>> than once.

> hmm ... wouldn't the change violate spec of adding listeners:
> 
> > If the same listener is added more than once, then it will be notified more 
> > than once.

True, I hadn't read that spec in ObservableValueBase. 
Although that does seem odd behaviour to me. Obviously as the original 
implementation was using an array I can see how the implementation drove that 
specification.

Non of the JavaFx unit tests test for that specific case as the unit tests all 
passed. It would be nice if there was a specific test case for this behaviour.

I would need to store a registration count for each listener to satisfy this 
requirement.

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 6 Feb 2020 16:13:33 GMT, dannygonzalez 
 wrote:

> https://bugs.openjdk.java.net/browse/JDK-8185886
> 
> Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays 
> to improve listener removal speed.
> 
> This was due to the removal of listeners in TableView taking up to 50% of CPU 
> time on the JavaFX Application thread when scrolling/adding rows to 
> TableViews.
> 
> This may alleviate some of the issues seen here:
> 
> TableView has a horrific performance with many columns #409
> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
> 
> JDK-8088394 : Huge memory consumption in TableView with too many columns
> JDK-8166956: JavaFX TreeTableView slow scroll performance
> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
> horizontal direction
> 
> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
> columns
> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html

/signed

I have signed the Oracle Contributor Agreement today. Have not received back 
any confirmation yet though.

-

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