[performance] faster Tree expand - eclipse.platform.swt#901 #1331

jukzi · 2023-11-27T07:32:05Z

insert every item at index 0 (in reverse order)

eclipse-platform/eclipse.platform.swt#901

laeubi · 2023-11-27T07:33:23Z

@SyntevoAlex should this happen for all platforms or only for Windows?

github-actions · 2023-11-27T07:38:33Z

Test Results

    900 files +    15     900 suites +15 51m 40s ⏱️ + 13m 18s
  7 468 tests ±      0   7 314 ✔️ -       1 153 💤 ±    0 1 ❌ +1
23 553 runs +1 962 23 043 ✔️ +1 834 509 💤 +127 1 ❌ +1

For more details on these failures, see this check.

Results for commit 727c6be. ± Comparison against base commit c41b487.

♻️ This comment has been updated with latest results.

akurtakov · 2023-11-27T07:39:13Z

Jface should be ws agnostic.

laeubi · 2023-11-27T07:53:03Z

Jface should be ws agnostic.

That's true but still we can have platform switches in it if required.

SyntevoAlex · 2023-11-28T14:12:04Z

I suggest adding a code comment with explanation.

should this happen for all platforms or only for Windows?

Doing it on all platforms won't hurt, I think. But I didn't test it myself. You could play with my Bug575787_Tree_SetItemCount_Perf snippet (already in SWT)

jukzi · 2023-11-28T14:58:16Z

I suggest adding a code comment with explanation.

i added comments and reverse order

SyntevoAlex · 2023-11-28T16:06:03Z

Good. Note that I didn't test anything.

SyntevoAlex

Looks reasonable to me, but I didn't test it.

jukzi · 2023-11-28T17:18:34Z

I also added the setRedraw(false) trick, which cuts downs setImage() and setText() time when i sort the Problems view with many elements. for example ~4000 elements org.eclipse.swt.widgets.Display.readAndDispatch () reduces from 1300ms to 600ms

basilevs · 2023-11-28T20:17:44Z

bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/TreeViewer.java

 			super.preservingSelection(updateCode, reveal);
+			tree.setRedraw(true);


Should setRedraw(true) be in a finally block?

basilevs · 2023-11-28T20:49:39Z

I've tried the test reproducing the problem I'm working on from #649

GTK

No regressions

Before change

210 ms to process 10000 items
202 ms to process 100000 items
The time to select scales as O(n^-0.02) as the size of the model grows from 10000 to 100000
1160 ms to process 10000 items
83267 ms to process 100000 items
The time to reveal scales as O(n^1.86) as the size of the model grows from 10000 to 100000

After change

196 ms to process 10000 items
399 ms to process 100000 items
The time to select scales as O(n^0.31) as the size of the model grows from 10000 to 100000
1260 ms to process 10000 items
86350 ms to process 100000 items
The time to reveal scales as O(n^1.84) as the size of the model grows from 10000 to 100000

MacOS

Slowdown?

Before the change

53 ms to process 10000 items
430 ms to process 100000 items
The time to select scales as O(n^0.90) as the size of the model grows from 10000 to 100000
38 ms to process 10000 items
381 ms to process 100000 items
The time to reveal scales as O(n^1.00) as the size of the model grows from 10000 to 100000

After the change

110 ms to process 10000 items
1005 ms to process 100000 items
The time to select scales as O(n^0.96) as the size of the model grows from 10000 to 100000
91 ms to process 10000 items
944 ms to process 100000 items
The time to reveal scales as O(n^1.01) as the size of the model grows from 10000 to 100000

basilevs · 2023-11-28T20:56:47Z

MacOS increased tree size

The change definitely increases a time to reveal an item in a large tree

Before

413 ms to process 100000 items
4265 ms to process 1000000 items
The time to select scales as O(n^1.01) as the size of the model grows from 100000 to 1000000
389 ms to process 100000 items
4133 ms to process 1000000 items
The time to reveal scales as O(n^1.03) as the size of the model grows from 100000 to 1000000

After

969 ms to process 100000 items
9748 ms to process 1000000 items
The time to select scales as O(n^1.00) as the size of the model grows from 100000 to 1000000
963 ms to process 100000 items
9959 ms to process 1000000 items
The time to reveal scales as O(n^1.01) as the size of the model grows from 100000 to 1000000

