From 8ca1d02592eba57819434d2b262a9922903f09aa Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Thu, 13 Aug 2026 14:57:24 +0200 Subject: [PATCH] Hash tree paths incrementally when restoring the expansion state Restoring the expansion state walks every item of the tree and looks each one up in a table keyed by TreePath. A tree path hashes all of its segments, so an element whose hashCode is recursive over its children, as an LSP DocumentSymbol is, was hashed once for every descendant of every ancestor on its path. The cost of the walk grew with the square of the tree rather than with its size. A tree path hashes as the sum of its segments, which composes, so internalSetExpandedTreePaths now carries the parent's hash down the recursion and derives each child's from it with a single element hash. Once the set of paths to expand is exhausted the remaining items can only be collapsed, which needs neither a path nor its hash, so the walk switches to a plain collapse. CustomHashtable keeps the hash in its entries, which lets a lookup rule out an entry without comparing the keys and lets the table grow without hashing the keys again, and gains a remove that takes an already computed hash. Measured with TreeViewerExpansionTest, recursive hash visits and the minimum of 100 restores: fully expanded, 156 of 1884 elements 361056 -> 44676 visits, 8.3 -> 5.1 ms 2 of 1803 elements, all materialized 1087807 -> 4206 visits, 10.2 -> 0.2 ms --- .../jface/viewers/AbstractTreeViewer.java | 35 ++++++++-- .../jface/viewers/CustomHashtable.java | 69 +++++++++++++------ 2 files changed, 80 insertions(+), 24 deletions(-) diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java index cb02a29cd77..aeb30120d57 100644 --- a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java +++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java @@ -2202,19 +2202,34 @@ private void internalSetExpanded(CustomHashtable expandedElements, * which are expanded * @param widget * the widget + * @param currentHash + * the hash {@link TreePath#hashCode(IElementComparer)} would return + * for currentPath */ private void internalSetExpandedTreePaths( CustomHashtable expandedTreePaths, Widget widget, - TreePath currentPath) { + TreePath currentPath, int currentHash, IElementComparer comparer) { Item[] items = getChildren(widget); for (Item item : items) { + if (expandedTreePaths.size() == 0) { + // Every path that had to be expanded has been found, so the rest of the + // tree can only be collapsed, which needs neither a path nor its hash. + internalCollapseSubtree(item); + continue; + } Object data = item.getData(); TreePath childPath = data == null ? null : currentPath .createChildPath(data); + int childHash = currentHash; if (data != null && childPath != null) { + // A tree path hashes as the sum of its segments, so the child's hash + // follows from the parent's. Computing it from the path instead would + // hash every segment again, which is what makes an element with an + // expensive hashCode cost the whole traversal. + childHash += comparer == null ? data.hashCode() : comparer.hashCode(data); // remove the element to avoid an infinite loop // if the same element appears on a child item - boolean expanded = expandedTreePaths.remove(childPath) != null; + boolean expanded = expandedTreePaths.remove(childPath, childHash) != null; if (expanded != getExpanded(item)) { if (expanded) { createChildren(item); @@ -2222,7 +2237,19 @@ private void internalSetExpandedTreePaths( setExpanded(item, expanded); } } - internalSetExpandedTreePaths(expandedTreePaths, item, childPath); + internalSetExpandedTreePaths(expandedTreePaths, item, childPath, childHash, comparer); + } + } + + /** + * Collapses the given item and everything below it. + */ + private void internalCollapseSubtree(Item item) { + if (item.getData() != null && getExpanded(item)) { + setExpanded(item, false); + } + for (Item child : getChildren(item)) { + internalCollapseSubtree(child); } } @@ -2677,7 +2704,7 @@ public int hashCode(Object element) { // equal elements, and those are in the set of elements to be expanded, // only the first item found for each element will be expanded. internalSetExpandedTreePaths(expandedTreePaths, getControl(), - new TreePath(new Object[0])); + new TreePath(new Object[0]), 0, comparer); } /** diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/CustomHashtable.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/CustomHashtable.java index 164c148b241..4c46c28ae1f 100644 --- a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/CustomHashtable.java +++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/CustomHashtable.java @@ -36,11 +36,19 @@ private static class HashMapEntry { Object key, value; + /** + * The key's hash, kept so that growing the table does not hash the keys again + * and so that a lookup can rule out an entry without comparing the keys. Both + * matter for elements whose hashCode and equals are expensive. + */ + final int hash; + HashMapEntry next; - HashMapEntry(Object theKey, Object theValue) { + HashMapEntry(Object theKey, Object theValue, int theHash) { key = theKey; value = theValue; + hash = theHash; } } @@ -239,22 +247,15 @@ public Enumeration elements() { * does not exist */ public Object get(Object key) { - int index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; - HashMapEntry entry = elementData[index]; - while (entry != null) { - if (keyEquals(key, entry.key)) { - return entry.value; - } - entry = entry.next; - } - return null; + HashMapEntry entry = getEntry(key); + return entry == null ? null : entry.value; } private HashMapEntry getEntry(Object key) { - int index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; - HashMapEntry entry = elementData[index]; + int hash = hashCode(key); + HashMapEntry entry = elementData[indexFor(hash)]; while (entry != null) { - if (keyEquals(key, entry.key)) { + if (entry.hash == hash && keyEquals(key, entry.key)) { return entry; } entry = entry.next; @@ -262,6 +263,13 @@ private HashMapEntry getEntry(Object key) { return null; } + /** + * Answers the slot the given hash belongs into. + */ + private int indexFor(int hash) { + return (hash & 0x7FFFFFFF) % elementData.length; + } + /** * Answers the hash code for the given key. */ @@ -308,15 +316,16 @@ public Enumeration keys() { */ public Object put(Object key, Object value) { if (key != null && value != null) { - int index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; + int hash = hashCode(key); + int index = indexFor(hash); HashMapEntry entry = elementData[index]; - while (entry != null && !keyEquals(key, entry.key)) { + while (entry != null && !(entry.hash == hash && keyEquals(key, entry.key))) { entry = entry.next; } if (entry == null) { if (++elementCount > threshold) { rehash(); - index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; + index = indexFor(hash); } if (index < firstSlot) { firstSlot = index; @@ -324,7 +333,7 @@ public Object put(Object key, Object value) { if (index > lastSlot) { lastSlot = index; } - entry = new HashMapEntry(key, value); + entry = new HashMapEntry(key, value, hash); entry.next = elementData[index]; elementData[index] = entry; return null; @@ -352,7 +361,7 @@ private void rehash() { for (int i = elementData.length; --i >= 0;) { HashMapEntry entry = elementData[i]; while (entry != null) { - int index = (hashCode(entry.key) & 0x7FFFFFFF) % length; + int index = (entry.hash & 0x7FFFFFFF) % length; if (index < firstSlot) { firstSlot = index; } @@ -377,10 +386,30 @@ private void rehash() { * did not exist */ public Object remove(Object key) { + if (elementCount == 0) { + return null; + } + return remove(key, hashCode(key)); + } + + /** + * Removes the key/value pair for the given key, whose hash the caller has + * already computed. The hash must be the one this table's comparer would + * produce for the key. + * + * @param key the key to remove + * @param hash the key's hash + * @return the value associated with the key, or null if the key + * did not exist + */ + public Object remove(Object key, int hash) { + if (elementCount == 0) { + return null; + } HashMapEntry last = null; - int index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; + int index = indexFor(hash); HashMapEntry entry = elementData[index]; - while (entry != null && !keyEquals(key, entry.key)) { + while (entry != null && !(entry.hash == hash && keyEquals(key, entry.key))) { last = entry; entry = entry.next; }