Search View: Performance issues on remove and sort by mehmet-karaman · Pull Request #3733 · eclipse-platform/eclipse.platform.ui

mehmet-karaman · 2026-02-17T14:17:58Z

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.

iloveeclipse · 2026-02-17T14:21:47Z

@mehmet-karaman : please first create a ticket and explain the problem.
Note: you seem to use wrong git author mail, please use same which you've used in ECA process.

github-actions · 2026-02-17T15:34:19Z

Test Results

3 027 files +3 3 027 suites +3 2h 21m 23s ⏱️ + 4m 39s
8 236 tests +2 7 987 ✅ +1 248 💤 ±0 0 ❌ ±0 1 🔥 +1
23 532 runs +6 22 738 ✅ +3 791 💤 ±0 0 ❌ ±0 3 🔥 +3

For more details on these errors, see this check.

Results for commit 7b15304. ± Comparison against base commit 89a19bf.

♻️ This comment has been updated with latest results.

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

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

iloveeclipse · 2026-02-18T15:38:22Z

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

+		if (!parents.isEmpty()) {
+			result.removeParents(parents);
+			if (others.isEmpty()) {
+				return;


This doesn't update UI, at least not for me.
I see the tree in the search view, select subtree, click on delete ... nothing happens at all now.
What is missing is the code that updates UI after this operation.

I'm using exact this steps: #3735 (comment), should be reproducible for everyone.

I hope i understood it correct, I have tried to record what is currently on my branch:

Videoprojekt.4.mp4

As explained in the reproduction scenario.. Could it be that you didn't checked out that commit?

Please just follow steps I've provided.

Now I got it.. It seems that the method AbstractTextSearchResult.removeElements(List<?>) gets called with wrong 'enclosing' elements.. My solution needs to take care on deeper folder structures and gather all IFiles..

I've added a resource visitor which collects all IFiles inside of the selected 'enclosing' element.. Could be a better way to achieve this.. Maybe I should use the ContentProvider and navigate through the search results instead?

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

Copilot

Pull request overview

This PR addresses Search View performance and correctness when removing matches, by enabling batch removal at the “enclosing element” (e.g., file/resource) level and ensuring removals aren’t limited to only currently visible (element-limited) results.

Changes:

Updated AbstractTextSearchViewPage.internalRemoveSelected() to batch-remove matches by selected IFile / IContainer resources before falling back to per-match removal for non-resource selections.
Added AbstractTextSearchResult.removeElements(...) to remove all matches for selected elements via a single map-entry removal per element.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java`	Adds resource-aware pre-processing in “Remove Selected Matches” to remove matches by file/container in a batch.
`bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java`	Introduces an element-based batch removal method to efficiently drop all matches for selected elements.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

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

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

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

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-19T14:18:02Z

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

 	}

+	/**
+	 * @return {@code true} if the result is not empty


The Javadoc description is incorrect or incomplete. It says "if the result is not empty" but should describe that this method checks if the specified element has any matches, not whether the entire result is not empty. Consider updating the documentation to something like "Returns true if the given element has matches" to clarify the method's purpose.

Suggested change

* @return {@code true} if the result is not empty

* Returns {@code true} if the given element has matches.

Copilot · 2026-02-19T14:18:03Z

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

+	 *
+	 * @param elements
+	 *            the elements of which matches should be removed.
+	 */
+	void removeElements(List<?> elements) {


The removeElements method is package-private but appears to be a new API method that follows the pattern of other removal methods in this class (removeMatch, removeMatches, removeAll). All other removal methods in AbstractTextSearchResult are public. Consider making this method public for API consistency, and adding proper Javadoc documentation with @SInCE 3.18 tag and "Subclasses may extend this method" as seen in other removal methods. Alternatively, if this is intentionally internal-only, consider adding a comment explaining why it deviates from the pattern.

Suggested change

*

* @param elements

* the elements of which matches should be removed.

*/

void removeElements(List<?> elements) {

* <p>

* Subclasses may extend this method.

* </p>

*

* @param elements

* the elements of which matches should be removed.

*

* @since 3.18

*/

public void removeElements(List<?> elements) {

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

Copilot · 2026-02-19T14:18:04Z

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

+		List<IResource> elementsToRemove = new ArrayList<>();
+		List<Object> others = new ArrayList<>();
+		Object[] resultElements = result.getElements();
+		for (Object selectedObj : selection) {
+			if (selectedObj instanceof IContainer container) {
+				for (Object resElement : resultElements) {
+					if (resElement instanceof IResource res && container.getFullPath().isPrefixOf(res.getFullPath())
+							&& res.getType() == IResource.FILE) {
+						elementsToRemove.add(res);
+					}
+				}
+			} else if (selectedObj instanceof IFile file) {
+				elementsToRemove.add(file);


The elementsToRemove list can contain duplicate IResource entries if a user selects both a container and a file within that container. When processing the container at line 1398-1404, all files under it are added; then if one of those files is also explicitly selected, it gets added again at line 1406. While removeElements handles this gracefully (the second removal is a no-op), this creates unnecessary work. Consider using a Set instead of a List for elementsToRemove, or checking if the file is already in the list before adding it at line 1406.

This can happen. fElementsToMatches is removing the key and if its called second time it returns null which is also catched. So this is also obsolete.

Can be checked here: org.eclipse.search.ui.text.AbstractTextSearchResult.removeElements(List<?>)

- 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 eclipse-platform#3735 Co-authored-by: Andrey Loskutov <loskutov@gmx.de>

mehmet-karaman · 2026-02-20T11:31:57Z

Added new tests:

checks that duplicate matches don't get lost
check that the batch removal works.

iloveeclipse · 2026-02-20T13:13:17Z

tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java

+
+
+	@Test
+	public void testBatchRemoveElements() throws Exception {


This is a good basic case, but not necessarily testing the problem reported.

It deletes files, not folders with children as reported.

Let keep this test, but please add one more that would delete a folder, and check that all matches & elements with matches in that folder are removed but nothing else.

Here the folder I'm thinking about, which should be in the result set already:

iloveeclipse · 2026-02-20T13:19:42Z

@mehmet-karaman : please could you mark all comments on PR as resolved if they are already addressed by the latest change, and those that are unresolved give a comment what is missing?

iloveeclipse · 2026-02-20T13:27:56Z

tests/org.eclipse.search.tests/src/org/eclipse/search/tests/filesearch/SortingTest.java

 	}

+	@Test
+	public void testDuplicateOffsetsAndLengths() throws Exception {


Is this not a redundant test, see TestSearchResult tests which were failing before?

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

iloveeclipse

I've played a bit with the code and will address few comments in an extra commit on top, so you can see what I did. I will push in a moment.

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

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

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-20T15:14:30Z

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

+		for (Object selectedObj : selection) {
+			if (selectedObj instanceof IResource resource) {
+				elementsToRemove.add(resource);
+				if(resource instanceof IContainer container) {


Missing space after 'if' keyword. The codebase consistently uses a space after 'if', e.g., 'if (condition)' not 'if(condition)'.

Suggested change

if(resource instanceof IContainer container) {

if (resource instanceof IContainer container) {

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

Copilot · 2026-02-20T15:14:30Z

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

+				elementsToRemove.add(resource);
+				if(resource instanceof IContainer container) {
+					for (Object resElement : resultElements) {
+						if (resElement instanceof IResource res && container.getFullPath().isPrefixOf(res.getFullPath())) {


The condition container.getFullPath().isPrefixOf(res.getFullPath()) will match the container itself since a path is a prefix of itself. This means the container will be added to elementsToRemove twice (once at line 1419, and again at line 1423 when it matches itself). While using a Set prevents duplicates, this is still inefficient. Consider adding a check to exclude the container itself: && !container.equals(res) or using a more specific condition like checking if the resource is strictly a child.

Suggested change

if (resElement instanceof IResource res && container.getFullPath().isPrefixOf(res.getFullPath())) {

if (resElement instanceof IResource res

&& container.getFullPath().isPrefixOf(res.getFullPath())

&& !container.equals(res)) {

Copilot · 2026-02-20T15:14:31Z

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

+	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()) {
+			fireChange(getSearchResultEvent(removedMatches, MatchEvent.REMOVED));
+		}
+	}


The removeElements method has package-private visibility while all other removal methods (removeAll, removeMatch, removeMatches) are public. If this method is intended for internal use only (e.g., by AbstractTextSearchViewPage), consider adding a JavaDoc comment with @noreference or @nooverride annotations to explicitly document that this is internal API. Alternatively, if this is a generally useful method for batch element removal, consider making it public to maintain consistency with the existing API design.

Copilot · 2026-02-20T15:14:31Z

tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java

+		int matchCountElement1= result.getMatchCount(elements[1]);
+
+		List<Object> toRemove= Arrays.asList(elements[0], elements[1]);
+		Method method= AbstractTextSearchResult.class.getDeclaredMethod("removeElements", List.class);


The test uses getDeclaredMethod("removeElements", List.class) to access the method, but the actual method signature is void removeElements(Collection<?> elements). This mismatch will cause a NoSuchMethodException at runtime. Change the parameter type to Collection.class to match the actual method signature.

Suggested change

Method method= AbstractTextSearchResult.class.getDeclaredMethod("removeElements", List.class);

Method method= AbstractTextSearchResult.class.getDeclaredMethod("removeElements", java.util.Collection.class);

Copilot · 2026-02-20T15:14:31Z

tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java

@@ -0,0 +1,66 @@
+package org.eclipse.search.tests;


Missing copyright header. All Eclipse project source files should include a standard copyright header at the top of the file. Add the standard Eclipse copyright header similar to other files in the codebase.

mehmet-karaman force-pushed the batch_remove_search_matches branch from 2a62c61 to 73cab0d Compare February 17, 2026 14:52

mehmet-karaman force-pushed the batch_remove_search_matches branch 4 times, most recently from dff7693 to 300ca6b Compare February 17, 2026 23:07

iloveeclipse requested changes Feb 18, 2026

View reviewed changes

mehmet-karaman force-pushed the batch_remove_search_matches branch from 300ca6b to fe5e861 Compare February 18, 2026 22:21

mehmet-karaman commented Feb 18, 2026

View reviewed changes

bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java Outdated Show resolved Hide resolved

mehmet-karaman force-pushed the batch_remove_search_matches branch from fe5e861 to 7f02394 Compare February 18, 2026 23:19

iloveeclipse requested a review from Copilot February 19, 2026 08:03

Copilot started reviewing on behalf of iloveeclipse February 19, 2026 08:04 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

fedejeanne mentioned this pull request Feb 19, 2026

Search view with millions of matches is freezing the UI #3735

Open

mehmet-karaman force-pushed the batch_remove_search_matches branch 2 times, most recently from 4cd364f to b53503b Compare February 19, 2026 11:20

mehmet-karaman changed the title ~~Search View: Batch removing of search matches~~ Search View: Performance issues on remove and sort Feb 19, 2026

iloveeclipse requested a review from Copilot February 19, 2026 11:56

Copilot started reviewing on behalf of iloveeclipse February 19, 2026 11:56 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java Outdated Show resolved Hide resolved

bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java Outdated Show resolved Hide resolved

mehmet-karaman force-pushed the batch_remove_search_matches branch 2 times, most recently from 17ec4af to fc2f401 Compare February 19, 2026 12:46

iloveeclipse reviewed Feb 19, 2026

View reviewed changes

bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java Outdated Show resolved Hide resolved

iloveeclipse force-pushed the batch_remove_search_matches branch from fc2f401 to 40ee202 Compare February 19, 2026 13:48

iloveeclipse requested a review from Copilot February 19, 2026 14:12

Copilot started reviewing on behalf of iloveeclipse February 19, 2026 14:12 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

iloveeclipse force-pushed the batch_remove_search_matches branch from 40ee202 to 7d37158 Compare February 19, 2026 15:13

mehmet-karaman force-pushed the batch_remove_search_matches branch from 7d37158 to d572e09 Compare February 20, 2026 11:22

iloveeclipse reviewed Feb 20, 2026

View reviewed changes

bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java Outdated Show resolved Hide resolved

iloveeclipse reviewed Feb 20, 2026

View reviewed changes

Refactoring based on review comments

48e5747

iloveeclipse requested a review from Copilot February 20, 2026 15:08

Copilot started reviewing on behalf of iloveeclipse February 20, 2026 15:09 View session

Copilot AI reviewed Feb 20, 2026

View reviewed changes

Fixing hash collisions with counters

7b15304

	* @return {@code true} if the result is not empty
	* Returns {@code true} if the given element has matches.



		@Test
		public void testBatchRemoveElements() throws Exception {

	if(resource instanceof IContainer container) {
	if (resource instanceof IContainer container) {

	Method method= AbstractTextSearchResult.class.getDeclaredMethod("removeElements", List.class);
	Method method= AbstractTextSearchResult.class.getDeclaredMethod("removeElements", java.util.Collection.class);

Comments

Conversation

mehmet-karaman commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iloveeclipse commented Feb 17, 2026

Uh oh!

github-actions bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iloveeclipse Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

iloveeclipse Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

mehmet-karaman Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iloveeclipse Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

mehmet-karaman Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehmet-karaman Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

mehmet-karaman Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehmet-karaman commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iloveeclipse Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

iloveeclipse commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iloveeclipse Feb 20, 2026

Choose a reason for hiding this comment

mehmet-karaman commented Feb 17, 2026 •

edited

Loading

github-actions bot commented Feb 17, 2026 •

edited

Loading

mehmet-karaman Feb 18, 2026 •

edited

Loading

mehmet-karaman Feb 18, 2026 •

edited

Loading

mehmet-karaman Feb 19, 2026 •

edited

Loading

mehmet-karaman commented Feb 20, 2026 •

edited

Loading

iloveeclipse commented Feb 20, 2026 •

edited

Loading