From 6ffeee45fd42daf66536cd3e1744cf3121412e2d Mon Sep 17 00:00:00 2001 From: Mehmet Karaman Date: Tue, 17 Feb 2026 14:51:30 +0100 Subject: [PATCH] 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 https://github.com/eclipse-platform/eclipse.platform.ui/issues/3735 Co-authored-by: Andrey Loskutov --- .../ui/text/AbstractTextSearchResult.java | 87 ++++++++- .../ui/text/AbstractTextSearchViewPage.java | 39 +++- .../search/tests/TextSearchResultTest.java | 171 ++++++++++++++++++ .../tests/filesearch/AllFileSearchTests.java | 5 +- 4 files changed, 292 insertions(+), 10 deletions(-) create mode 100644 tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java diff --git a/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java b/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java index 88b5a992d2e8..f934e870e225 100644 --- a/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java +++ b/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> fElementsToMatches; private final List fListeners; private final AtomicInteger matchCount; + private final ConcurrentHashMap 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 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 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 = match2.getOffset() - match1.getOffset(); + if (diff != 0) { + return diff; + } + diff = 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.18 + */ + public boolean hasMatches(Object element) { + if (element == null) { + return false; + } + Set 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 getMatches(element).length @@ -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.18 + */ + public void removeElements(Collection elements) { + matchCount.set(0); + List removedMatches = new ArrayList<>(); + for (Object object : elements) { + Set 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)); + } + } } diff --git a/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java b/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java index a7bc5c94932b..3d0a2bb26464 100644 --- a/bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java +++ b/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 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 elementsToRemove = new HashSet<>(); + List 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 set, Object[] elements) { for (Object element : elements) { Match[] matches = getDisplayedMatches(element); diff --git a/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java b/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java new file mode 100644 index 000000000000..eb59f8c05d33 --- /dev/null +++ b/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java @@ -0,0 +1,171 @@ +/******************************************************************************* + * Copyright (c) 2026 Advantest Europe GmbH and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Mehmet Karaman (mehmet.karaman@advantest.com) - initial API and implementation + *******************************************************************************/ +package org.eclipse.search.tests; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Arrays; +import java.util.List; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import org.eclipse.swt.widgets.Display; + +import org.eclipse.core.resources.IFolder; +import org.eclipse.core.resources.IProject; + +import org.eclipse.jface.viewers.ITreeContentProvider; +import org.eclipse.jface.viewers.StructuredSelection; +import org.eclipse.jface.viewers.StructuredViewer; + +import org.eclipse.search.internal.ui.text.FileSearchPage; +import org.eclipse.search.internal.ui.text.FileSearchQuery; +import org.eclipse.search.tests.filesearch.JUnitSourceSetup; +import org.eclipse.search.ui.NewSearchUI; +import org.eclipse.search.ui.text.AbstractTextSearchResult; +import org.eclipse.search.ui.text.FileTextSearchScope; + +import org.eclipse.search2.internal.ui.SearchView; + +public class TextSearchResultTest { + + FileSearchQuery fQuery1; + + @RegisterExtension + static JUnitSourceSetup fgJUnitSource= new JUnitSourceSetup(); + + @BeforeEach + public void setUp() throws Exception { + String[] fileNamePatterns= { "*.java" }; + FileTextSearchScope scope= FileTextSearchScope.newWorkspaceScope(fileNamePatterns, false); + + fQuery1= new FileSearchQuery("Test", false, true, scope); + NewSearchUI.runQueryInForeground(null, fQuery1); + + SearchView sV= (SearchView) NewSearchUI.getSearchResultView(); + FileSearchPage currentPage= (FileSearchPage) sV.getCurrentPage(); + currentPage.setLayout(org.eclipse.search.ui.text.AbstractTextSearchViewPage.FLAG_LAYOUT_TREE); + runEventLoopUntilEmpty(); + } + + @AfterAll + public static void tearDown() throws Exception { + SearchView sV= (SearchView) NewSearchUI.getSearchResultView(); + FileSearchPage currentPage= (FileSearchPage) sV.getCurrentPage(); + currentPage.setLayout(org.eclipse.search.ui.text.AbstractTextSearchViewPage.FLAG_LAYOUT_FLAT); + runEventLoopUntilEmpty(); + } + + @Test + public void testBatchRemoveElements() throws Exception { + AbstractTextSearchResult result= (AbstractTextSearchResult) fQuery1.getSearchResult(); + Object[] elements= result.getElements(); + + assertTrue(elements.length >= 2, "Should have at least 2 elements"); + int originalCount= result.getMatchCount(); + assertTrue(originalCount > 0, "Should have matches"); + + int matchCountElement0= result.getMatchCount(elements[0]); + int matchCountElement1= result.getMatchCount(elements[1]); + + List toRemove= Arrays.asList(elements[0], elements[1]); + result.removeElements(toRemove); + + int expectedCount= originalCount - matchCountElement0 - matchCountElement1; + assertEquals(expectedCount, result.getMatchCount(), "Correct number of matches removed"); + + assertFalse(result.hasMatches(elements[0]), "First element should have no matches"); + assertFalse(result.hasMatches(elements[1]), "Second element should have no matches"); + } + + + @Test + public void testBatchRemoveElementsByProject() throws Exception { + AbstractTextSearchResult result= (AbstractTextSearchResult) fQuery1.getSearchResult(); + Object[] elements= result.getElements(); + + assertTrue(elements.length >= 2, "Should have at least 2 elements"); + int originalCount= result.getMatchCount(); + assertTrue(originalCount > 0, "Should have matches"); + + SearchView sV= (SearchView) NewSearchUI.getSearchResultView(); + FileSearchPage currentPage= (FileSearchPage) sV.getCurrentPage(); + + StructuredViewer resViewer= currentPage.getViewer(); + ITreeContentProvider contentProvider= (ITreeContentProvider) resViewer.getContentProvider(); + Object[] directElements= contentProvider.getElements(resViewer.getInput()); + + assertTrue(directElements.length == 1, "Should have only one direct element"); + assertTrue(directElements[0] instanceof IProject, "Should be a project"); + + resViewer.setSelection(new StructuredSelection(directElements[0])); + + currentPage.internalRemoveSelected(); + + assertEquals(0, result.getMatchCount(), "Correct number of matches removed"); + } + + @Test + public void testBatchRemoveElementsByFolder() throws Exception { + AbstractTextSearchResult result= (AbstractTextSearchResult) fQuery1.getSearchResult(); + Object[] elements= result.getElements(); + + assertTrue(elements.length >= 2, "Should have at least 2 elements"); + int originalCount= result.getMatchCount(); + assertTrue(originalCount > 0, "Should have matches"); + + SearchView sV= (SearchView) NewSearchUI.getSearchResultView(); + FileSearchPage currentPage= (FileSearchPage) sV.getCurrentPage(); + + StructuredViewer resViewer= currentPage.getViewer(); + ITreeContentProvider treeContentProvider= (ITreeContentProvider) resViewer.getContentProvider(); + Object[] directElements= treeContentProvider.getElements(resViewer.getInput()); + Object[] children= treeContentProvider.getChildren(directElements[0]); + + assertTrue(directElements.length == 1, "Should have one direct element"); + assertTrue(children.length == 1, "Should have one child element"); + assertTrue(children[0] instanceof IFolder, "Should be a folder"); + + resViewer.setSelection(new StructuredSelection(children[0])); + + currentPage.internalRemoveSelected(); + + assertEquals(0, result.getMatchCount(), "Correct number of matches removed"); + } + + + /* + * Process all pending UI events to ensure that the UI is updated before assertions are made. + */ + private static void runEventLoopUntilEmpty() { + final Display display= Display.getDefault(); + if (display == null) { + return; + } + if (Display.getCurrent() == display) { + while (display.readAndDispatch()) { + } + } else { + display.syncExec(() -> { + while (display.readAndDispatch()) { + } + }); + } + } +} diff --git a/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/AllFileSearchTests.java b/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/AllFileSearchTests.java index 8dd38dd2ac9c..969d6c976f87 100644 --- a/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/AllFileSearchTests.java +++ b/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/AllFileSearchTests.java @@ -16,6 +16,8 @@ import org.junit.platform.suite.api.SelectClasses; import org.junit.platform.suite.api.Suite; +import org.eclipse.search.tests.TextSearchResultTest; + @Suite @SelectClasses({ AnnotationManagerTest.class, @@ -24,7 +26,8 @@ PositionTrackerTest.class, ResultUpdaterTest.class, SearchResultPageTest.class, - SortingTest.class + SortingTest.class, + TextSearchResultTest.class, }) public class AllFileSearchTests { }