From 4e8f78b29fe25ed9e31a957c62c17576f7d14721 Mon Sep 17 00:00:00 2001 From: Andrey Loskutov Date: Wed, 2 Sep 2026 15:57:52 +0200 Subject: [PATCH] File search: don't re-evaluate match filters in the UI thread FileTreeContentProvider#elementsChanged(..) evaluated the active match filters again for every match of every updated file. The filter state of a match is however already computed once per match by AbstractTextSearchResult#didAddMatch(..) (and updated when the filters change) and is cached in Match#isFiltered(), which is also what initialize(..) and AbstractTextSearchViewPage#getDisplayedMatchCount(..) use. Re-evaluating the filters is not only redundant, it is expensive: OuterProjectFileFilter calls IWorkspaceRoot#findFilesForLocationURI(..), which iterates over all projects of the workspace. With a search producing thousands of matches this ran for every match in the UI thread on every batched update and froze the UI. The provider now reads the already computed filter state instead. The collection of the updated line elements was reworked as well: the matches of a file are enumerated only once, no matter how many lines of that file were updated, only the updated lines are remembered instead of all lines of the touched files, and the enumeration stops as soon as all updated lines are known to have matches. Since line elements have identity semantics (LineElement doesn't implement equals(..)/hashCode()), identity based sets are used. OuterProjectFileFilter is still evaluated once per match by the search result, so it now remembers the filter state per file instead of repeating the workspace lookup for every match of that file. The states are kept in a weak map keyed by the file handles the matches hold, so they are collected together with the search result they were computed for and the filter, which is shared by all file search results, doesn't keep anything alive. A resource change listener invalidates the states if projects are added, removed, opened, closed or moved - the only changes that can modify which files represent a location - and deregisters itself again as soon as no states are left. The listener is (un)registered without holding a lock of the filter to avoid a lock order inversion with the workspace notification. Added test for OuterProjectFileFilter (missed in the original https://github.com/eclipse-platform/eclipse.platform.text/pull/144). Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/4337 Assisted-by: Github Copilot (Claude Opus 5) --- .../ui/text/FileTreeContentProvider.java | 92 +++--- .../ui/text/OuterProjectFileFilter.java | 120 ++++++-- .../tests/filesearch/AllFileSearchTests.java | 1 + .../filesearch/NestedProjectFilterTest.java | 276 ++++++++++++++++++ 4 files changed, 431 insertions(+), 58 deletions(-) create mode 100644 tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/NestedProjectFilterTest.java diff --git a/bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/FileTreeContentProvider.java b/bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/FileTreeContentProvider.java index dc9e000cd91..a66a8199166 100644 --- a/bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/FileTreeContentProvider.java +++ b/bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/FileTreeContentProvider.java @@ -17,18 +17,13 @@ *******************************************************************************/ package org.eclipse.search.internal.ui.text; -import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; -import java.util.Spliterator; -import java.util.Spliterators; -import java.util.stream.Collectors; -import java.util.stream.Stream; -import java.util.stream.StreamSupport; import org.eclipse.core.resources.IProject; import org.eclipse.core.resources.IResource; @@ -251,22 +246,6 @@ public boolean hasChildren(Object element) { return !children.isEmpty(); } - static Stream toStream(Enumeration e) { - return StreamSupport.stream(Spliterators.spliteratorUnknownSize(e.asIterator(), Spliterator.ORDERED), false); - } - - private boolean isUnfiltered(FileMatch m) { - MatchFilter[] filters = fResult.getActiveMatchFilters(); - if (filters != null) { - for (MatchFilter filter : filters) { - if (filter.filters(m)) { - return false; - } - } - } - return true; - } - /** * * Update the search contents. Screen out any results that are filtered via @@ -281,25 +260,7 @@ private boolean isUnfiltered(FileMatch m) { @Override public synchronized void elementsChanged(Object[] updatedElements) { boolean singleElement = updatedElements.length == 1; - Set lineMatches = Collections.emptySet(); - // if we have active match filters, we should only use non-filtered FileMatch - // objects to collect LineElements to update - if (hasActiveMatchFilters()) { - lineMatches = Arrays.stream(updatedElements).filter(LineElement.class::isInstance) - // only for distinct files: - .map(u -> ((LineElement) u).getParent()).distinct() - // query matches: - .map(fResult::getMatchSet).flatMap(FileTreeContentProvider::toStream) - .map(m -> ((FileMatch) m)).filter(this::isUnfiltered).map(m -> m.getLineElement()) - .collect(Collectors.toSet()); - } else { - lineMatches = Arrays.stream(updatedElements).filter(LineElement.class::isInstance) - // only for distinct files: - .map(u -> ((LineElement) u).getParent()).distinct() - // query matches: - .map(fResult::getMatchSet).flatMap(FileTreeContentProvider::toStream) - .map(m -> ((FileMatch) m).getLineElement()).collect(Collectors.toSet()); - } + Set lineMatches = getUpdatedLinesWithMatches(updatedElements); try { for (Object updatedElement : updatedElements) { if (!(updatedElement instanceof LineElement lineElement)) { @@ -337,6 +298,55 @@ private boolean hasActiveMatchFilters() { return activeMatchFilters != null && activeMatchFilters.length > 0; } + /** + * Collects the updated line elements that still have matches. The matches of a + * file are enumerated only once, no matter how many lines of that file have been + * updated, and only the given lines are remembered instead of all lines of the + * touched files. + * + * @param updatedElements the updated elements, may contain elements that are no + * line elements + * @return the line elements of updatedElements that have at least + * one match that is not hidden by an active match filter + */ + private Set getUpdatedLinesWithMatches(Object[] updatedElements) { + // LineElement doesn't implement equals(..)/hashCode(), matches refer to the + // very same instance the update is reported for + Set updatedLines = Collections.newSetFromMap(new IdentityHashMap<>()); + Set files = new HashSet<>(); + for (Object updatedElement : updatedElements) { + if (updatedElement instanceof LineElement lineElement) { + updatedLines.add(lineElement); + files.add(lineElement.getParent()); + } + } + if (updatedLines.isEmpty()) { + return Collections.emptySet(); + } + // if we have active match filters, we should only use non-filtered FileMatch + // objects to collect LineElements to update. The filter state is evaluated + // once per match by the search result (see AbstractTextSearchResult), it must + // not be computed again here: match filters can be expensive and this code + // runs in the UI thread for every batch of search results. + boolean useFilterState = hasActiveMatchFilters(); + Set linesWithMatches = Collections.newSetFromMap(new IdentityHashMap<>()); + for (IResource file : files) { + Enumeration matches = fResult.getMatchSet(file); + while (matches.hasMoreElements()) { + Match match = matches.nextElement(); + if (useFilterState && match.isFiltered()) { + continue; + } + LineElement lineElement = ((FileMatch) match).getLineElement(); + if (updatedLines.contains(lineElement) && linesWithMatches.add(lineElement) + && linesWithMatches.size() == updatedLines.size()) { + return linesWithMatches; // all updated lines have matches + } + } + } + return linesWithMatches; + } + @Override public void clear() { initialize(fResult); diff --git a/bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/OuterProjectFileFilter.java b/bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/OuterProjectFileFilter.java index 021c1cd8df2..3123330a944 100644 --- a/bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/OuterProjectFileFilter.java +++ b/bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/OuterProjectFileFilter.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2023 Red Hat Inc. and others. + * Copyright (c) 2023, 2026 Red Hat Inc. and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -15,9 +15,18 @@ import java.net.URI; import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; +import java.util.Map; +import java.util.Optional; +import java.util.WeakHashMap; +import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.core.resources.IFile; +import org.eclipse.core.resources.IResourceChangeEvent; +import org.eclipse.core.resources.IResourceChangeListener; +import org.eclipse.core.resources.IResourceDelta; +import org.eclipse.core.resources.ResourcesPlugin; import org.eclipse.search.internal.ui.SearchMessages; import org.eclipse.search.ui.text.Match; @@ -25,25 +34,102 @@ public class OuterProjectFileFilter extends MatchFilter { + /** + * Remembers for the files of the reported matches whether they are filtered. + *

