Skip to content

Search View: Performance issues on remove and sort#3733

Open
mehmet-karaman wants to merge 1 commit intoeclipse-platform:masterfrom
mehmet-karaman:batch_remove_search_matches
Open

Search View: Performance issues on remove and sort#3733
mehmet-karaman wants to merge 1 commit intoeclipse-platform:masterfrom
mehmet-karaman:batch_remove_search_matches

Conversation

@mehmet-karaman
Copy link
Contributor

@mehmet-karaman mehmet-karaman commented Feb 17, 2026

  • 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.

Fixes #3735

@iloveeclipse
Copy link
Member

@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.

@mehmet-karaman mehmet-karaman force-pushed the batch_remove_search_matches branch from 2a62c61 to 73cab0d Compare February 17, 2026 14:52
@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2026

Test Results

 3 018 files   -  6   3 018 suites   - 6   2h 13m 53s ⏱️ - 2m 51s
 8 237 tests + 3   7 989 ✅ + 3  248 💤 ±0  0 ❌ ±0 
23 509 runs   - 17  22 721 ✅  - 14  788 💤  - 3  0 ❌ ±0 

Results for commit 6ffeee4. ± Comparison against base commit 89a19bf.

♻️ This comment has been updated with latest results.

@mehmet-karaman mehmet-karaman force-pushed the batch_remove_search_matches branch 4 times, most recently from dff7693 to 300ca6b Compare February 17, 2026 23:07
@mehmet-karaman mehmet-karaman force-pushed the batch_remove_search_matches branch from 300ca6b to fe5e861 Compare February 18, 2026 22:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mehmet-karaman 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 mehmet-karaman changed the title Search View: Batch removing of search matches Search View: Performance issues on remove and sort Feb 19, 2026
@iloveeclipse iloveeclipse requested a review from Copilot February 19, 2026 11:56
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iloveeclipse iloveeclipse force-pushed the batch_remove_search_matches branch from 40ee202 to 7d37158 Compare February 19, 2026 15:13
@mehmet-karaman mehmet-karaman force-pushed the batch_remove_search_matches branch from 7d37158 to d572e09 Compare February 20, 2026 11:22
@mehmet-karaman
Copy link
Contributor Author

I am going to try to reproduce the failure locally..

@iloveeclipse
Copy link
Member

Seems that the default is table view and not tree.

I assume this be forced by using right preference for the search, but I haven't checked what is the probllem with the test. Note: you must "process UI events loop" in order to see fully updated search view after some search job is executed, this could be the problem you observe.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

@mehmet-karaman mehmet-karaman force-pushed the batch_remove_search_matches branch 2 times, most recently from d53e827 to 47c1437 Compare February 23, 2026 22:08
@mehmet-karaman
Copy link
Contributor Author

Seems that the default is table view and not tree.

I assume this be forced by using right preference for the search, but I haven't checked what is the probllem with the test. Note: you must "process UI events loop" in order to see fully updated search view after some search job is executed, this could be the problem you observe.

Your guess was correct, It was failing due to pending UI events.. The tests are running now, but another test is failing. Could be that I have to set the default value back in tearDown.. This is also pushed now.

@mehmet-karaman mehmet-karaman force-pushed the batch_remove_search_matches branch 2 times, most recently from 26d440a to c7599f8 Compare February 24, 2026 11:10
@iloveeclipse iloveeclipse requested a review from Copilot February 25, 2026 07:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

@mehmet-karaman mehmet-karaman force-pushed the batch_remove_search_matches branch 2 times, most recently from 07fe961 to 127e48e Compare February 25, 2026 09:31
@iloveeclipse iloveeclipse requested a review from Copilot February 25, 2026 09:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


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

@mehmet-karaman mehmet-karaman force-pushed the batch_remove_search_matches branch from 66dda3f to 27539cf Compare February 25, 2026 13:12


- 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 mehmet-karaman force-pushed the batch_remove_search_matches branch from 27539cf to 6ffeee4 Compare February 26, 2026 09:51
@mehmet-karaman
Copy link
Contributor Author

Resolved all comments .. (force pushed one last commit hopefully)

@mehmet-karaman
Copy link
Contributor Author

I've ran the RcpTestSuite locally many times and never run into a timeout.

@iloveeclipse
Copy link
Member

I've ran the RcpTestSuite locally many times and never run into a timeout.

As discussed, please check existing tickets and if there is no one, report this test and if there is one, check the test failure whether it is still same or not.

@mehmet-karaman
Copy link
Contributor Author

mehmet-karaman commented Feb 26, 2026

Due to hint of @vogella this was a known issue: #1517

I guess we are done with this PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Search view with millions of matches is freezing the UI

3 participants