Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction #125

Open
wants to merge 2 commits into
base: master
from

Conversation

@yososs
Copy link

@yososs yososs commented Feb 24, 2020

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.

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<String[]> tableView = new TableView<>();

//	    tableView.setTableMenuButtonVisible(true); //too heavy
		if (USE_HEIGHT_FIXED_SIZE) {
			tableView.setFixedCellSize(24);
		}

		final ObservableList<TableColumn<String[], ?>> columns = tableView.getColumns();
		for (int i = 0; i < COL_COUNT; i++) {
			final TableColumn<String[], String> 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<String[]> 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<String[], ?>[] 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();
	}
}
import javafx.animation.AnimationTimer;
import javafx.application.Application;
import javafx.application.Platform;
import javafx.beans.property.ReadOnlyStringWrapper;
import javafx.scene.Scene;
import javafx.scene.control.TreeItem;
import javafx.scene.control.TreeTableColumn;
import javafx.scene.control.TreeTableColumn.CellDataFeatures;
import javafx.scene.control.TreeTableView;
import javafx.stage.Stage;

public class BigTreeTableViewTest extends Application {

	private static final boolean USE_HEIGHT_FIXED_SIZE = true;
//	private static final int COL_COUNT = 900;
	private static final int COL_COUNT = 800;
//	private static final int COL_COUNT = 600;
//	private static final int COL_COUNT = 500;
//	private static final int COL_COUNT = 400;
//	private static final int COL_COUNT = 300;
//	private static final int COL_COUNT = 200;
//	private static final int COL_COUNT = 100;

	public static void main(final String[] args) {
		Application.launch(args);
	}

	@Override
	public void start(final Stage stage) {
		final TreeItem<String> root = new TreeItem<>("Root");
		final TreeTableView<String> treeTableView = new TreeTableView<>(root);
		if (USE_HEIGHT_FIXED_SIZE) {
			treeTableView.setFixedCellSize(24);
		}
		treeTableView.setPrefWidth(800);
		treeTableView.setPrefHeight(500);
		stage.setWidth(800);
		stage.setHeight(500);

		Platform.runLater(() -> {
			for (int i = 0; i < 100; i++) {
				TreeItem<String> child = this.addNodes(root);
				child = this.addNodes(child);
				child = this.addNodes(child);
				child = this.addNodes(child);
			}
		});

		final TreeTableColumn<String, String>[] cols = new TreeTableColumn[COL_COUNT + 1];
		final TreeTableColumn<String, String> column = new TreeTableColumn<>("Column");
		column.setPrefWidth(150);
		column.setCellValueFactory(
				(final CellDataFeatures<String, String> p) -> new ReadOnlyStringWrapper(p.getValue().getValue()));
		cols[0] = column;

		for (int i = 0; i < COL_COUNT; i++) {
			final TreeTableColumn<String, String> col = new TreeTableColumn<>(Integer.toString(i));
			col.setPrefWidth(60);
			col.setCellValueFactory(val -> new ReadOnlyStringWrapper(val.getValue().getValue()+":"+val.getTreeTableColumn().getText()));
			cols[i + 1] = col;
		}
		treeTableView.getColumns().addAll(cols);

		final Scene scene = new Scene(treeTableView, 800, 500);
		stage.setScene(scene);
		stage.show();
		this.prepareTimeline(scene);
	}

	private TreeItem<String> addNodes(final TreeItem<String> parent) {

		final TreeItem<String>[] childNodes = new TreeItem[20];
		for (int i = 0; i < childNodes.length; i++) {
			childNodes[i] = new TreeItem<>("N" + i);
		}
		final TreeItem<String> root = new TreeItem<>("dir");
		root.setExpanded(true);
		root.getChildren().addAll(childNodes);
		parent.setExpanded(true);
		parent.getChildren().add(root);
		return root;
	}

	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();
	}

}

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction

Download

$ git fetch https://git.openjdk.java.net/jfx pull/125/head:pull/125
$ git checkout pull/125

@bridgekeeper bridgekeeper bot added the oca label Feb 24, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 24, 2020

Hi yososs, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user yososs" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@yososs
Copy link
Author

@yososs yososs commented Feb 24, 2020

/signed

@bridgekeeper bridgekeeper bot added the oca-verify label Feb 24, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 24, 2020

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@yososs
Copy link
Author

@yososs yososs commented Feb 24, 2020

I'm signed to OCA and already a contributor.

