Search View: Performance issues on remove and sort#3733
Search View: Performance issues on remove and sort#3733mehmet-karaman wants to merge 1 commit intoeclipse-platform:masterfrom
Conversation
|
@mehmet-karaman : please first create a ticket and explain the problem. |
2a62c61 to
73cab0d
Compare
dff7693 to
300ca6b
Compare
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
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/AbstractTextSearchViewPage.java
Outdated
Show resolved
Hide resolved
300ca6b to
fe5e861
Compare
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
fe5e861 to
7f02394
Compare
There was a problem hiding this comment.
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 selectedIFile/IContainerresources 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
Outdated
Show resolved
Hide resolved
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/AbstractTextSearchViewPage.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
4cd364f to
b53503b
Compare
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
17ec4af to
fc2f401
Compare
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
fc2f401 to
40ee202
Compare
There was a problem hiding this comment.
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.
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java
Outdated
Show resolved
Hide resolved
40ee202 to
7d37158
Compare
7d37158 to
d572e09
Compare
|
I am going to try to reproduce the failure locally.. |
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. |
There was a problem hiding this comment.
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.
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Outdated
Show resolved
Hide resolved
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Show resolved
Hide resolved
d53e827 to
47c1437
Compare
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. |
26d440a to
c7599f8
Compare
There was a problem hiding this comment.
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.
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java
Outdated
Show resolved
Hide resolved
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Outdated
Show resolved
Hide resolved
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Show resolved
Hide resolved
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java
Outdated
Show resolved
Hide resolved
07fe961 to
127e48e
Compare
There was a problem hiding this comment.
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.
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Outdated
Show resolved
Hide resolved
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Show resolved
Hide resolved
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Outdated
Show resolved
Hide resolved
127e48e to
66dda3f
Compare
There was a problem hiding this comment.
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.
tests/org.eclipse.search.tests/src/org/eclipse/search/tests/TextSearchResultTest.java
Show resolved
Hide resolved
66dda3f to
27539cf
Compare
- 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>
27539cf to
6ffeee4
Compare
|
Resolved all comments .. (force pushed one last commit hopefully) |
|
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. |
which causes performance issues
only the visible elements (while all others are filtered by the limit
number)
ConcurrentHashMap.newKeySet() with the ConcurrentSkipListSet with the
comparator and makes the additional sorting obsolete.
Fixes #3735