jukzi · 2023-11-28T20:58:45Z

The change definitely increases a time to reveal an item in a large tree

@SyntevoAlex any idea?
@basilevs can you please share the testprogram? i want to try it on windows

basilevs · 2023-11-28T20:59:26Z

@Juksi #649

https://github.com/eclipse-platform/eclipse.platform.ui/files/11019153/bug516281.zip

jukzi · 2023-11-28T21:04:40Z

@Juksi #649

jukZi :-)
thanks,. can you post measurements if you combine this PR with 649? all platforms, before+after
This PR contains 3 different optimizations:

move setExpanded() after expanding the children
insert every item at index 0 (in reverse order)
setRedraw

can you please figure out which of them does cause regression on Mac?

basilevs · 2023-11-28T21:18:58Z

can you post measurements if you combine this PR with 649?

#649 is WIP, there is nothing to combine, I've tested on master and on your branch

all platforms, before+after

I don't have Windows env and have already covered Linux and MacOS

This PR contains 3 different optimizations:
* move setExpanded() after expanding the children
* insert every item at index 0 (in reverse order)
* setRedraw
can you please figure out which of them does cause regression on Mac?

I've removed new setRedraw() lines from TreeViewer and got old performance:

395 ms to process 100000 items
4172 ms to process 1000000 items
The time to select scales as O(n^1.02) as the size of the model grows from 100000 to 1000000
366 ms to process 100000 items
4070 ms to process 1000000 items
The time to reveal scales as O(n^1.05) as the size of the model grows from 100000 to 1000000

This is a major surprise - I assumed setRedraw() was a natural wrapper for any batch operations.

basilevs · 2023-11-28T21:26:35Z

Performance tests do not work, but are necessary. Life is pain.

jukzi · 2023-11-30T12:09:33Z

on windows:

before PR:
3 ms to process 10000 items
6 ms to process 100000 items
The time to select scales as O(n^0,24) as the size of the model grows from 10000 to 100000
1094 ms to process 10000 items
10413 ms to process 100000 items
The time to reveal scales as O(n^0,98) as the size of the model grows from 10000 to 100000

without redraw(false)
3 ms to process 10000 items
6 ms to process 100000 items
The time to select scales as O(n^0,27) as the size of the model grows from 10000 to 100000
995 ms to process 10000 items
9993 ms to process 100000 items
The time to reveal scales as O(n^1,00) as the size of the model grows from 10000 to 100000

with redraw(false)
5 ms to process 10000 items
13 ms to process 100000 items
The time to select scales as O(n^0,38) as the size of the model grows from 10000 to 100000
3105 ms to process 10000 items
30691 ms to process 100000 items
The time to reveal scales as O(n^0,99) as the size of the model grows from 10000 to 100000

=> that test can not show that this PR can make some things faster, but also no regression
=> with this test redraw(false) is not good. probably because the tested view is much less complicated then the Problem View i measured with. So probably it's better to use redraw(false) not in general but may be in Problem View

basilevs · 2023-11-30T13:05:17Z

@jukzi reveal() operation is not used in Problem View. Project Explorer or Outline views are a better match for my test.

jukzi · 2023-11-30T15:45:19Z

let forget about redraw(false), it's already set in org.eclipse.ui.internal.views.markers.UIUpdateJob.runInUIThread(IProgressMonitor)

basilevs · 2023-12-01T08:46:00Z

@jukzi have you tried using TreeItem.setItemCount() instead of creating items one-by-one? IMO, it should dramatically increase performance.

basilevs · 2023-12-01T10:13:20Z

newItem() is already not used in org.eclipse.jface.viewers.TreeViewer.setChildCount(Object, int) used by lazy TreeViewer and likely in other places.

basilevs · 2023-12-01T10:20:44Z

None of new createTreeItem() lines are called in the test. So problem is probably with move of setExpanded()

UPDATE: confirmed, moving it to the old spot fixes one test (I only check one test)

jukzi · 2023-12-01T10:36:15Z

UPDATE: confirmed, moving it to the old spot fixes one test (I only check one test)

i will split that commit

basilevs · 2023-12-01T10:51:51Z

There is a hint in Tree:
https://github.com/basilevs/eclipse.platform.swt/blob/4f69de6866723e1e139a5762d993c5dabf2c4799/bundles/org.eclipse.swt/Eclipse%20SWT/gtk/org/eclipse/swt/widgets/Tree.java#L2545-L2566

long gtk_row_has_child_toggled (long model, long path, long iter) {
	/*
	* Feature in GTK. The expanded state of a row that lost
	* its children is not persisted by GTK. So, the row
	* doesn't exhibit the expanded state after obtaining the
	* children. The fix is to preserve the expanded state
	* and use this callback, as it is invoked when a row has
	* gotten the first child row or lost its last child row.
	*/
	int [] index = new int [1];
	GTK.gtk_tree_model_get (modelHandle, iter, ID_COLUMN, index, -1);
	if (index [0] >= items.length) return 0;
	TreeItem item = items [index [0]];
	if (item == null) return 0;
	int childCount = GTK.gtk_tree_model_iter_n_children (modelHandle, item.handle);
	if (childCount != 0 && item.isExpanded) {
		OS.g_signal_handlers_block_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, TEST_EXPAND_ROW);
		GTK.gtk_tree_view_expand_row (handle, path, false);
		OS.g_signal_handlers_unblock_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, TEST_EXPAND_ROW);
	}
	return 0;
}

But the condition was not used during my debugging.

basilevs · 2023-12-01T11:12:41Z

Idea - Tree does not recheck states of children when item is expanded. So if state did not stick to GTK model the first time, when its parent is expanded the expansion state is not reapplied.

Effectively the tree only works as expected when it is manipulated in a way end user would - top to bottom.
This seems to be a bug in GTK Tree implementation, as other platforms persist expanded state of invisible item.

jukzi · 2023-12-01T11:26:21Z

None of new createTreeItem() lines are called in the test. So problem is probably with move of setExpanded()

i created an experiment: https://github.com/eclipse-platform/eclipse.platform.ui/pull/1357/files
it seems to be SLOWER on windows to use setItemCount

jukzi · 2023-12-01T11:28:09Z

This seems to be a bug in GTK Tree implementation, as other platforms persist expanded state of invisible item.

sounds reasonable

jukzi · 2023-12-01T11:30:37Z

only unrelated test errors: #1351

laeubi · 2023-12-01T11:30:50Z

This seems to be a bug in GTK Tree implementation, as other platforms persist expanded state of invisible item.

sounds reasonable

Why is it not a bug in windows that expanding invisible childs is faster than expand visible ones? Just a thought ;-)

jukzi · 2023-12-01T11:32:58Z

Why is it not a bug in windows that expanding invisible childs is faster than expand visible ones? Just a thought ;-)

Invisible things don't need to be drawn.

SyntevoAlex · 2023-12-04T13:05:51Z

linux VirtualLazyTreeViewerTest.testExpandToLevel

The test fails to expand second level of the tree:

but why - @SyntevoAlex ?

I glanced at gtk_tree_view_real_expand_row() in GTK sources and I had the impression that "item is expanded" and "internal data for child items exist" are the same thing. In other words, child items are only created when parent is expanded. This might explain why it's not possible to modify item that is not expanded yet.

If you care to make a pure SWT snippet, I could spend a bit of time to check it with native debugger.

jukzi · 2023-12-05T10:20:48Z

If you care to make a pure SWT snippet, I could spend a bit of time to check it with native debugger.

@basilevs can you create a pure SWT reproducer for GTK?

basilevs · 2023-12-05T18:51:12Z

Following test fails on last line on GTK. If user manually "helps" to expand "item2" node, the test instantly passes.
It does not fail if order of expansions is reversed.

diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Tree.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Tree.java
index 0ad17d5..86ada48 100644
--- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Tree.java
+++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Tree.java
@@ -1188,6 +1188,35 @@
 		new TreeItem(item_0, 0, 0);
 		assertEquals(10, item_0.getItemCount());
 	});
 }
 
+@Test
+public void test_persistExpandStatusForInvisibleItems() throws InterruptedException {
+	tree.dispose();
+	tree = new Tree(shell, SWT.VIRTUAL);
+	setWidget(tree);
+	shell.setLayout(new FillLayout());
+	SwtTestUtil.openShell(shell);
+	int itemCount[] = new int[] {1};
+	List<TreeItem> dataRequests = new ArrayList<>();
+	tree.addListener(SWT.SetData, event -> {
+		TreeItem item = (TreeItem) event.item;
+		dataRequests.add(item);
+		item.setText("item"+itemCount[0]++);
+		item.setItemCount(1);
+	});
+	TreeItem item1 = new TreeItem(tree, SWT.NONE);
+	TreeItem item2 = item1.getItem(0);
+	TreeItem item3 = item2.getItem(0);
+	assertTrue(dataRequests.remove(item1));
+	assertTrue(dataRequests.remove(item2));
+	assertTrue(dataRequests.isEmpty());
+	item2.setExpanded(true);
+	assertTrue(dataRequests.isEmpty());
+	item1.setExpanded(true);
+	// Item 3 is now visible and should request data
+	SwtTestUtil.processEvents(10000, () -> !dataRequests.isEmpty());
+	assertFalse(dataRequests.isEmpty());
+}
+
 }

fails on GTK eclipse-platform/eclipse.platform.ui#1331 (comment)

eclipse-platform/eclipse.platform.swt#901

jukzi · 2023-12-06T16:41:39Z

ignoring random fail #275

If items limit is enabled, clicking on expandable element adds all elements **before** the node, so the sort order is totally wrong and "expandable node" appears "in the middle" of the siblings. However, "hidden and now expanded" elements (if exists) should always be appended **to the end** of the shown children. This is a regression from 5930a51 / eclipse-platform#1331. The problem with the patch above is that it inserts all previously hidden elements at the zero index in the tree, but the tree **has already some children**, so instead of adding new elements below existing, the patch prepends them. Since eclipse-platform#1331 consists of only two changes (changed tree item creation order on expand and updated javadoc), this is revert the former one. Fixes eclipse-platform#1417

If items limit is enabled, clicking on expandable element adds all elements **before** the node, so the sort order is totally wrong and "expandable node" appears "in the middle" of the siblings. However, "hidden and now expanded" elements (if exists) should always be appended **to the end** of the shown children. This is a regression from 5930a51 / #1331. The problem with the patch above is that it inserts all previously hidden elements at the zero index in the tree, but the tree **has already some children**, so instead of adding new elements below existing, the patch prepends them. Since #1331 consists of only two changes (changed tree item creation order on expand and updated javadoc), this is revert the former one. Fixes #1417

fails on GTK eclipse-platform/eclipse.platform.ui#1331 (comment)

If items limit is enabled, clicking on expandable element adds all elements **before** the node, so the sort order is totally wrong and "expandable node" appears "in the middle" of the siblings. However, "hidden and now expanded" elements (if exists) should always be appended **to the end** of the shown children. This is a regression from 5930a51 / eclipse-platform#1331. The problem with the patch above is that it inserts all previously hidden elements at the zero index in the tree, but the tree **has already some children**, so instead of adding new elements below existing, the patch prepends them. Since eclipse-platform#1331 consists of only two changes (changed tree item creation order on expand and updated javadoc), this is revert the former one. Fixes eclipse-platform#1417

jukzi requested a review from SyntevoAlex November 27, 2023 07:32

jukzi force-pushed the Tree_expand branch from 4d314f0 to 5cc6cb6 Compare November 28, 2023 14:57

SyntevoAlex reviewed Nov 28, 2023

View reviewed changes

jukzi force-pushed the Tree_expand branch from de648f5 to 9de1e2a Compare November 28, 2023 17:15

basilevs reviewed Nov 28, 2023

View reviewed changes

jukzi force-pushed the Tree_expand branch from 9de1e2a to 726b26b Compare November 30, 2023 11:52

jukzi force-pushed the Tree_expand branch 2 times, most recently from 0458de6 to 73fb5e9 Compare November 30, 2023 15:43

jukzi requested a review from basilevs November 30, 2023 15:47

basilevs approved these changes Nov 30, 2023

View reviewed changes

jukzi force-pushed the Tree_expand branch from 73fb5e9 to d1c5aed Compare December 1, 2023 08:07

jukzi force-pushed the Tree_expand branch from d1c5aed to bda8dc4 Compare December 1, 2023 10:45

jukzi force-pushed the Tree_expand branch from bda8dc4 to 4ad8433 Compare December 5, 2023 10:19

jukzi force-pushed the Tree_expand branch from 4ad8433 to eee7c5e Compare December 5, 2023 17:02

