diff options
author | Joerg Kubitz | 2021-09-09 07:02:58 +0000 |
---|---|---|
committer | Jörg Kubitz | 2021-09-22 10:50:06 +0000 |
commit | f3831f0ee7f44eddb3ae75fcb0c7b3fd2f08f083 (patch) | |
tree | d9658ae48512405b6428968fd7a9ea7e7e9c72ee | |
parent | 60e9c94534cbfa3ec5ea59b66d18dd9d15f3ed0d (diff) | |
download | eclipse.platform.text-f3831f0ee7f44eddb3ae75fcb0c7b3fd2f08f083.tar.gz eclipse.platform.text-f3831f0ee7f44eddb3ae75fcb0c7b3fd2f08f083.tar.xz eclipse.platform.text-f3831f0ee7f44eddb3ae75fcb0c7b3fd2f08f083.zip |
Bug 575893 - [performance] improve file search: avoid synchronizationI20210924-0200I20210923-1800I20210922-1800
FileSearchQuery did spend the most time in waiting for synchronization.
By using a ConcurrentHashMap the lock congestion can be avoided
Requires flushMatches AFTER searching a file (i.e of the single current
file of the thread) - not at start (i.e. of all other files which are
handled in parallel).
Change-Id: If412e738dcf3a36034836c9a2f0a2af1af839ddd
Signed-off-by: Joerg Kubitz <jkubitz-eclipse@gmx.de>
Reviewed-on: https://git.eclipse.org/r/c/platform/eclipse.platform.text/+/185196
Tested-by: Platform Bot <platform-bot@eclipse.org>
3 files changed, 50 insertions, 56 deletions
diff --git a/org.eclipse.search/new search/org/eclipse/search/core/text/TextSearchRequestor.java b/org.eclipse.search/new search/org/eclipse/search/core/text/TextSearchRequestor.java index eca7b53c9e7..a9c49e80b5c 100644 --- a/org.eclipse.search/new search/org/eclipse/search/core/text/TextSearchRequestor.java +++ b/org.eclipse.search/new search/org/eclipse/search/core/text/TextSearchRequestor.java @@ -101,6 +101,21 @@ public abstract class TextSearchRequestor { } /** + * Notification that the matches of the given file should be flushed. The + * default behaviour is to ignore this notification. Implementors can use + * this notification to update the progress after a file was searched. + * Otherwise the progress may not be visible until all files have been + * searched. + * + * @param file + * the file that was just processed. + * @since 3.14 + */ + public void flushMatches(IFile file) { + // do nothing + } + + /** * Notification sent that a file might contain binary context. * It is the choice of the search engine to report binary files and it is the heuristic of the search engine to decide * that a file could be binary. diff --git a/org.eclipse.search/search/org/eclipse/search/internal/core/text/TextSearchVisitor.java b/org.eclipse.search/search/org/eclipse/search/internal/core/text/TextSearchVisitor.java index 01a9296c396..88f846a0ac2 100644 --- a/org.eclipse.search/search/org/eclipse/search/internal/core/text/TextSearchVisitor.java +++ b/org.eclipse.search/search/org/eclipse/search/internal/core/text/TextSearchVisitor.java @@ -239,6 +239,7 @@ public class TextSearchVisitor { break; } } + fCollector.flushMatches(file); } else { if (charsequenceForPreviousLocation != null) { try { diff --git a/org.eclipse.search/search/org/eclipse/search/internal/ui/text/FileSearchQuery.java b/org.eclipse.search/search/org/eclipse/search/internal/ui/text/FileSearchQuery.java index 721662cd70d..039d8babed3 100644 --- a/org.eclipse.search/search/org/eclipse/search/internal/ui/text/FileSearchQuery.java +++ b/org.eclipse.search/search/org/eclipse/search/internal/ui/text/FileSearchQuery.java @@ -19,9 +19,8 @@ package org.eclipse.search.internal.ui.text; import java.util.ArrayList; -import java.util.HashMap; -import java.util.Iterator; -import java.util.Map; +import java.util.List; +import java.util.concurrent.ConcurrentHashMap; import java.util.regex.Pattern; import org.eclipse.core.runtime.CoreException; @@ -55,15 +54,15 @@ public class FileSearchQuery implements ISearchQuery { private final boolean fSearchInBinaries; private final boolean fIsLightweightAutoRefresh; - private Map<IFile, ArrayList<FileMatch>> fCachedMatches; - private Object fLock= new Object(); + private final ConcurrentHashMap<IFile, ArrayList<FileMatch>> fCachedMatches; + private volatile boolean stop; private TextSearchResultCollector(AbstractTextSearchResult result, boolean isFileSearchOnly, boolean searchInBinaries) { fResult= result; fIsFileSearchOnly= isFileSearchOnly; fSearchInBinaries= searchInBinaries; fIsLightweightAutoRefresh= Platform.getPreferencesService().getBoolean(ResourcesPlugin.PI_RESOURCES, ResourcesPlugin.PREF_LIGHTWEIGHT_AUTO_REFRESH, false, null); - + fCachedMatches = new ConcurrentHashMap<>(); } @Override @@ -77,11 +76,8 @@ public class FileSearchQuery implements ISearchQuery { return false; if (fIsFileSearchOnly) { - synchronized (fLock) { - fResult.addMatch(new FileMatch(file)); - } + fResult.addMatch(new FileMatch(file)); } - flushMatches(); return true; } @@ -92,45 +88,23 @@ public class FileSearchQuery implements ISearchQuery { @Override public boolean acceptPatternMatch(TextSearchMatchAccess matchRequestor) throws CoreException { - ArrayList<FileMatch> matches; - synchronized(fLock) { - // fCachedMatches is set to null when the caller invokes endReporting(), - // indicating that no further results are desired/expected, so discard - // any additional results. - if (fCachedMatches == null) { - return false; - } - matches= fCachedMatches.get(matchRequestor.getFile()); + if (stop) { + return false; } - - int matchOffset= matchRequestor.getMatchOffset(); - - /* - * Another job may call flushCaches() at any time, which will clear the cached matches. - * Any addition of matches to the cache needs to be protected against the flushing of - * the cache by other jobs. It is OK to call getLineElement() with an unprotected local - * reference to the matches for this file, because getLineElement() uses previous matches - * as an optimization when creating new matches but doesn't update the cache directly - * (and because each file is processed by at most one job). - */ - LineElement lineElement= getLineElement(matchOffset, matchRequestor, matches); - if (lineElement != null) { - FileMatch fileMatch= new FileMatch(matchRequestor.getFile(), matchOffset, matchRequestor.getMatchLength(), lineElement); - synchronized(fLock) { - // fCachedMatches is set to null when the caller invokes endReporting(), - // indicating that no further results are desired/expected, so discard - // any additional results. - if (fCachedMatches == null) { - return false; - } - matches= fCachedMatches.get(matchRequestor.getFile()); + fCachedMatches.compute(matchRequestor.getFile(), (f, matches) -> { + // each file is processed by at most one job + int matchOffset = matchRequestor.getMatchOffset(); + LineElement lineElement = getLineElement(matchOffset, matchRequestor, matches); + if (lineElement != null) { + FileMatch fileMatch = new FileMatch(matchRequestor.getFile(), matchOffset, + matchRequestor.getMatchLength(), lineElement); if (matches == null) { - matches= new ArrayList<>(); - fCachedMatches.put(matchRequestor.getFile(), matches); + matches = new ArrayList<>(); } matches.add(fileMatch); } - } + return matches; + }); return true; } @@ -191,28 +165,32 @@ public class FileSearchQuery implements ISearchQuery { @Override public void beginReporting() { - fCachedMatches= new HashMap<>(); + stop = false; } @Override public void endReporting() { + stop = true; flushMatches(); - synchronized (fLock) { - fCachedMatches= null; + fCachedMatches.clear(); + } + + @Override + public void flushMatches(IFile file) { + List<FileMatch> matches = fCachedMatches.remove(file); + if (matches != null && !matches.isEmpty()) { + fResult.addMatches(matches.toArray(new Match[matches.size()])); } } private void flushMatches() { - synchronized (fLock) { - if (fCachedMatches != null && !fCachedMatches.isEmpty()) { - Iterator<ArrayList<FileMatch>> it = fCachedMatches.values().iterator(); - while(it.hasNext()) { - ArrayList<FileMatch> matches= it.next(); - fResult.addMatches(matches.toArray(new Match[matches.size()])); - } - fCachedMatches.clear(); + fCachedMatches.values().removeIf(matches -> { + if (matches != null && !matches.isEmpty()) { + fResult.addMatches(matches.toArray(new Match[matches.size()])); + return true; } - } + return false; + }); } } |