Skip to content

Conversation

@basilevs
Copy link
Contributor

@basilevs basilevs commented Nov 11, 2023

This alleviates #882, by caching results of heavy operations.

On Linux, methods TreeItem.getItemCount() and TreeItem.getItem(int) take time proportional to child count (have linear asymptotic complexity). This causes quadratic execution time in JFace TreeViewer.reveal() method, which uses these to eagerly populate expanded virtual Tree nodes.

This PR demonstrates the problem with corresponding JUnit tests and suggests an incomplete solution, that relies on caching.
An alternative solution is discussed in #882 to reimplement SWT GTK child tracking in a manner similar to that of Mac OS implementation, so this proposal should not be merged, if anyone is ready to implement a better and more complete solution.

…pse-platform#882

On Linux these methods have linear complexity over child count, that
causes quadratic complexity for virtual tree navigation.
…pse-platform#882

On Linux these methods have linear complexity over child count, that
causes quadratic complexity for virtual tree navigation.
@github-actions
Copy link
Contributor

github-actions bot commented Nov 11, 2023

Test Results

     299 files  ±0       299 suites  ±0   7m 40s ⏱️ + 1m 31s
  4 098 tests +3    4 090 ✔️ +3    8 💤 ±0  0 ±0 
12 206 runs  +9  12 133 ✔️ +9  73 💤 ±0  0 ±0 

Results for commit fbdd86b. ± Comparison against base commit eafdfbd.

♻️ This comment has been updated with latest results.

@basilevs
Copy link
Contributor Author

basilevs commented Nov 11, 2023

I've implemented a straightforward caching of item count. It helps in getItemCount() but not in getItem().
getItem() asks GTK to fetch the n-th child and org.eclipse.swt.internal.gtk.GTK.gtk_tree_model_iter_nth_child(long, long, long, int) has linear execution time.

static gboolean
gtk_tree_store_iter_nth_child (GtkTreeModel *tree_model,
			       GtkTreeIter  *iter,
			       GtkTreeIter  *parent,
			       gint          n)
{
  GtkTreeStore *tree_store = (GtkTreeStore *) tree_model;
  GtkTreeStorePrivate *priv = tree_store->priv;
  GNode *parent_node;
  GNode *child;

  g_return_val_if_fail (parent == NULL || parent->user_data != NULL, FALSE);

  if (parent == NULL)
    parent_node = priv->root;
  else
    parent_node = parent->user_data;

  child = g_node_nth_child (parent_node, n);

  if (child)
    {
      iter->user_data = child;
      iter->stamp = priv->stamp;
      return TRUE;
    }
  else
    {
      iter->stamp = 0;
      return FALSE;
    }
}

g_node_nth_child is linear:

GNode*
g_node_nth_child (GNode *node,
		  guint	 n)
{
  g_return_val_if_fail (node != NULL, NULL);
  
  node = node->children;
  if (node)
    while ((n-- > 0) && node)
      node = node->next;
  
  return node;
}

So at best I've reduced time in Jface case of interest by half, not by a thousand times.

UPDATE:
getId() in Tree._getItem() is also O(N)

UPDATE 2:
I've cached getItem() too.

TreeItem.getItem(int) had linear complexity, full traversal had
quadratic complexity. To alleviate this, we cache the result.

Note: this only speeds up breadth-first traversal as cache operates on "item of interest" basis, dropping contents if another item is requested.
datasets eclipse-platform#882

Test_org_eclipse_swt_widgets_Tree.test_getItemNoGrowth() was failing for
virtual trees.
@basilevs
Copy link
Contributor Author

The immediate problem is solved, but in a strange way. There is a suggestion to abolish GTK queires for children entirely and track parent-child relationship in Java code like MacOS implementation does. I'm not ready to work on that, as scope is much greater, so I publish my current hackish solution for review and discussion.

@basilevs basilevs changed the title Test quick execution of TreeItem.getItemCount() #882 Speed up TreeItem.getItemCount() and TreeItem.getItemCount() #882 Nov 13, 2023
@basilevs basilevs marked this pull request as ready for review November 13, 2023 10:13
…-platform#882

As assertion now works with a time require for a single operation, we can't assume that non-constant operation will not fit in 100 ns window.
Tests have been modified to use time limits instead of fixed iteration
count. Now each test takes 7-14 seconds on Mac OS.
Any algorithm would be fast on a binary tree due to low count of items
in each node. We need a wide tree to break traversal tests.
Wide tree traversal shows performance 4 times worse than optimal on wide
trees.
Breadth first traversal is somewhat similar lazy TreeViewer from JFace
does on reveal - it populates a whole level of a tree, then select an
item and goes down a level.
@basilevs basilevs marked this pull request as draft November 14, 2023 05:46
@basilevs
Copy link
Contributor Author

Sorry, I've forgot to test the JFace reveal test from eclipse-platform/eclipse.platform.ui#649 Back to draft.

@basilevs
Copy link
Contributor Author

basilevs commented Nov 14, 2023

JFace reveal is still quadratic:

1153 ms to process 10000 items
79621 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

My tests probably do not adequately represent JFace interaction with Tree.

@basilevs
Copy link
Contributor Author

I give up for now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant