Skip to content

Commit

Permalink
Revert WeakHashMap children cache as it slows down on large
Browse files Browse the repository at this point in the history 
datasets eclipse-platform#882

Test_org_eclipse_swt_widgets_Tree.test_getItemNoGrowth() was failing for
virtual trees.
  • Loading branch information
@basilevs
basilevs committed Nov 12, 2023
1 parent 5577230 commit eaa5e60
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 42 deletions.
9 changes: 5 additions & 4 deletions bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
package org.eclipse.swt.widgets;


import java.util.*;

import org.eclipse.swt.*;
import org.eclipse.swt.events.*;
import org.eclipse.swt.graphics.*;
Expand Down Expand Up @@ -113,7 +111,7 @@ public class Tree extends Composite {

private long headerCSSProvider;

private final WeakHashMap<TreeItem, TreeItemCache> itemCache = new WeakHashMap<>();
private TreeItemCache itemCache = null;

static final int ID_COLUMN = 0;
static final int CHECKED_COLUMN = 1;
Expand Down Expand Up @@ -4352,6 +4350,9 @@ public void dispose() {
}

TreeItemCache getItemCache(TreeItem treeItem) {
return itemCache.computeIfAbsent(treeItem, TreeItemCache::new);
if (itemCache == null || itemCache.owner != treeItem) {
itemCache = new TreeItemCache(treeItem);
}
return itemCache;
}
}
125 changes: 87 additions & 38 deletions ....swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Tree.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;
import java.util.function.IntFunction;

import org.eclipse.swt.SWT;
import org.eclipse.swt.events.TreeListener;
Expand Down Expand Up @@ -1190,23 +1192,11 @@ public void test_setItemCount_itemCount2() {
});
}

private long measureGetItemNanos(int childCount) {
private double measureGetItemNanos(int childCount) {
tree.setItemCount(1);
TreeItem parent = tree.getItem(0);
parent.setItemCount(childCount);
// warmup
for (int i = 0; i < 10000; i++) {
System.nanoTime();
assertNotNull(parent.getItem(childCount - 1));
}

long start = System.nanoTime();
for (int i = 0; i < 10000; i++) {
parent.getItem(childCount - 1);
}
long stop = System.nanoTime();
long elapsed = stop-start;
return elapsed;
return measureNanos(() -> parent.getItem(childCount - 1));
}

/**
Expand All @@ -1217,48 +1207,107 @@ private long measureGetItemNanos(int childCount) {
public void test_getItemNoGrowth() {
// This test takes 1 second on MacOS
testTreeRegularAndVirtual(() -> {
measureGetItemNanos(1000); // warmup
double elapsed_100 = measureGetItemNanos(100);
double elapsed_100000 = measureGetItemNanos(100000);
double ratio = elapsed_100000 / elapsed_100;
System.out.printf("Time for 100 elements: %f ns\nTime for 100000 elements: %f ns\nRatio: %f\n",elapsed_100, elapsed_100000, ratio);
assertTrue("Execution time should not grow with index", (elapsed_100000 <= 100 && elapsed_100 <= 100) || ratio < 10);
assertConstant("getItem() execution time", this::measureGetItemNanos);
});
}

private long measureGetItemCountNanos(int childCount) {
private double measureGetItemCountNanos(int childCount) {
tree.setItemCount(1);
TreeItem parent = tree.getItem(0);
parent.setItemCount(childCount);
// warmup
for (int i = 0; i < 10000; i++) {
return measureNanos(parent::getItemCount);
}

/**
* Execution time of <code> TreeItem.getItemCount()</code> should not depend on child count.
* @see https://github.com/eclipse-platform/eclipse.platform.swt/issues/882
*/
@Test
public void test_getItemCountNoGrowth() {
// This test takes 1 second on MacOS
testTreeRegularAndVirtual(() -> {
assertConstant("itemCount execution time", this::measureGetItemCountNanos);
});
}


void buildBinaryTree(TreeItem parent, int totalChildCount) {
if (totalChildCount <= 0) {
return;
}
int leftCount = totalChildCount / 2;
int rightCount = totalChildCount - leftCount;
if (leftCount > 1) {
buildBinaryTree(new TreeItem(parent, SWT.NONE), leftCount - 1);
}
if (rightCount > 1) {
buildBinaryTree(new TreeItem(parent, SWT.NONE), rightCount - 1);
}
}

void depthFirstTraverse(TreeItem parent) {
int count = parent.getItemCount();
if (count <= 0) {
return;
}
for (int i = 0; i < count ; i++) {
depthFirstTraverse(parent.getItem(i));
}
}

/**
* Measures time required to do one operation
* @return nanoseconds
*/
private double measureNanos(Runnable operation) {
// warmup and calibration - we measure, how many iterations we can approximately do in a second

long warmupStop = System.nanoTime() + TimeUnit.SECONDS.toNanos(1);
long iterationCount = 0;
while (System.nanoTime() < warmupStop) {
System.nanoTime();
assertNotNull(parent.getItemCount());
operation.run();
iterationCount++;
}

long start = System.nanoTime();
for (int i = 0; i < 10000; i++) {
parent.getItemCount();
for (int i = 0; i < iterationCount; i++) {
operation.run();
}
long stop = System.nanoTime();
long elapsed = stop-start;
return elapsed;
return ((double)elapsed) / iterationCount;
}

private double measureDepthFirstTraverse(int totalChildCount) {
TreeItem root = new TreeItem(tree, SWT.NONE);
buildBinaryTree(root, totalChildCount - 1);
return measureNanos(() -> depthFirstTraverse(root));
}

private void assertConstant(String message, IntFunction<Double> function) {
function.apply(1000); // warmmup
double elapsed_100 = function.apply(100);
double elapsed_100000 = function.apply(100000);
double ratio = elapsed_100000 / elapsed_100;
String error = String.format( "%s should be constant. But:\nTime for 100 elements: %f ns\nTime for 100000 elements: %f ns\nRatio: %f\n", message, elapsed_100, elapsed_100000, ratio);
assertTrue(error, (elapsed_100000 <= 100 && elapsed_100 <= 100) || ratio < 10);
}

private void assertLinear(String message, IntFunction<Double> function) {
function.apply(1000); // warmmup
double elapsed_100 = function.apply(100);
double elapsed_100000 = function.apply(100000);
double ratio = elapsed_100000 / elapsed_100;
String error = String.format( "%s should be linear. But:\nTime for 100 elements: %f ns\nTime for 100000 elements: %f ns\nRatio: %f\n", message, elapsed_100, elapsed_100000, ratio);
assertTrue(error, (elapsed_100000 <= 100 && elapsed_100 <= 100) || ratio < 10000);
}

/**
* Execution time of <code> TreeItem.getItemCount()</code> should not depend on child count.
* @see https://github.com/eclipse-platform/eclipse.platform.swt/issues/882
*/
@Test
public void test_getItemCountNoGrowth() {
public void test_depthFirstTraversalLinearGrowth() {
// This test takes 1 second on MacOS
testTreeRegularAndVirtual(() -> {
measureGetItemNanos(1000); // warmup
double elapsed_100 = measureGetItemCountNanos(100);
double elapsed_100000 = measureGetItemCountNanos(100000);
double ratio = elapsed_100000 / elapsed_100;
System.out.printf("Time for 100 elements: %f ns\nTime for 100000 elements: %f ns\nRatio: %f\n",elapsed_100, elapsed_100000, ratio);
assertTrue("Execution time should not grow with child count", (elapsed_100000 <= 100 && elapsed_100 <= 100) || ratio < 10);
assertLinear("Depth first traversal", this::measureDepthFirstTraverse);
});
}

Expand Down

0 comments on commit eaa5e60

Please sign in to comment.