synchronize write access to HUImpl.ToolHighlights (part of IJPL-775 "unreachable code" highlighting sometimes stuck when git "preview diff" is checked)

GitOrigin-RevId: 0211724f8bc91003537ec88733c606a7fad4c6b2
This commit is contained in:
Alexey Kudravtsev
2024-05-21 16:07:29 +02:00
committed by intellij-monorepo-bot
parent 54226c6bf4
commit c020b4193e
3 changed files with 80 additions and 65 deletions

View File

@@ -285,6 +285,30 @@ final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater implements Dis
@NotNull HighlightingSession session) {
Map<Object, ToolHighlights> data = getData(psiFile, hostDocument);
ToolHighlights toolHighlights = newInfos.isEmpty() ? data.get(toolId) : data.computeIfAbsent(toolId, __ -> new ToolHighlights());
runSynchronized(toolHighlights, () -> {
List<? extends HighlightInfo> oldInfos = toolHighlights == null ? null : toolHighlights.elementHighlights.get(visitedPsiElement);
List<? extends HighlightInfo> newInfosToStore;
if (oldInfos != null || !newInfos.isEmpty()) {
newInfosToStore = List.copyOf(newInfos);
if (LOG.isDebugEnabled()) {
//noinspection removal
LOG.debug("psiElementVisited: " + visitedPsiElement + " in " + visitedPsiElement.getTextRange() +
(psiFile.getViewProvider() instanceof InjectedFileViewProvider ?
" injected in " + InjectedLanguageManager.getInstance(project).injectedToHost(psiFile, psiFile.getTextRange()) : "") +
"; tool:" + toolId + "; infos:" + newInfosToStore + "; oldInfos:" + oldInfos + "; document:" + hostDocument);
}
MarkupModelEx markup = (MarkupModelEx)DocumentMarkupModel.forDocument(hostDocument, project, true);
setHighlightersInRange(newInfosToStore, oldInfos, markup, session);
}
else {
newInfosToStore = List.of();
}
// store back only after markup model changes are applied to avoid PCE thrown in the middle leaving corrupted data behind
putInfosForVisitedPsi(data, toolId, visitedPsiElement, newInfosToStore, toolHighlights);
});
}
private void runSynchronized(@Nullable ToolHighlights toolHighlights, @NotNull Runnable runnable) {
// Sometimes multiple file editors are submitted for highlighting, some of which may have the same underlying document,
// e.g., when the editor for file v is opened along with the git log with "preview diff" for the same file.
// In this case, it's possible that several instances of e.g., LocalInspectionPass can run in parallel,
@@ -292,27 +316,9 @@ final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater implements Dis
// so we need to guard `ToolHighlights` against parallel modification:
Object monitor = toolHighlights == null ? this : toolHighlights;
synchronized (monitor) {
List<? extends HighlightInfo> oldInfos = toolHighlights == null ? null : toolHighlights.elementHighlights.get(visitedPsiElement);
if (oldInfos != null || !newInfos.isEmpty()) {
newInfos = List.copyOf(newInfos);
if (LOG.isDebugEnabled()) {
//noinspection removal
LOG.debug("psiElementVisited: " + visitedPsiElement + " in " + visitedPsiElement.getTextRange() +
(psiFile.getViewProvider() instanceof InjectedFileViewProvider ?
" injected in " + InjectedLanguageManager.getInstance(project).injectedToHost(psiFile, psiFile.getTextRange()) : "") +
"; tool:" + toolId + "; infos:" + newInfos + "; oldInfos:" + oldInfos + "; document:" + hostDocument);
}
MarkupModelEx markup = (MarkupModelEx)DocumentMarkupModel.forDocument(hostDocument, project, true);
setHighlightersInRange(newInfos, oldInfos, markup, session);
}
else {
newInfos = List.of();
}
// store back only after markup model changes are applied to avoid PCE thrown in the middle leaving corrupted data behind
putInfosForVisitedPsi(data, toolId, visitedPsiElement, newInfos, toolHighlights);
runnable.run();
}
}
private static void setHighlightersInRange(@NotNull List<? extends HighlightInfo> newInfos,
@Nullable List<? extends HighlightInfo> oldInfos,
@NotNull MarkupModelEx markup,
@@ -371,35 +377,37 @@ final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater implements Dis
// remove all highlight infos from `data` generated by tools absent in 'actualToolsRun'
static void removeHighlightsForObsoleteTools(@NotNull PsiFile containingFile,
@NotNull Document hostDocument,
@NotNull List<? extends PsiFile> injectedFragments,
@NotNull Set<? extends Pair<Object, PsiFile>> actualToolsRun,
@NotNull HighlightingSession highlightingSession) {
for (PsiFile psiFile: ContainerUtil.append(injectedFragments, containingFile)) {
getData(psiFile, hostDocument).entrySet().removeIf(entry -> {
Object toolId = entry.getKey();
ToolHighlights toolHighlights = entry.getValue();
if (UNKNOWN_ID.equals(toolId) || !isInspectionToolId(toolId)) {
return false;
}
void removeHighlightsForObsoleteTools(@NotNull PsiFile containingFile,
@NotNull Document hostDocument,
@NotNull List<? extends PsiFile> injectedFragments,
@NotNull Set<? extends Pair<Object, PsiFile>> actualToolsRun,
@NotNull HighlightingSession highlightingSession) {
synchronized (this) {
for (PsiFile psiFile: ContainerUtil.append(injectedFragments, containingFile)) {
getData(psiFile, hostDocument).entrySet().removeIf(entry -> {
Object toolId = entry.getKey();
ToolHighlights toolHighlights = entry.getValue();
if (UNKNOWN_ID.equals(toolId) || !isInspectionToolId(toolId)) {
return false;
}
if (actualToolsRun.contains(Pair.create(toolId, psiFile))) {
return false;
}
for (List<? extends HighlightInfo> highlights : toolHighlights.elementHighlights.values()) {
for (HighlightInfo info : highlights) {
RangeHighlighterEx highlighter = info.highlighter;
if (highlighter != null) {
if (LOG.isTraceEnabled()) {
LOG.trace("removeHighlightsForObsoleteTools: " + highlighter);
if (actualToolsRun.contains(Pair.create(toolId, psiFile))) {
return false;
}
for (List<? extends HighlightInfo> highlights : toolHighlights.elementHighlights.values()) {
for (HighlightInfo info : highlights) {
RangeHighlighterEx highlighter = info.highlighter;
if (highlighter != null) {
if (LOG.isTraceEnabled()) {
LOG.trace("removeHighlightsForObsoleteTools: " + highlighter);
}
UpdateHighlightersUtil.disposeWithFileLevelIgnoreErrors(highlighter, info, highlightingSession);
}
UpdateHighlightersUtil.disposeWithFileLevelIgnoreErrors(highlighter, info, highlightingSession);
}
}
}
return true;
});
return true;
});
}
}
}
@@ -408,7 +416,7 @@ final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater implements Dis
}
// TODO very dirty method which throws all incrementality away, but we'd need to rewrite too many inspections to get rid of it
static void removeWarningsInsideErrors(@NotNull List<? extends PsiFile> injectedFragments,
void removeWarningsInsideErrors(@NotNull List<? extends PsiFile> injectedFragments,
@NotNull Document hostDocument,
@NotNull HighlightingSession highlightingSession) {
HighlightersRecycler recycler = new HighlightersRecycler();
@@ -441,20 +449,22 @@ final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater implements Dis
recycler.recycleHighlighter(highlighter);
ToolHighlights elementHighlights = map.get(info.toolId);
for (Map.Entry<PsiElement, List<? extends HighlightInfo>> elementEntry : elementHighlights.elementHighlights.entrySet()) {
List<? extends HighlightInfo> infos = elementEntry.getValue();
int i = infos.indexOf(info);
if (i != -1) {
List<HighlightInfo> listMinusInfo = ContainerUtil.concat(infos.subList(0, i), infos.subList(i + 1, infos.size()));
if (listMinusInfo.isEmpty()) {
elementHighlights.elementHighlights.remove(elementEntry.getKey());
runSynchronized(elementHighlights, () -> {
for (Map.Entry<PsiElement, List<? extends HighlightInfo>> elementEntry : elementHighlights.elementHighlights.entrySet()) {
List<? extends HighlightInfo> infos = elementEntry.getValue();
int i = infos.indexOf(info);
if (i != -1) {
List<HighlightInfo> listMinusInfo = ContainerUtil.concat(infos.subList(0, i), infos.subList(i + 1, infos.size()));
if (listMinusInfo.isEmpty()) {
elementHighlights.elementHighlights.remove(elementEntry.getKey());
}
else {
elementEntry.setValue(listMinusInfo);
}
break;
}
else {
elementEntry.setValue(listMinusInfo);
}
break;
}
}
});
}
}
return true;
@@ -471,7 +481,7 @@ final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater implements Dis
* after inspections completed, save their latencies (from corresponding {@link InspectionRunner.InspectionContext#holder})
* to use later in {@link com.intellij.codeInsight.daemon.impl.InspectionProfilerDataHolder#sortByLatencies(PsiFile, List, HighlightInfoUpdaterImpl)}
*/
static void saveLatencies(@NotNull PsiFile psiFile, @NotNull Map<Object, ToolLatencies> latencies) {
void saveLatencies(@NotNull PsiFile psiFile, @NotNull Map<Object, ToolLatencies> latencies) {
if (!psiFile.getViewProvider().isPhysical()) {
// ignore editor text fields/consoles etc.
return;
@@ -483,10 +493,13 @@ final class HighlightInfoUpdaterImpl extends HighlightInfoUpdater implements Dis
ToolHighlights toolHighlights = map.get(toolId);
// no point saving latencies if nothing was reported
if (toolHighlights == null) continue;
ToolLatencies lats = entry.getValue();
toolHighlights.latencies = new ToolLatencies(merge(toolHighlights.latencies.errorLatency, lats.errorLatency),
merge(toolHighlights.latencies.warningLatency, lats.warningLatency),
merge(toolHighlights.latencies.otherLatency, lats.otherLatency));
runSynchronized(toolHighlights, () -> {
ToolLatencies lats = entry.getValue();
toolHighlights.latencies = new ToolLatencies(merge(toolHighlights.latencies.errorLatency, lats.errorLatency),
merge(toolHighlights.latencies.warningLatency, lats.warningLatency),
merge(toolHighlights.latencies.otherLatency, lats.otherLatency));
});
}
}

View File

@@ -28,7 +28,7 @@ class InspectionProfilerDataHolder {
Math.max(0, context.holder().toolStamps.errorStamp - context.holder().toolStamps.initTimeStamp),
Math.max(0, context.holder().toolStamps.warningStamp - context.holder().toolStamps.initTimeStamp),
Math.max(0, context.holder().toolStamps.otherStamp - context.holder().toolStamps.initTimeStamp))));
HighlightInfoUpdaterImpl.saveLatencies(psiFile, latencies);
highlightInfoUpdater.saveLatencies(psiFile, latencies);
}
/**

View File

@@ -147,8 +147,10 @@ public final class LocalInspectionsPass extends ProgressableTextEditorHighlighti
injectedFragments = runner.getInjectedFragments();
}
Set<Pair<Object, PsiFile>> pairs = ContainerUtil.map2Set(resultContexts, context -> Pair.create(context.tool().getShortName(), context.psiFile()));
HighlightInfoUpdaterImpl.removeHighlightsForObsoleteTools(getFile(), getDocument(), injectedFragments, pairs, getHighlightingSession());
HighlightInfoUpdaterImpl.removeWarningsInsideErrors(injectedFragments, getDocument(), getHighlightingSession()); // must be the last
if (myHighlightInfoUpdater instanceof HighlightInfoUpdaterImpl impl) {
impl.removeHighlightsForObsoleteTools(getFile(), getDocument(), injectedFragments, pairs, getHighlightingSession());
impl.removeWarningsInsideErrors(injectedFragments, getDocument(), getHighlightingSession()); // must be the last
}
}
private static final TextAttributes NONEMPTY_TEXT_ATTRIBUTES = new UnmodifiableTextAttributes(){