8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction

[yososs]

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

• [x] Change must not contain extraneous whitespace
• [x] 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[bot]]

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]

/signed

[bridgekeeper[bot]]

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]

I'm signed to OCA and already a contributor.

[nlisker]

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

[robilad]

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

[yososs]

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

[mlbridge[bot]]

Webrevs

• 00: Full (c81dd2e2)
• 03: Full - Incremental (c81dd2e2fe935fa87c3e678cdadeb2f977f5975f)
• 02: Full - Incremental (dcbbfdcd7ffdf9bddb4e5618805308dc77514bc9)
• 01: Full - Incremental (19fabf2eafcb02b519d39a1b0a9dad5b9209db64)
• 00: Full (04951abfcb885265afc65f82d8fd1984a7bf0ecc)

[kevinrushforth]

/reviewers 2

[openjdk[bot]]

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

[yososs]

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.

[yososs]

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]

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]

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]

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]

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]

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]

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]

@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]

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]

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.

[openjdk[bot]]

@yososs Unknown command signed - for a list of valid commands use /help.

[sghpjuikit]

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.

• [ ] I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

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

[openjdk[bot]]

@yososs this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8185886_fix_scroll_performance_of_tableview
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk[bot]]

@yososs Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[bridgekeeper[bot]]

@yososs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@yososs This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.