+ * The filter state is evaluated for every single match, but + * {@link org.eclipse.core.resources.IWorkspaceRoot#findFilesForLocationURI(URI)} + * iterates over all projects of the workspace and is therefore much too + * expensive to be called once per match: a file usually has many matches. + *

+ *

+ * The keys are the file handles held by the matches ({@link Match#getElement()} + * ), and the values don't reference them, so the remembered states are garbage + * collected together with the search result they were computed for: the filter, + * which is shared by all file search results, doesn't keep them alive. + *

+ *

+ * Outdated states are discarded by replacing the whole map. A state that is + * computed while the map is replaced is put into the replaced map and is + * therefore never seen again, so an invalidation cannot be lost. + *

+ */ + private volatile Map filterStates = newFilterStates(); + + private final AtomicBoolean isListening = new AtomicBoolean(); + + /** + * The files representing a location only change if projects are added, removed, + * opened, closed or moved. + */ + private final IResourceChangeListener projectChangeListener = event -> { + if (affectsProjects(event.getDelta())) { + filterStates = newFilterStates(); + } + }; + + private static Map newFilterStates() { + return Collections.synchronizedMap(new WeakHashMap<>()); + } + + private void ensureListeningToProjectChanges() { + if (!isListening.get() && isListening.compareAndSet(false, true)) { + ResourcesPlugin.getWorkspace().addResourceChangeListener(projectChangeListener, + IResourceChangeEvent.POST_CHANGE); + } + } + + private static boolean affectsProjects(IResourceDelta delta) { + if (delta == null) { + return false; + } + for (IResourceDelta projectDelta : delta.getAffectedChildren()) { + if (projectDelta.getKind() != IResourceDelta.CHANGED) { + return true; // project added or removed + } + int flags= projectDelta.getFlags(); + if ((flags & (IResourceDelta.OPEN | IResourceDelta.DESCRIPTION | IResourceDelta.MOVED_FROM + | IResourceDelta.MOVED_TO | IResourceDelta.LOCAL_CHANGED | IResourceDelta.REPLACED)) != 0) { + return true; + } + } + return false; + } + @Override public boolean filters(Match match) { - if (match instanceof FileMatch) { - IFile file = ((FileMatch) match).getFile(); - URI locationUri = file.getLocationURI(); - - IFile innermostFile = locationUri == null ? file : // - Arrays.stream(file.getWorkspace().getRoot().findFilesForLocationURI(locationUri)) // - // Don't consider the content of a closed project - // for filtering because the matches there cannot be - // shown - .filter(aFile -> aFile.getProject().isAccessible()) - .min(Comparator.comparingInt(aFile -> aFile.getFullPath().segments().length)) - // shortest workspace (project relative) full path - // means most nested project - .orElse(file); - return !file.equals(innermostFile); + if (!(match instanceof FileMatch fileMatch)) { + return false; } - return false; + IFile file= fileMatch.getFile(); + // the listener is registered before the state of the workspace is read, so + // that every change that invalidates the computed state is reported + ensureListeningToProjectChanges(); + Map states= filterStates; + Boolean isFiltered= states.get(file); + if (isFiltered == null) { + // computed without holding a lock: it may be computed twice for a file, + // but it must not block the other search threads + isFiltered= Boolean.valueOf(computeFilters(file)); + states.put(file, isFiltered); + } + return isFiltered.booleanValue(); + } + + private static boolean computeFilters(IFile file) { + URI locationUri= file.getLocationURI(); + if (locationUri == null) { + return false; + } + Optional innermostFile= Arrays + .stream(file.getWorkspace().getRoot().findFilesForLocationURI(locationUri)) // + // Don't consider the content of a closed project for filtering + // because the matches there cannot be shown + .filter(aFile -> aFile.getProject().isAccessible()) + // shortest workspace (project relative) full path means most + // nested project + .min(Comparator.comparingInt(aFile -> aFile.getFullPath().segments().length)); + return innermostFile.isPresent() && !file.equals(innermostFile.get()); } @Override 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 5647b6ac2e9..36946714cf9 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 @@ -23,6 +23,7 @@ AnnotationManagerTest.class, FileSearchTests.class, LineAnnotationManagerTest.class, + NestedProjectFilterTest.class, PositionTrackerTest.class, ResultUpdaterTest.class, SearchResultPageTest.class, diff --git a/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/NestedProjectFilterTest.java b/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/NestedProjectFilterTest.java new file mode 100644 index 00000000000..cf3bb1a4282 --- /dev/null +++ b/tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/NestedProjectFilterTest.java @@ -0,0 +1,276 @@ +/******************************************************************************* + * Copyright (c) 2026 Andrey Loskutov 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: + * Andrey Loskutov - initial API and implementation + *******************************************************************************/ +package org.eclipse.search.tests.filesearch; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.eclipse.swt.widgets.Display; + +import org.eclipse.core.runtime.jobs.IJobManager; +import org.eclipse.core.runtime.jobs.Job; + +import org.eclipse.core.resources.IFile; +import org.eclipse.core.resources.IFolder; +import org.eclipse.core.resources.IProject; +import org.eclipse.core.resources.IProjectDescription; +import org.eclipse.core.resources.IResource; +import org.eclipse.core.resources.IWorkspace; +import org.eclipse.core.resources.ResourcesPlugin; + +import org.eclipse.jface.viewers.AbstractTreeViewer; + +import org.eclipse.search.internal.ui.text.FileMatch; +import org.eclipse.search.internal.ui.text.FileSearchPage; +import org.eclipse.search.internal.ui.text.FileSearchQuery; +import org.eclipse.search.internal.ui.text.FileSearchResult; +import org.eclipse.search.internal.ui.text.OuterProjectFileFilter; +import org.eclipse.search.tests.ResourceHelper; +import org.eclipse.search.tests.SearchTestUtil; +import org.eclipse.search.ui.ISearchResultViewPart; +import org.eclipse.search.ui.NewSearchUI; +import org.eclipse.search.ui.text.AbstractTextSearchViewPage; +import org.eclipse.search.ui.text.FileTextSearchScope; +import org.eclipse.search.ui.text.Match; +import org.eclipse.search.ui.text.MatchFilter; + +/** + * Tests the match filter that hides the matches of files which are reported for + * an outer project although they belong to a nested project, see + * https://github.com/eclipse-platform/eclipse.platform.text/issues/143 + *

+ * The tests use two projects that share the same file on disk: the location of + * the inner project is a folder of the outer project, so the very same file is + * represented by two resources and is reported twice by a search. + *

+ */ +public class NestedProjectFilterTest { + + private static final String OUTER_PROJECT_NAME= "nested-project-filter-outer"; + + private static final String INNER_PROJECT_NAME= "nested-project-filter-inner"; + + private static final String FILE_NAME= "test.txt"; + + private static final String SEARCH_STRING= "nestedProjectFilterNeedle"; + + private IProject outerProject; + + private IProject innerProject; + + /** The file as seen by the inner (innermost) project. */ + private IFile innerFile; + + /** The very same file on disk, as seen by the enclosing outer project. */ + private IFile outerFile; + + private FileSearchPage page; + + private int previousLayout; + + private MatchFilter[] lastUsedFilters; + + @BeforeEach + public void setUp() throws Exception { + SearchTestUtil.ensureWelcomePageClosed(); + // new search results pick up the last used filters, start without any + lastUsedFilters= FileSearchResult.getLastUsedFilters(); + FileSearchResult.setLastUsedFilters(new MatchFilter[0]); + + outerProject= ResourceHelper.createProject(OUTER_PROJECT_NAME); + IFolder nestedFolder= ResourceHelper.createFolder(outerProject.getFolder(INNER_PROJECT_NAME)); + outerFile= ResourceHelper.createFile(nestedFolder, FILE_NAME, SEARCH_STRING); + innerProject= createProjectAt(INNER_PROJECT_NAME, nestedFolder); + innerFile= innerProject.getFile(FILE_NAME); + + assertTrue(innerFile.exists(), "the nested project must see the file of the outer project"); + assertEquals(outerFile.getLocationURI(), innerFile.getLocationURI(), + "both resources must represent the same file on disk"); + assertTrue(innerFile.getFullPath().segmentCount() < outerFile.getFullPath().segmentCount(), + "the file of the innermost project must have the shortest path"); + } + + @AfterEach + public void tearDown() throws Exception { + if (page != null) { + // the layout is shared by all file search pages + page.setLayout(previousLayout); + page= null; + } + // setActiveMatchFilters(..) persists the filters in the dialog settings + FileSearchResult.setLastUsedFilters(lastUsedFilters); + // the inner project is located inside the outer one, delete it first + ResourceHelper.deleteProject(INNER_PROJECT_NAME); + ResourceHelper.deleteProject(OUTER_PROJECT_NAME); + } + + /** + * Only the file of the outer project is a duplicate, the file of the innermost + * project is the one to show. + */ + @Test + public void testDuplicateOfOuterProjectIsFiltered() { + OuterProjectFileFilter filter= new OuterProjectFileFilter(); + + assertTrue(filter.filters(new FileMatch(outerFile)), + "the file reported for the outer project must be filtered"); + assertFalse(filter.filters(new FileMatch(innerFile)), + "the file of the innermost project must not be filtered"); + } + + /** + * The filter remembers its answer per file, repeated evaluations must not + * change the result. + */ + @Test + public void testRepeatedEvaluationIsStable() { + OuterProjectFileFilter filter= new OuterProjectFileFilter(); + + for (int i= 0; i < 3; i++) { + assertTrue(filter.filters(new FileMatch(outerFile)), "evaluation " + i); + assertFalse(filter.filters(new FileMatch(innerFile)), "evaluation " + i); + } + } + + /** + * The matches of a closed project cannot be shown, so the file of the outer + * project is not a duplicate anymore once the inner project is closed. The + * filter must not answer with an outdated (remembered) state. + */ + @Test + public void testFilterIsUpdatedWhenInnerProjectIsClosed() throws Exception { + OuterProjectFileFilter filter= new OuterProjectFileFilter(); + assertTrue(filter.filters(new FileMatch(outerFile)), "precondition: the file is a duplicate"); + + innerProject.close(null); + + assertFalse(filter.filters(new FileMatch(outerFile)), + "the file of the outer project is the only one that can be shown now"); + } + + /** + * A file that exists only once must never be filtered. + */ + @Test + public void testUniqueFileIsNotFiltered() throws Exception { + IFolder folder= ResourceHelper.createFolder(outerProject.getFolder("unique")); + IFile uniqueFile= ResourceHelper.createFile(folder, FILE_NAME, SEARCH_STRING); + OuterProjectFileFilter filter= new OuterProjectFileFilter(); + + assertFalse(filter.filters(new FileMatch(uniqueFile))); + } + + /** + * Without the filter the same file is reported twice, with the filter only the + * matches of the innermost project are shown. + */ + @Test + public void testFilterStateOfSearchResult() throws Exception { + FileSearchQuery query= createQuery(); + NewSearchUI.runQueryInForeground(null, query); + FileSearchResult result= (FileSearchResult) query.getSearchResult(); + + assertEquals(2, result.getMatchCount(), "the same file must be found in both projects"); + assertEquals(1, result.getMatchCount(innerFile)); + assertEquals(1, result.getMatchCount(outerFile)); + + result.setActiveMatchFilters(new MatchFilter[] { getInnermostProjectFilter(result) }); + + assertTrue(isFiltered(result, outerFile), "the duplicate of the outer project must be filtered"); + assertFalse(isFiltered(result, innerFile), "the file of the innermost project must be shown"); + assertEquals(2, result.getMatchCount(), "filtered matches are still part of the result"); + } + + /** + * The filtered matches must not be shown in the tree of the search view. + */ + @Test + public void testFilteredFileIsNotShownInTree() throws Exception { + FileSearchQuery query= createQuery(); + NewSearchUI.runQueryInForeground(null, query); + FileSearchResult result= (FileSearchResult) query.getSearchResult(); + + ISearchResultViewPart view= NewSearchUI.getSearchResultView(); + page= (FileSearchPage) view.getActivePage(); + previousLayout= page.getLayout(); + page.setLayout(AbstractTextSearchViewPage.FLAG_LAYOUT_TREE); + result.setActiveMatchFilters(new MatchFilter[] { getInnermostProjectFilter(result) }); + consumeEvents(); + + AbstractTreeViewer viewer= (AbstractTreeViewer) page.getViewer(); + viewer.expandAll(); + + assertNotNull(viewer.testFindItem(innerFile), "the file of the innermost project must be shown"); + assertNull(viewer.testFindItem(outerFile), "the duplicate of the outer project must not be shown"); + } + + private static boolean isFiltered(FileSearchResult result, IFile file) { + Match[] matches= result.getMatches(file); + assertEquals(1, matches.length, "unexpected number of matches for " + file.getFullPath()); + return matches[0].isFiltered(); + } + + private static MatchFilter getInnermostProjectFilter(FileSearchResult result) { + for (MatchFilter filter : result.getAllMatchFilters()) { + if (filter instanceof OuterProjectFileFilter) { + return filter; + } + } + throw new AssertionError("the file search result must provide the innermost project filter"); + } + + private FileSearchQuery createQuery() { + FileTextSearchScope scope= FileTextSearchScope.newSearchScope( + new IResource[] { innerProject, outerProject }, new String[] { "*.txt" }, false); + return new FileSearchQuery(SEARCH_STRING, false, true, scope); + } + + /** + * Creates a project at the location of the given folder, so that the content of + * the folder belongs to two projects. + */ + private static IProject createProjectAt(String projectName, IFolder folder) throws Exception { + IWorkspace workspace= ResourcesPlugin.getWorkspace(); + IProject project= workspace.getRoot().getProject(projectName); + IProjectDescription description= workspace.newProjectDescription(projectName); + description.setLocation(folder.getLocation()); + project.create(description, null); + project.open(null); + project.refreshLocal(IResource.DEPTH_INFINITE, null); + folder.getProject().refreshLocal(IResource.DEPTH_INFINITE, null); + return project; + } + + private void consumeEvents() { + IJobManager manager= Job.getJobManager(); + while (manager.find(page).length > 0) { + runEventLoop(); + } + runEventLoop(); + } + + private static void runEventLoop() { + Display display= Display.getCurrent(); + while (display != null && display.readAndDispatch()) { + // process all pending events + } + } +}