File search: don't re-evaluate match filters in the UI thread - #4338
Open
iloveeclipse wants to merge 1 commit into
Open
File search: don't re-evaluate match filters in the UI thread#4338iloveeclipse wants to merge 1 commit into
iloveeclipse wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Cache publication can race with project-change invalidation and retain stale filter states.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Optimizes file-search filtering to prevent UI freezes caused by redundant match-filter evaluation.
Changes:
- Reuses cached match filter state during tree updates.
- Caches outer-project filter results with resource-change invalidation.
- Adds nested-project filtering tests.
File summaries
| File | Description |
|---|---|
FileTreeContentProvider.java |
Optimizes incremental tree updates. |
OuterProjectFileFilter.java |
Adds per-file filter caching and invalidation. |
NestedProjectFilterTest.java |
Tests nested-project filtering behavior. |
AllFileSearchTests.java |
Registers the new test class. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
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 eclipse-platform/eclipse.platform.text#144). Fixes eclipse-platform#4337 Assisted-by: Github Copilot (Claude Opus 5)
iloveeclipse
force-pushed
the
ui_freeze_on_search
branch
from
September 2, 2026 16:27
8d63d1e to
4e8f78b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Cache invalidation, listener lifecycle, and multi-file enumeration still have correctness and performance issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
Comment on lines
+57
to
+59
| private volatile Map<IFile, Boolean> filterStates = newFilterStates(); | ||
|
|
||
| private final AtomicBoolean isListening = new AtomicBoolean(); |
Comment on lines
+75
to
+80
| private void ensureListeningToProjectChanges() { | ||
| if (!isListening.get() && isListening.compareAndSet(false, true)) { | ||
| ResourcesPlugin.getWorkspace().addResourceChangeListener(projectChangeListener, | ||
| IResourceChangeEvent.POST_CHANGE); | ||
| } | ||
| } |
Comment on lines
+86
to
+92
| 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) { |
Comment on lines
+340
to
+343
| LineElement lineElement = ((FileMatch) match).getLineElement(); | ||
| if (updatedLines.contains(lineElement) && linesWithMatches.add(lineElement) | ||
| && linesWithMatches.size() == updatedLines.size()) { | ||
| return linesWithMatches; // all updated lines have matches |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 eclipse-platform/eclipse.platform.text#144).
Fixes #4337
Assisted-by: Github Copilot (Claude Opus 5)