@yososs yososs force-pushed the yososs:8185886_fix_scroll_performance_of_tableview branch from 17ce602 to 04951ab Feb 24, 2020
// TableCells in the scenegraph
getChildren().remove(tableCell);
}
}else if(fixedCellSizeEnabled && lastVisibleColumnIndex<column){

This comment has been minimized.

@sghpjuikit

sghpjuikit Feb 27, 2020

Hi @sghpjuikit, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user sghpjuikit for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

if(tableCell.getParent()==null){
children.add(tableCell);
}
}else{

This comment has been minimized.

@sghpjuikit

sghpjuikit Feb 27, 2020

Hi @sghpjuikit, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user sghpjuikit for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

}
lastVisibleColumnIndex = column;
}else if( firstVisibleColumnIndex != -1 ) {

This comment has been minimized.

@sghpjuikit

sghpjuikit Feb 27, 2020

Hi @sghpjuikit, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user sghpjuikit for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

final VirtualFlow<?> virtualFlow = getVirtualFlow();
final double scrollX = virtualFlow == null ? 0.0 : virtualFlow.getHbar().getValue();
final Insets padding = getSkinnable().getPadding();
final double vfWidth = virtualFlow == null ? 0.0:virtualFlow.getWidth();

This comment has been minimized.

@sghpjuikit

sghpjuikit Feb 27, 2020

Hi @sghpjuikit, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user sghpjuikit for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.


boolean isVisible = firstVisibleColumnIndex <= column && column <= lastVisibleColumnIndex;
width = snapSizeX(tableColumn.getWidth());
if (isVisible || isOverlap(tableCell.getLayoutX(), tableCell.getLayoutX()+width, scrollX, headerWidth + scrollX)) {

This comment has been minimized.

@sghpjuikit

sghpjuikit Feb 27, 2020

Hi @sghpjuikit, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user sghpjuikit for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

Copy link

@sghpjuikit sghpjuikit left a comment

Hi @sghpjuikit, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user sghpjuikit for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Mar 9, 2020

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).

@yososs yososs changed the title 8185886: Fix scroll performance of TableView with many columns 8185886: Improve scrolling performance of TableView and TreeTableView Mar 10, 2020
@robilad
Copy link

@robilad robilad commented Apr 2, 2020

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.topic@oracle.com with information about your OCA, so that I can look it up.

@yososs
Copy link
Author

@yososs yososs commented Apr 3, 2020

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

@bridgekeeper bridgekeeper bot removed oca oca-verify labels Apr 3, 2020
@openjdk openjdk bot added the rfr label Apr 3, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 3, 2020

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 14, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@yososs
Copy link
Author

@yososs yososs commented Apr 18, 2020

19fabf2

  • 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.
@kevinrushforth kevinrushforth self-requested a review Apr 21, 2020
@yososs
Copy link
Author

@yososs yososs commented Aug 27, 2020

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.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Aug 30, 2020

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. Any idea why that might happen?

@yososs yososs force-pushed the yososs:8185886_fix_scroll_performance_of_tableview branch from 19fabf2 to dcbbfdc Aug 31, 2020
@yososs
Copy link
Author

@yososs yososs commented Aug 31, 2020

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?

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Sep 1, 2020

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")

@yososs
Copy link
Author

@yososs yososs commented Sep 1, 2020

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.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Sep 2, 2020

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 :)

@yososs yososs force-pushed the yososs:8185886_fix_scroll_performance_of_tableview branch from dcbbfdc to c81dd2e Sep 3, 2020
@yososs
Copy link
Author

@yososs yososs commented Sep 3, 2020

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.

 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);
    }
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 8, 2020

@yososs Per this message on the openjfx-dev mailing list, I have closed 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
@yososs yososs changed the title 8185886: Improve scrolling performance of TableView and TreeTableView 8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction Sep 9, 2020
@yososs
Copy link
Author

@yososs yososs commented Sep 9, 2020

Below are the answers to JBS's suggestions.

The patch below resolves the issue, but it is not likely the correct solution (we are trying to determine the table width, but we are getting the width and padding on the underlying virtualflow (the width is fine, the padding should really come from tableview directly):

You probably mention this part,
At least padding refers to Skinnable(TableRow or TreeTableRow). This is the same as before (L683). The width of VirtualFlow is determined by the header of Table.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java#L360-L365

 final VirtualFlow<?> virtualFlow = getVirtualFlow(); 
 final double scrollX = virtualFlow == null ? 0.0 : virtualFlow.getHbar().getValue(); 
 final Insets padding = getSkinnable().getPadding(); 
 final double vfWidth = virtualFlow == null ? 0.0:virtualFlow.getWidth(); 
 final double headerWidth = vfWidth - (padding.getLeft() + padding.getRight()); 

For example, can you assume a case that does not work properly?

@yososs
Copy link
Author

@yososs yososs commented Sep 10, 2020

This is the answer to JBS's comment.

The BigTreeTableViewTest.java attached to this PR commentary is a modified version of the JDK-8166956 test code. The original test code is good for reproducing the problem, but I have decided that it cannot be used as a test code because the cell values are random and the cell values change randomly with the close / expand operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants