Search view: performance issues on remove and sort #3735

- Implemented a batch removal of search matches
- Search matches are removed from their parent, instead of one by one
which causes performance issues
- It's also fixing the problem, that all elements are removed instead
only the visible elements (while all others are filtered by the limit
number)
- Replaces the
ConcurrentHashMap.newKeySet() with the ConcurrentSkipListSet with the
comparator and makes the additional sorting obsolete.
- After this change, avoid using `size()` on values, because it is not
more constant time operation, see `ConcurrentSkipListSet` javadoc.
- Also comparator must be changed to consider different Match elements
with same size and offsets in the set - otherwise the
`ConcurrentSkipListSet` would lost them.
- Added `AbstractTextSearchResult.hasMatches(Object element)` API for
cases where match count not needed.
- If possible, calls to `size()` changed to `isEmpty()` or omitted.
- Implemented Tests

Fixes #3735

Co-authored-by: Andrey Loskutov <loskutov@gmx.de>

Author: mehmet-karaman

bundles/org.eclipse.search/META-INF/MANIFEST.MF

@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.search; singleton:=true
-Bundle-Version: 3.18.100.qualifier
+Bundle-Version: 3.19.0.qualifier
 Bundle-Activator: org.eclipse.search.internal.ui.SearchPlugin
 Bundle-ActivationPolicy: lazy
 Bundle-Vendor: %providerName

bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java

@@ -14,7 +14,6 @@
 package org.eclipse.search.ui.text;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Enumeration;
@@ -23,8 +22,10 @@
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentSkipListSet;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
 
 import org.eclipse.search.ui.ISearchResult;
 import org.eclipse.search.ui.ISearchResultListener;
@@ -44,6 +45,8 @@ public abstract class AbstractTextSearchResult implements ISearchResult {
 	private final ConcurrentMap<Object, Set<Match>> fElementsToMatches;
 	private final List<ISearchResultListener> fListeners;
 	private final AtomicInteger matchCount;
+	private final ConcurrentHashMap<Match, Long> collisionOrder;
+	private final AtomicLong collisionCounter;
 
 	private MatchFilter[] fMatchFilters;
 
@@ -55,6 +58,8 @@ protected AbstractTextSearchResult() {
 		fListeners = new CopyOnWriteArrayList<>();
 		matchCount = new AtomicInteger(0);
 		fMatchFilters= null; // filtering disabled by default
+		collisionOrder = new ConcurrentHashMap<>();
+		collisionCounter = new AtomicLong(0);
 	}
 
 	/**
@@ -78,9 +83,7 @@ public Match[] getMatches(Object element) {
 		}
 		Set<Match> matches = fElementsToMatches.get(element);
 		if (matches != null) {
-			Match[] sortingCopy = matches.toArray(new Match[matches.size()]);
-			Arrays.sort(sortingCopy, AbstractTextSearchResult::compare);
-			return sortingCopy;
+			return matches.toArray(new Match[0]);
 		}
 		return EMPTY_ARRAY;
 	}
@@ -161,15 +164,36 @@ private MatchEvent getSearchResultEvent(Collection<Match> matches, int eventKind
 	private boolean didAddMatch(Match match) {
 		matchCount.set(0);
 		updateFilterState(match);
-		return fElementsToMatches.computeIfAbsent(match.getElement(), k -> ConcurrentHashMap.newKeySet()).add(match);
+		return fElementsToMatches.computeIfAbsent(match.getElement(),
+				k -> new ConcurrentSkipListSet<>(this::compare)).add(match);
 	}
 
-	private static int compare(Match match2, Match match1) {
-		int diff= match2.getOffset()-match1.getOffset();
+	private int compare(Match match2, Match match1) {
+		if (match1 == match2) {
+			return 0;
+		}
+		int diff = Integer.compare(match2.getOffset(), match1.getOffset());
+		if (diff != 0) {
+			return diff;
+		}
+		diff = Integer.compare(match2.getLength(), match1.getLength());
 		if (diff != 0) {
 			return diff;
 		}
-		return match2.getLength()-match1.getLength();
+		diff = Integer.compare(System.identityHashCode(match2), System.identityHashCode(match1));
+		if (diff != 0) {
+			return diff;
+		}
+		// Identity hash collision for two distinct objects: use a stable
+		// tiebreaker so the set does not treat them as duplicates.
+		return resolveCollision(match2, match1);
+	}
+
+	@SuppressWarnings("boxing")
+	private int resolveCollision(Match a, Match b) {
+		long orderA = collisionOrder.computeIfAbsent(a, k -> collisionCounter.incrementAndGet());
+		long orderB = collisionOrder.computeIfAbsent(b, k -> collisionCounter.incrementAndGet());
+		return Long.compare(orderA, orderB);
 	}
 
 	/**
@@ -185,6 +209,7 @@ public void removeAll() {
 	private void doRemoveAll() {
 		matchCount.set(0);
 		fElementsToMatches.clear();
+		collisionOrder.clear();
 	}
 
 	/**
@@ -233,6 +258,9 @@ private boolean didRemoveMatch(Match match) {
 			}
 			return matches;
 		});
+		if (existed[0]) {
+			collisionOrder.remove(match);
+		}
 		return existed[0];
 	}
 
@@ -333,6 +361,25 @@ public boolean hasMatches() {
 		return false;
 	}
 
+	/**
+	 * Returns whether the given element has any matches in this search result.
+	 *
+	 * @param element
+	 *            the element to test for matches
+	 * @return {@code true} if the given element has at least one match
+	 * @since 3.19
+	 */
+	public boolean hasMatches(Object element) {
+		if (element == null) {
+			return false;
+		}
+		Set<Match> matches = fElementsToMatches.get(element);
+		if (matches != null) {
+			return !matches.isEmpty();
+		}
+		return false;
+	}
+
 	/**
 	 * Returns the number of matches reported against a given element. This is
 	 * equivalent to calling <code>getMatches(element).length</code>
@@ -442,4 +489,28 @@ public MatchFilter[] getAllMatchFilters() {
 	 * @see IFileMatchAdapter
 	 */
 	public abstract IFileMatchAdapter getFileMatchAdapter();
+
+	/**
+	 * Batch removing of matches by using their enclosing elements.
+	 *
+	 * @param elements
+	 *            the elements of which matches should be removed.
+	 * @since 3.19
+	 */
+	public void removeElements(Collection<?> elements) {
+		matchCount.set(0);
+		List<Match> removedMatches = new ArrayList<>();
+		for (Object object : elements) {
+			Set<Match> matches = fElementsToMatches.remove(object);
+			if (matches != null) {
+				removedMatches.addAll(matches);
+			}
+		}
+		if (!removedMatches.isEmpty()) {
+			if (!collisionOrder.isEmpty()) {
+				removedMatches.forEach(collisionOrder::remove);
+			}
+			fireChange(getSearchResultEvent(removedMatches, MatchEvent.REMOVED));
+		}
+	}
 }

bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java

@@ -18,6 +18,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.LinkedBlockingDeque;
@@ -41,7 +42,9 @@
 import org.eclipse.core.runtime.SafeRunner;
 import org.eclipse.core.runtime.Status;
 
+import org.eclipse.core.resources.IContainer;
 import org.eclipse.core.resources.IFile;
+import org.eclipse.core.resources.IResource;
 
 import org.eclipse.jface.action.Action;
 import org.eclipse.jface.action.IAction;
@@ -61,6 +64,7 @@
 import org.eclipse.jface.viewers.ITreeContentProvider;
 import org.eclipse.jface.viewers.OpenEvent;
 import org.eclipse.jface.viewers.SelectionChangedEvent;
+import org.eclipse.jface.viewers.StructuredSelection;
 import org.eclipse.jface.viewers.StructuredViewer;
 import org.eclipse.jface.viewers.TableViewer;
 import org.eclipse.jface.viewers.TreeViewer;
@@ -645,7 +649,7 @@ protected void postEnsureSelection() {
 
 	private void updateBusyLabel() {
 		AbstractTextSearchResult result = getInput();
-		boolean shouldShowBusy = result != null && NewSearchUI.isQueryRunning(result.getQuery()) && result.getMatchCount() == 0;
+		boolean shouldShowBusy = result != null && NewSearchUI.isQueryRunning(result.getQuery()) && !result.hasMatches();
 		if (shouldShowBusy == fIsBusyShown) {
 			return;
 		}
@@ -1387,6 +1391,10 @@ public void internalRemoveSelected() {
 		StructuredViewer viewer = getViewer();
 		IStructuredSelection selection = viewer.getStructuredSelection();
 
+		selection = handleRemovalOfResources(result, selection);
+		if (selection.isEmpty()) {
+			return;
+		}
 		HashSet<Match> set = new HashSet<>();
 		if (viewer instanceof TreeViewer) {
 			ITreeContentProvider cp = (ITreeContentProvider) viewer.getContentProvider();
@@ -1401,6 +1409,35 @@ public void internalRemoveSelected() {
 		result.removeMatches(matches);
 	}
 
+	private IStructuredSelection handleRemovalOfResources(AbstractTextSearchResult result,
+			IStructuredSelection selection) {
+		Set<IResource> elementsToRemove = new HashSet<>();
+		List<Object> others = new ArrayList<>();
+		Object[] resultElements = result.getElements();
+		for (Object selectedObj : selection) {
+			if (selectedObj instanceof IFile resource) {
+				elementsToRemove.add(resource);
+			} else if (selectedObj instanceof IContainer container) {
+				for (Object resElement : resultElements) {
+					if (resElement instanceof IResource res && container.getFullPath().isPrefixOf(res.getFullPath())) {
+						elementsToRemove.add(res);
+					}
+				}
+			} else {
+				others.add(selectedObj);
+			}
+		}
+		if (!elementsToRemove.isEmpty()) {
+			result.removeElements(elementsToRemove);
+			if (others.isEmpty()) {
+				navigateNext(true);
+				return StructuredSelection.EMPTY;
+			}
+			selection = new StructuredSelection(others);
+		}
+		return selection;
+	}
+
 	private void collectAllMatches(HashSet<Match> set, Object[] elements) {
 		for (Object element : elements) {
 			Match[] matches = getDisplayedMatches(element);

tests/org.eclipse.search.tests/META-INF/MANIFEST.MF

@@ -2,22 +2,22 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.search.tests;singleton:=true
-Bundle-Version: 3.12.0.qualifier
+Bundle-Version: 3.12.100.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Export-Package: org.eclipse.search.core.tests;x-internal:=true,
  org.eclipse.search.tests;x-internal:=true,
  org.eclipse.search.tests.filesearch;x-internal:=true
 Require-Bundle: 
  org.eclipse.ui;bundle-version="[3.208.0,4.0.0)",
- org.eclipse.ui.ide;bundle-version="[3.21.200,4.0.0)",
- org.eclipse.search;bundle-version="[3.16.0,4.0.0)",
- org.eclipse.core.runtime;bundle-version="[3.29.100,4.0.0)",
- org.eclipse.core.resources;bundle-version="[3.19.200,4.0.0)",
- org.eclipse.ui.workbench.texteditor;bundle-version="[3.17.200,4.0.0)",
- org.eclipse.jface.text;bundle-version="[3.24.200,4.0.0)",
- org.eclipse.ui.editors;bundle-version="[3.17.100,4.0.0)",
- org.eclipse.ltk.core.refactoring;bundle-version="[3.14.100,4.0.0)",
+ org.eclipse.ui.ide;bundle-version="[3.23.0,4.0.0)",
+ org.eclipse.search;bundle-version="[3.19.0,4.0.0)",
+ org.eclipse.core.runtime;bundle-version="[3.34.200,4.0.0)",
+ org.eclipse.core.resources;bundle-version="[3.23.200,4.0.0)",
+ org.eclipse.ui.workbench.texteditor;bundle-version="[3.20.0,4.0.0)",
+ org.eclipse.jface.text;bundle-version="[3.30.0,4.0.0)",
+ org.eclipse.ui.editors;bundle-version="[3.21.0,4.0.0)",
+ org.eclipse.ltk.core.refactoring;bundle-version="[3.15.100,4.0.0)",
  org.hamcrest;bundle-version="3.0.0"
 Import-Package: org.junit.jupiter.api;version="[5.14.0,6.0.0)",
  org.junit.jupiter.api.extension;version="[5.14.0,6.0.0)",