Repository navigation
File search: don't re-evaluate match filters in the UI thread - #4338
Open
iloveeclipse wants to merge 1 commit into
Open
iloveeclipse 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
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
iloveeclipse
force-pushed
the
ui_freeze_on_search
branch
from
September 10, 2026 15:43
4e8f78b to
e66ee74
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The cache can remain stale after linked-folder content changes, and its permanent workspace listener lacks lifecycle cleanup.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/OuterProjectFileFilter.java:95
- Ignoring additions/removals below linked folders leaves this new static cache stale even after a search is rerun. New matches are recomputed, but filtering them still looks up the same equal
IFilekey infilterStates, socomputeIsFilteredis skipped and the pre-change answer can keep showing a duplicate (or hiding the remaining representation). Invalidate these topology-changing deltas, or scope the cache to a search-result lifetime so a rerun starts with fresh states.
* Not reported is the creation or deletion of a file below a linked folder,
* although such a file can be represented by more than one project as well.
* Those changes are not reflected by the filter state cached per match by
* {@link org.eclipse.search.ui.text.AbstractTextSearchResult} either, the
* matches of the affected files are recomputed when the search is run again.
* Invalidating the states for every added or removed file would discard them
* whenever a build or a refresh touches the workspace, which would defeat the
* purpose of remembering them.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Linked-folder child changes can leave static cached filter states stale across subsequent searches.
Review details
Suppressed comments (1)
bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/OuterProjectFileFilter.java:92
- This exception leaves the static cache stale across later searches. If a file was cached before an add/remove below a linked folder changes the set returned by
findFilesForLocationURI, rerunning the search still calls this filter with the sameIFilekey and receives the old map entry instead of recomputing it; a weak key does not help while an older search result still retains that file. Thus the comment's claimed recovery on rerun does not occur, and the wrong project representation can remain visible/hidden until some unrelated project-level invalidation. Please invalidate for these alias-changing deltas or scope/version the cache so a new search cannot reuse such entries.
* Not reported is the creation or deletion of a file below a linked folder,
* although such a file can be represented by more than one project as well.
* Those changes are not reflected by the filter state cached per match by
* {@link org.eclipse.search.ui.text.AbstractTextSearchResult} either, the
* matches of the affected files are recomputed when the search is run again.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
iloveeclipse
force-pushed
the
ui_freeze_on_search
branch
from
September 10, 2026 16:08
e66ee74 to
7746bbe
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The workspace listener lacks lifecycle cleanup, and the cache-reset test is invalidated by a different mechanism.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
bundles/org.eclipse.search/search/org/eclipse/search/internal/ui/text/OuterProjectFileFilter.java:148
- This listener is registered against the workspace for the lifetime of the JVM but is never removed when
org.eclipse.searchstops. Because the workspace keeps a strong reference to the lambda (and therefore this bundle's class loader), updating/restarting the bundle leaks the old bundle and can leave multiple listeners active. Register/unregister it withSearchPlugin's lifecycle (or add a shutdown hook called fromSearchPlugin.stop) instead.
ResourcesPlugin.getWorkspace().addResourceChangeListener(PROJECT_CHANGE_LISTENER,
IResourceChangeEvent.POST_CHANGE);
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
iloveeclipse
force-pushed
the
ui_freeze_on_search
branch
from
September 30, 2026 11:38
7746bbe to
4d7d7dc
Compare
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. Relying on the computed filter state required fixing a race in AbstractTextSearchResult: didAddMatch(..) evaluated the filters for a new match before adding it, while setActiveMatchFilters(..) updates only the matches that are already added. A match that was evaluated with the previous filters but added after the filters had been changed kept an outdated filter state. The re-evaluation in the UI thread masked that for the updates of the tree, but not for its initialization, the table or the displayed match counts. Changing the filters and adding a match are now mutually exclusive. The filters are changed while the write lock of a StampedLock is held, which also increments a filter generation. Matches are added while its read lock is held, so the search threads don't block each other. A match is only added if the generation hasn't changed since its filter state was evaluated, otherwise it is evaluated again with the new filters. The (potentially expensive) filters are evaluated without holding the lock, and no filter or other overridable method is called while it is held, so subclasses synchronizing their accessors (like FileSearchResult) can't deadlock with it. updateFilterStateForAllMatches() now reads the active filters once instead of once per match. 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(), matches and updates refer to the very same instance), 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. The state doesn't depend on the filter instance, therefore the map and the resource change listener that invalidates it are static: at most one listener is registered, no matter how many filters are created. The listener discards all remembered states if projects are added, removed, opened, closed or moved, and if the description file of a project has changed. Linked resources, resource filters and virtual folders change which files represent a location, but they are not reported by the flags of a project delta (the platform's own AliasManager uses internal lifecycle events for them); they are however stored in the project description, which is written whenever they change. Looking for that single file in the delta is much cheaper than visiting the whole delta in the notification thread, and much less disruptive than discarding the states for every added or removed file. Since the file handles are equal for all searches, a remembered state must not outlive the search it was computed for: changes the listener cannot detect - a file added or removed below a linked folder, or a resource turned hidden or team private, which IResource#setHidden(..) and #setTeamPrivateMember(..) deliberately don't report in a delta - would otherwise still be answered from the map when the search is run again. FileSearchQuery#run(..) therefore discards the remembered states when a search is started. The listener is registered before the state of the workspace is read, and the registration is published only after it is done, so that a state is never computed and remembered while the changes that would invalidate it are not reported yet. 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. Added tests for OuterProjectFileFilter (missed in the original eclipse-platform/eclipse.platform.text#144), including the invalidation of the remembered states, and a test that the tree of the search view doesn't evaluate the match filters again. The test for the discarded states makes an undetectable change (a file turned team private) and asserts that the stale state survives it, so that it really verifies the discarding done by FileSearchQuery#run(..) and not the resource change listener. MatchFilterStateTest deterministically changes the filters while a match is being evaluated and verifies that the added match gets the filter state of the new filters, and that a match is evaluated only once if the filters don't change. Fixes eclipse-platform#4337 Assisted-by: Github Copilot (Claude Opus 5)
iloveeclipse
force-pushed
the
ui_freeze_on_search
branch
from
September 30, 2026 12:38
4d7d7dc to
23b7afc
Compare
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.
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(),
matches and updates refer to the very same instance), 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. The state doesn't depend on the filter instance, therefore the map and the resource change listener that invalidates it are static: at most one listener is registered, no matter how many filters are created.
The listener discards all remembered states if projects are added, removed, opened, closed or moved, and if the description file of a project has changed. Linked resources, resource filters and virtual folders change which files represent a location, but they are not reported by the flags of a project delta (the platform's own AliasManager uses internal lifecycle events for them); they are however stored in the project description, which is written whenever they change. Looking for that single file in the delta is much cheaper than visiting the whole delta in the notification thread, and much less disruptive than discarding the states for every added or removed file.
The listener is registered before the state of the workspace is read, and the registration is published only after it is done, so that a state is never computed and remembered while the changes that would invalidate
it are not reported yet. 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.
Added tests for OuterProjectFileFilter (missed in the original eclipse-platform/eclipse.platform.text#144), including the invalidation of the remembered states, and a test that the
tree of the search view doesn't evaluate the match filters again.
Fixes #4337
Assisted-by: Github Copilot (Claude Opus 5)