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

8197991: Selecting many items in a TableView is very slow #127

Open

Conversation

@yososs
Copy link

@yososs yososs commented Feb 26, 2020

https://bugs.openjdk.java.net/browse/JDK-8197991

The performance of the selectAll and selectRange methods of the MultiSelectionModel class has been greatly improved.

This greatly improves the response when selecting multiple items in ListView and TableView.

However, in TreeView / TreeTableView, the improvement effect is hidden mainly due to the design problem of the cache of TreeUtil.getTreeItem ().

Reference value of the number of data that can be handled within 3 seconds of processing time (before-> after)

ListView

  • selectAll: 400_000-> 10_000_000
  • selectRange: 7_000-> 70_000

TableView

  • selectAll: 8_000-> 700_000
  • selectRange: 7_000-> 50_000

You can see performance improvements in the following applications:

import javafx.application.Application;
import javafx.collections.ObservableList;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.ListView;
import javafx.scene.control.SelectionMode;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class SelectListViewTest extends Application {
	final int ROW_COUNT = 70_000;
//	final int ROW_COUNT = 400_000;
//	final int ROW_COUNT = 10_000_000;
//	final int ROW_COUNT = 7_000;
	
	@Override
    public void start(Stage stage) {
    	ListView<String> listView = new ListView<>();
    	listView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
        

    	ObservableList<String> items = listView.getItems();
    	for(int i=0; i<ROW_COUNT; i++) {
    		String rec = String.valueOf(i);
        	items.add(rec);
    	}
        BorderPane root = new BorderPane(listView);
    	Button selectAll = new Button("selectAll");
    	Button clearSelection = new Button("clearSelection");
    	Button selectToStart = new Button("selectToStart");
    	Button selectToEnd = new Button("selectToEnd");
    	Button selectPrevious = new Button("selectPrevious");
    	Button selectNext= new Button("selectNext");
    	
    	selectAll.setFocusTraversable(true);
    	clearSelection.setFocusTraversable(true);
    	selectToStart.setFocusTraversable(true);
    	selectToEnd.setFocusTraversable(true);
    	selectPrevious.setFocusTraversable(true);
    	selectNext.setFocusTraversable(true);

        root.setRight(new VBox(6, selectAll, selectToStart, selectToEnd, selectPrevious, selectNext, clearSelection));
        stage.setScene(new Scene(root, 600, 600));
        
        selectAll.setOnAction((e)->selectAll(listView));
        clearSelection.setOnAction((e)->clearSelection(listView));
        selectToStart.setOnAction((e)->selectToStart(listView));
        selectToEnd.setOnAction((e)->selectToLast(listView));
        selectPrevious.setOnAction((e)->selectPrevious(listView));
        selectNext.setOnAction((e)->selectNext(listView));
        
        stage.show();
    }

	private void selectAll(ListView listView) {
		long t = System.currentTimeMillis();
		listView.getSelectionModel().selectAll();
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
	private void clearSelection(ListView listView) {
		long t = System.currentTimeMillis();
		listView.getSelectionModel().clearSelection();
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
	private void selectToStart(ListView listView) {
		long t = System.currentTimeMillis();
		listView.getSelectionModel().selectRange(0, listView.getSelectionModel().getSelectedIndex());
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
	private void selectToLast(ListView listView) {
		long t = System.currentTimeMillis();
		listView.getSelectionModel().selectRange(listView.getSelectionModel().getSelectedIndex(), listView.getItems().size());
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}

	private void selectPrevious(ListView listView) {
		long t = System.currentTimeMillis();
		listView.getSelectionModel().selectPrevious();
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
	
	private void selectNext(ListView listView) {
		long t = System.currentTimeMillis();
		listView.getSelectionModel().selectNext();
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
    public static void main(String[] args) {
    	Application.launch(args);
	}
}
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.SelectionMode;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableView;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class SelectTableViewTest extends Application {

	final int ROW_COUNT = 700_000;
//	final int ROW_COUNT = 80_000;
//	final int ROW_COUNT = 50_000;
//	final int ROW_COUNT = 8_000;
	final int COL_COUNT = 3;

	@Override
    public void start(Stage stage) {
    	TableView<String[]> tableView = new TableView<>();
    	tableView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
//    	tableView.getSelectionModel().setSelectionMode(SelectionMode.SINGLE);
        
        final ObservableList<TableColumn<String[], ?>> columns = tableView.getColumns();
    	for(int i=0; i<COL_COUNT; i++) {
    		TableColumn<String[], String> column = new TableColumn<>("Col"+i);
    		final int colIndex=i;
    		column.setCellValueFactory((cell)->new SimpleStringProperty(cell.getValue()[colIndex]));
    		column.setPrefWidth(150);
			columns.add(column);
    	}

    	ObservableList<String[]> items = tableView.getItems();
    	for(int i=0; i<ROW_COUNT; i++) {
    		String[] rec = new String[COL_COUNT];
    		for(int j=0; j<rec.length; j++) {
    			rec[j] = i+":"+j;
    		}
        	items.add(rec);
    	}
        BorderPane root = new BorderPane(tableView);
    	Button selectAll = new Button("selectAll");
    	Button clearSelection = new Button("clearSelection");
    	Button selectToStart = new Button("selectToStart");
    	Button selectToEnd = new Button("selectToEnd");
    	Button selectPrevious = new Button("selectPrevious");
    	Button selectNext= new Button("selectNext");
    	
    	selectAll.setFocusTraversable(true);
    	clearSelection.setFocusTraversable(true);
    	selectToStart.setFocusTraversable(true);
    	selectToEnd.setFocusTraversable(true);
    	selectPrevious.setFocusTraversable(true);
    	selectNext.setFocusTraversable(true);

        root.setRight(new VBox(6, selectAll, selectToStart, selectToEnd, selectPrevious, selectNext, clearSelection));
        stage.setScene(new Scene(root, 600, 600));
        
        selectAll.setOnAction((e)->selectAll(tableView));
        clearSelection.setOnAction((e)->clearSelection(tableView));
        selectToStart.setOnAction((e)->selectToStart(tableView));
        selectToEnd.setOnAction((e)->selectToLast(tableView));
        selectPrevious.setOnAction((e)->selectPrevious(tableView));
        selectNext.setOnAction((e)->selectNext(tableView));
        
        stage.show();
    }

	private void selectAll(TableView tableView) {
		long t = System.currentTimeMillis();
		tableView.getSelectionModel().selectAll();
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
	private void clearSelection(TableView tableView) {
		long t = System.currentTimeMillis();
		tableView.getSelectionModel().clearSelection();
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
	private void selectToStart(TableView tableView) {
		long t = System.currentTimeMillis();
		tableView.getSelectionModel().selectRange(0, tableView.getSelectionModel().getFocusedIndex());
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
	private void selectToLast(TableView tableView) {
		long t = System.currentTimeMillis();
		tableView.getSelectionModel().selectRange(tableView.getSelectionModel().getFocusedIndex(), tableView.getItems().size());
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}

	private void selectPrevious(TableView tableView) {
		long t = System.currentTimeMillis();
		tableView.getSelectionModel().selectPrevious();
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}
	
	private void selectNext(TableView tableView) {
		long t = System.currentTimeMillis();
		tableView.getSelectionModel().selectNext();
		System.out.println("time:"+ (System.currentTimeMillis() - t));
	}

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

Progress

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

Issue

  • JDK-8197991: Selecting many items in a TableView is very slow

Download

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

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

@bridgekeeper bridgekeeper bot commented Feb 26, 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 26, 2020

/signed

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

@bridgekeeper bridgekeeper bot commented Feb 26, 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 yososs changed the title 8197991: Fix multi-select performance issues 8197991: Selecting many items in a TableView is very slow Mar 10, 2020
@ThomasDaheim
Copy link

@ThomasDaheim ThomasDaheim commented Mar 31, 2020

Hi @ThomasDaheim, 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 ThomasDaheim 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.

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

@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

Webrevs

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

@kevinrushforth kevinrushforth self-requested a review Apr 21, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 21, 2020

@aghaisas can you also review this?

@baumeister
Copy link

@baumeister baumeister commented Sep 7, 2020

Any progress with having this merged?

@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Sep 14, 2020

This is a very good attempt to improve selection performance. I have few review comments.
I ran the manual test that you have provided. It does show the improvement.
Can you please provide an automated test along with your fix?

}

// is right most bit
if( index == bitset.length()-1 ){

This comment has been minimized.

@aghaisas

aghaisas Sep 14, 2020
Collaborator

Spacing correction needed.
Correct spacing is : "if (index == bitset.length()-1) {"
Please correct at other places in this PR.

@yososs
Copy link
Author

@yososs yososs commented Sep 14, 2020

Can you please provide an automated test along with your fix?

Automated performance testing should be distinguished from regular testing tasks, as it extends the build time.
Is there a task name for that in gradle?
Also, didn't you exclude performance testing from automated testing (or Unit Test)?

Or, if you want to maintain this test in a repository, you need to tell me the directory where it is stored.

The reviewer didn't point out that the
There's a little bit of wastage left in the toArray(), so I'm going to push a modified version.

@yososs yososs force-pushed the yososs:8197991_selecting_many_items_in_a_tableview_is_very_slow branch from 870e984 to 791c03c Sep 22, 2020
@openjdk openjdk bot removed the rfr label Sep 22, 2020
@yososs yososs force-pushed the yososs:8197991_selecting_many_items_in_a_tableview_is_very_slow branch from 791c03c to 59b6d0b Sep 22, 2020
@openjdk openjdk bot added the rfr label Sep 22, 2020
Copy link
Author

@yososs yososs left a comment

I commented.

@jperedadnr
Copy link
Collaborator

@jperedadnr jperedadnr commented Dec 29, 2020

I've run SelectListView and SelectTableView tests and both run fine. As for the SelectTableView test, with 700_000 rows, when there is no selection, pressing SelectToEnd takes around 3.1 seconds, as expected. However, if there is one single row selected, after pressing SelectToEnd, the selection doesn't complete, and I have to abort and quit the app.

Instead of:

tableView.getSelectionModel().selectRange(tableView.getSelectionModel().getFocusedIndex(), tableView.getItems().size());

this:

int focusedIndex = tableView.getSelectionModel().getFocusedIndex();
tableView.getSelectionModel().clearSelection();
tableView.getSelectionModel().selectRange(focusedIndex, tableView.getItems().size());

seems to work and times are again around 3 seconds or less.

Maybe you can check why that is happening?

And about the test discussion above, I understand that running the included tests as part of the automated test wouldn't make much sense (as these can take too long). However, maybe these could be added as unit tests with a reduced number of item/rows. Instead of measuring performance (selection times would depend on each machine), I'd say you could simply verify that selection works as expected.

While most of the changes are just about caching size(), the new implementation of MultipleSelectionModelBase::indexOf adds some new code that should be tested, as part of the unit tests, again not for performance, but to assert it returns the expected values.

@yososs
Copy link
Author

@yososs yososs commented Dec 30, 2020

As an overview, the new implementation can handle selectRange up to 50_000 records. This assumes general manual operation. However, after clearing the selection (a rare operation in manual operation), it is as efficient as selectAll. However, this is a two-time change.

The response difference is caused by the next conditional branch (size == 0) of SortedList. This will be the next hotspot to improve as seen by this improvement.

I can't include it in this PR because I don't have time to work.

I haven't tried it, but changing the call to insertToMapping for each record of this method to setAllToMapping for each range seems to be as efficient as selectAll.

javafx.collections.transformation.SortedList.java

    private void addRemove(Change<? extends E> c) {
        if (c.getFrom() == 0 && c.getRemovedSize() == size) {
            removeAllFromMapping();
        } else {
            for (int i = 0, sz = c.getRemovedSize(); i < sz; ++i) {
                removeFromMapping(c.getFrom(), c.getRemoved().get(i));
            }
        }
        if (size == 0) {
            setAllToMapping(c.getList(), c.getTo()); // This is basically equivalent to getAddedSubList
                                                     // as size is 0, only valid "from" is also 0
        } else {
            for (int i = c.getFrom(), to = c.getTo(); i < to; ++i) {
                insertToMapping(c.getList().get(i), i);
            }
        }
    }
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

7 participants