jukzi pushed a commit to jukzi/eclipse.platform.swt that referenced this pull request Dec 6, 2023

[test] add Test for expand of invisible TreeItem eclipse-platform#901

1086adf

fails on GTK eclipse-platform/eclipse.platform.ui#1331 (comment)

jukzi mentioned this pull request Dec 6, 2023

[test] add Test for expand of invisible TreeItem #901 eclipse-platform/eclipse.platform.swt#933

Closed

jukzi pushed a commit to jukzi/eclipse.platform.swt that referenced this pull request Dec 6, 2023

[test] add Test for expand of invisible TreeItem eclipse-platform#901

bbf4ec6

fails on GTK eclipse-platform/eclipse.platform.ui#1331 (comment)

[performance] Tree expand in reverse order - eclipse.platform.swt#901

727c6be

eclipse-platform/eclipse.platform.swt#901

jukzi force-pushed the Tree_expand branch from 487a18e to 727c6be Compare December 6, 2023 14:01

jukzi merged commit 5930a51 into eclipse-platform:master Dec 6, 2023

jukzi deleted the Tree_expand branch December 6, 2023 16:41

iloveeclipse mentioned this pull request Dec 14, 2023

Expandable node is shown at wrong place, sorting broken after expand #1417

Closed

iloveeclipse mentioned this pull request Dec 14, 2023

Expandable node is shown at wrong place, sorting broken after expand #1418

Merged

HannesWell pushed a commit to jukzi/eclipse.platform.swt that referenced this pull request Jan 8, 2024

[test] add Test for expand of invisible TreeItem eclipse-platform#901

985f5ea

fails on GTK eclipse-platform/eclipse.platform.ui#1331 (comment)

		super.preservingSelection(updateCode, reveal);
		tree.setRedraw(true);

[performance] faster Tree expand - eclipse.platform.swt#901 #1331

[performance] faster Tree expand - eclipse.platform.swt#901 #1331

Conversation

jukzi commented Nov 27, 2023 • edited Loading

laeubi commented Nov 27, 2023

github-actions bot commented Nov 27, 2023 • edited Loading

Test Results

akurtakov commented Nov 27, 2023

laeubi commented Nov 27, 2023

SyntevoAlex commented Nov 28, 2023

jukzi commented Nov 28, 2023

SyntevoAlex commented Nov 28, 2023

SyntevoAlex left a comment

Choose a reason for hiding this comment

jukzi commented Nov 28, 2023

basilevs Nov 28, 2023

Choose a reason for hiding this comment

basilevs commented Nov 28, 2023 • edited Loading

GTK

Before change

After change

MacOS

Before the change

After the change

basilevs commented Nov 28, 2023

MacOS increased tree size

Before

After

jukzi commented Nov 28, 2023

basilevs commented Nov 28, 2023

jukzi commented Nov 28, 2023

basilevs commented Nov 28, 2023 • edited Loading

basilevs commented Nov 28, 2023

jukzi commented Nov 30, 2023

basilevs commented Nov 30, 2023

jukzi commented Nov 30, 2023

basilevs commented Dec 1, 2023 • edited Loading

basilevs commented Dec 1, 2023

basilevs commented Dec 1, 2023 • edited Loading

jukzi commented Dec 1, 2023

basilevs commented Dec 1, 2023 • edited Loading

basilevs commented Dec 1, 2023

jukzi commented Dec 1, 2023

jukzi commented Dec 1, 2023

jukzi commented Dec 1, 2023

laeubi commented Dec 1, 2023

jukzi commented Dec 1, 2023

SyntevoAlex commented Dec 4, 2023

jukzi commented Dec 5, 2023

basilevs commented Dec 5, 2023 • edited Loading

jukzi commented Dec 6, 2023

jukzi commented Nov 27, 2023 •

edited

Loading

github-actions bot commented Nov 27, 2023 •

edited

Loading

basilevs commented Nov 28, 2023 •

edited

Loading

basilevs commented Nov 28, 2023 •

edited

Loading

basilevs commented Dec 1, 2023 •

edited

Loading

basilevs commented Dec 1, 2023 •

edited

Loading

basilevs commented Dec 1, 2023 •

edited

Loading

basilevs commented Dec 5, 2023 •

edited

Loading