From 372b4d43a22356fceb7aca363625311b2aba08eb Mon Sep 17 00:00:00 2001 From: peter Date: Sat, 19 May 2018 08:22:25 +0200 Subject: [PATCH] don't invalidate all PSI on root change It was originally done to avoid UI freeze (IDEA-172762), but has led to too many PIEAEs (like IDEA-191185, IDEA-188292, IDEA-184186, EA-114990). Ideally those clients should all be converted to smart pointers, but that proved to be quite hard to do, especially without breaking API. And they mostly worked before those batch invalidations. So now we have a smarter way of dealing with this issue. On root change, we mark PSI as "potentially invalid", and then, when someone calls "isValid" (hopefully not for all PSI and hopefully in a background thread), we check if the old PSI is equivalent to the one that would be re-created in its place. If yes, we return valid. If no, we invalidate the old PSI forever and return the new one. --- .../psi/AbstractFileViewProvider.java | 17 +- .../psi/impl/file/PsiBinaryFileImpl.java | 22 ++- .../psi/impl/file/impl/FileManagerImpl.java | 153 ++++++++++++++---- .../intellij/psi/impl/source/PsiFileImpl.java | 25 ++- .../psi/impl/file/impl/PsiVFSListener.java | 2 +- 5 files changed, 171 insertions(+), 48 deletions(-) diff --git a/platform/core-impl/src/com/intellij/psi/AbstractFileViewProvider.java b/platform/core-impl/src/com/intellij/psi/AbstractFileViewProvider.java index 222036b5dd39..9e5ffdc34255 100644 --- a/platform/core-impl/src/com/intellij/psi/AbstractFileViewProvider.java +++ b/platform/core-impl/src/com/intellij/psi/AbstractFileViewProvider.java @@ -44,6 +44,7 @@ import com.intellij.psi.impl.file.PsiBinaryFileImpl; import com.intellij.psi.impl.file.PsiLargeBinaryFileImpl; import com.intellij.psi.impl.file.PsiLargeTextFileImpl; import com.intellij.psi.impl.file.impl.FileManager; +import com.intellij.psi.impl.file.impl.FileManagerImpl; import com.intellij.psi.impl.source.PsiFileImpl; import com.intellij.psi.impl.source.PsiPlainTextFileImpl; import com.intellij.psi.impl.source.SourceTreeToPsiMap; @@ -62,6 +63,7 @@ import java.lang.ref.SoftReference; import java.util.Collections; import java.util.List; import java.util.Set; +import java.util.function.Consumer; public abstract class AbstractFileViewProvider extends UserDataHolderBase implements FileViewProvider { private static final Logger LOG = Logger.getInstance("#com.intellij.psi.AbstractFileViewProvider"); @@ -73,7 +75,6 @@ public abstract class AbstractFileViewProvider extends UserDataHolderBase implem private final VirtualFile myVirtualFile; private final boolean myEventSystemEnabled; private final boolean myPhysical; - private boolean myInvalidated; private volatile Content myContent; private volatile Reference myDocument; @NotNull @@ -418,11 +419,13 @@ public abstract class AbstractFileViewProvider extends UserDataHolderBase implem public abstract List getKnownTreeRoots(); public final void markInvalidated() { - if (myInvalidated) return; - invalidateCachedPsi(); - myInvalidated = true; - invalidateCopies(); + forKnownCopies(copy -> myManager.getFileManager().setViewProvider(copy.getVirtualFile(), null)); + } + + public final void markPossiblyInvalidated() { + invalidateCachedPsi(); + forKnownCopies(FileManagerImpl::markPossiblyInvalidated); } private void invalidateCachedPsi() { @@ -433,12 +436,12 @@ public abstract class AbstractFileViewProvider extends UserDataHolderBase implem } } - private void invalidateCopies() { + private void forKnownCopies(Consumer action) { Set knownCopies = getUserData(KNOWN_COPIES); if (knownCopies != null) { for (AbstractFileViewProvider copy : knownCopies) { if (copy.getCachedPsiFiles().stream().anyMatch(f -> f.getOriginalFile().getViewProvider() == this)) { - myManager.getFileManager().setViewProvider(copy.getVirtualFile(), null); + action.accept(copy); } } } diff --git a/platform/core-impl/src/com/intellij/psi/impl/file/PsiBinaryFileImpl.java b/platform/core-impl/src/com/intellij/psi/impl/file/PsiBinaryFileImpl.java index 761ce0b25003..7426e2d8a5b1 100644 --- a/platform/core-impl/src/com/intellij/psi/impl/file/PsiBinaryFileImpl.java +++ b/platform/core-impl/src/com/intellij/psi/impl/file/PsiBinaryFileImpl.java @@ -24,6 +24,7 @@ import com.intellij.openapi.util.TextRange; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.psi.*; import com.intellij.psi.impl.*; +import com.intellij.psi.impl.file.impl.FileManagerImpl; import com.intellij.psi.impl.source.resolve.FileContextUtil; import com.intellij.psi.search.PsiElementProcessor; import com.intellij.util.ArrayUtil; @@ -39,11 +40,11 @@ public class PsiBinaryFileImpl extends PsiElementBase implements PsiBinaryFile, private final PsiManagerImpl myManager; private String myName; // for myFile == null only private byte[] myContents; // for myFile == null only - private final FileViewProvider myViewProvider; - private boolean myInvalidated; + private final AbstractFileViewProvider myViewProvider; + private volatile boolean myPossiblyInvalidated; public PsiBinaryFileImpl(PsiManagerImpl manager, FileViewProvider viewProvider) { - myViewProvider = viewProvider; + myViewProvider = (AbstractFileViewProvider)viewProvider; myManager = manager; } @@ -244,7 +245,18 @@ public class PsiBinaryFileImpl extends PsiElementBase implements PsiBinaryFile, @Override public boolean isValid() { if (isCopy()) return true; // "dummy" file - return getVirtualFile().isValid() && !myManager.getProject().isDisposed() && !myInvalidated; + if (!getVirtualFile().isValid() || myManager.getProject().isDisposed()) return false; + + + if (!myPossiblyInvalidated) return true; + + // synchronized by read-write action + if (((FileManagerImpl)myManager.getFileManager()).evaluateValidity(myViewProvider)) { + myPossiblyInvalidated = false; + PsiInvalidElementAccessException.setInvalidationTrace(this, null); + return true; + } + return false; } @Override @@ -317,7 +329,7 @@ public class PsiBinaryFileImpl extends PsiElementBase implements PsiBinaryFile, @Override public void markInvalidated() { - myInvalidated = true; + myPossiblyInvalidated = true; DebugUtil.onInvalidated(this); } } diff --git a/platform/core-impl/src/com/intellij/psi/impl/file/impl/FileManagerImpl.java b/platform/core-impl/src/com/intellij/psi/impl/file/impl/FileManagerImpl.java index bd061c388aef..d00d4cfdfc48 100644 --- a/platform/core-impl/src/com/intellij/psi/impl/file/impl/FileManagerImpl.java +++ b/platform/core-impl/src/com/intellij/psi/impl/file/impl/FileManagerImpl.java @@ -27,18 +27,13 @@ import com.intellij.openapi.fileEditor.FileDocumentManager; import com.intellij.openapi.fileTypes.FileType; import com.intellij.openapi.project.DumbService; import com.intellij.openapi.roots.FileIndexFacade; -import com.intellij.openapi.util.Disposer; -import com.intellij.openapi.util.Key; -import com.intellij.openapi.util.LowMemoryWatcher; +import com.intellij.openapi.util.*; import com.intellij.openapi.util.registry.Registry; import com.intellij.openapi.vfs.VfsUtilCore; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.openapi.vfs.VirtualFileVisitor; import com.intellij.psi.*; -import com.intellij.psi.impl.DebugUtil; -import com.intellij.psi.impl.PsiManagerImpl; -import com.intellij.psi.impl.PsiModificationTrackerImpl; -import com.intellij.psi.impl.PsiTreeChangeEventImpl; +import com.intellij.psi.impl.*; import com.intellij.psi.impl.file.PsiDirectoryFactory; import com.intellij.testFramework.LightVirtualFile; import com.intellij.util.ConcurrencyUtil; @@ -54,6 +49,7 @@ import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicReference; public class FileManagerImpl implements FileManager { + private static final Key IN_COMA = Key.create("IN_COMA"); private static final Logger LOG = Logger.getInstance("#com.intellij.psi.impl.file.impl.FileManagerImpl"); private final Key myPsiHardRefKey = Key.create("HARD_REFERENCE_TO_PSI"); //non-static! @@ -63,6 +59,11 @@ public class FileManagerImpl implements FileManager { private final AtomicReference> myVFileToPsiDirMap = new AtomicReference<>(); private final AtomicReference> myVFileToViewProviderMap = new AtomicReference<>(); + /** + * Holds thread-local temporary providers that are sometimes needed while checking if a file is valid + */ + private final ThreadLocal> myTempProviders = ThreadLocal.withInitial(() -> new HashMap<>()); + private boolean myDisposed; private final FileDocumentManager myFileDocumentManager; @@ -197,6 +198,12 @@ public class FileManagerImpl implements FileManager { if (file instanceof VirtualFileWindow) { throw new IllegalStateException("File " + file + " is invalid"); } + + Map tempMap = myTempProviders.get(); + if (tempMap.containsKey(file)) { + return Objects.requireNonNull(tempMap.get(file), "Recursive file view provider creation"); + } + viewProvider = createFileViewProvider(file, true); if (file instanceof LightVirtualFile) { return file.putUserDataIfAbsent(myPsiHardRefKey, viewProvider); @@ -206,21 +213,34 @@ public class FileManagerImpl implements FileManager { @Override public FileViewProvider findCachedViewProvider(@NotNull final VirtualFile file) { + FileViewProvider viewProvider = getRawCachedViewProvider(file); + + if (viewProvider instanceof AbstractFileViewProvider && viewProvider.getUserData(IN_COMA) != null) { + Map tempMap = myTempProviders.get(); + if (tempMap.containsKey(file)) { + return tempMap.get(file); + } + + if (!evaluateValidity((AbstractFileViewProvider)viewProvider)) { + return null; + } + } + return viewProvider; + } + + @Nullable + private FileViewProvider getRawCachedViewProvider(@NotNull VirtualFile file) { ConcurrentMap map = myVFileToViewProviderMap.get(); FileViewProvider viewProvider = map == null ? null : map.get(file); - if (viewProvider == null) viewProvider = file.getUserData(myPsiHardRefKey); - return viewProvider; + return viewProvider == null ? file.getUserData(myPsiHardRefKey) : viewProvider; } @Override public void setViewProvider(@NotNull final VirtualFile virtualFile, @Nullable final FileViewProvider fileViewProvider) { - FileViewProvider prev = findCachedViewProvider(virtualFile); + FileViewProvider prev = getRawCachedViewProvider(virtualFile); if (prev == fileViewProvider) return; if (prev != null) { - DebugUtil.performPsiModification(null, () -> { - markInvalidated(prev); - DebugUtil.onInvalidated(prev); - }); + DebugUtil.performPsiModification(null, () -> markInvalidated(prev)); } if (fileViewProvider == null) { @@ -272,7 +292,7 @@ public class FileManagerImpl implements FileManager { event.setPropertyName(PsiTreeChangeEvent.PROP_FILE_TYPES); myManager.beforePropertyChange(event); - invalidateAllPsi(); + possiblyInvalidatePhysicalPsi(); myManager.propertyChanged(event); }); @@ -283,12 +303,12 @@ public class FileManagerImpl implements FileManager { }); } - void invalidateAllPsi() { - myVFileToPsiDirMap.set(null); - for (final FileViewProvider provider : getVFileToViewProviderMap().values()) { - markInvalidated(provider); + void possiblyInvalidatePhysicalPsi() { + ApplicationManager.getApplication().assertWriteAccessAllowed(); + removeInvalidDirs(true); + for (FileViewProvider provider : getVFileToViewProviderMap().values()) { + markPossiblyInvalidated(provider); } - myVFileToViewProviderMap.set(null); } void dispatchPendingEvents() { @@ -301,6 +321,10 @@ public class FileManagerImpl implements FileManager { @TestOnly public void checkConsistency() { + for (VirtualFile file : new ArrayList<>(getVFileToViewProviderMap().keySet())) { + findCachedViewProvider(file); // complete delayed validity checks + } + Map fileToViewProvider = new HashMap<>(getVFileToViewProviderMap()); myVFileToViewProviderMap.set(null); for (Map.Entry entry : fileToViewProvider.entrySet()) { @@ -426,10 +450,18 @@ public class FileManagerImpl implements FileManager { } private void markInvalidated(@NotNull FileViewProvider viewProvider) { + viewProvider.putUserData(IN_COMA, null); ((AbstractFileViewProvider)viewProvider).markInvalidated(); viewProvider.getVirtualFile().putUserData(myPsiHardRefKey, null); } + public static void markPossiblyInvalidated(@NotNull FileViewProvider viewProvider) { + LOG.assertTrue(!(viewProvider instanceof FreeThreadedFileViewProvider)); + viewProvider.putUserData(IN_COMA, true); + ((AbstractFileViewProvider)viewProvider).markPossiblyInvalidated(); + clearPsiCaches(viewProvider); + } + @Nullable PsiFile getCachedPsiFileInner(@NotNull VirtualFile file) { FileViewProvider fileViewProvider = findCachedViewProvider(file); @@ -440,8 +472,11 @@ public class FileManagerImpl implements FileManager { @Override public List getAllCachedFiles() { List files = new ArrayList<>(); - for (FileViewProvider provider : getVFileToViewProviderMap().values()) { - ContainerUtil.addIfNotNull(files, ((AbstractFileViewProvider)provider).getCachedPsi(provider.getBaseLanguage())); + for (VirtualFile file : new ArrayList<>(getVFileToViewProviderMap().keySet())) { + FileViewProvider provider = findCachedViewProvider(file); + if (provider != null) { + ContainerUtil.addIfNotNull(files, ((AbstractFileViewProvider)provider).getCachedPsi(provider.getBaseLanguage())); + } } return files; } @@ -484,8 +519,8 @@ public class FileManagerImpl implements FileManager { continue; } + FileViewProvider view = fileToPsiFileMap.get(vFile); if (useFind) { - FileViewProvider view = fileToPsiFileMap.get(vFile); if (view == null) { // soft ref. collected iterator.remove(); continue; @@ -503,6 +538,9 @@ public class FileManagerImpl implements FileManager { clearPsiCaches(view); } } + else if (!evaluateValidity((AbstractFileViewProvider)view)) { + iterator.remove(); + } } myVFileToViewProviderMap.set(null); getVFileToViewProviderMap().putAll(fileToPsiFileMap); @@ -519,10 +557,8 @@ public class FileManagerImpl implements FileManager { if (!view1.getLanguages().equals(view2.getLanguages())) return false; PsiFile psi1 = view1.getPsi(baseLanguage); PsiFile psi2 = view2.getPsi(baseLanguage); - if (psi1 == null) return psi2 == null; - if (psi1.getClass() != psi2.getClass()) return false; - - return true; + if (psi1 == null || psi2 == null) return psi1 == psi2; + return psi1.getClass() == psi2.getClass(); } private void markInvalidations(@NotNull Map originalFileToPsiFileMap) { @@ -559,4 +595,67 @@ public class FileManagerImpl implements FileManager { ((AbstractFileViewProvider)viewProvider).onContentReload(); } + + /** + * Should be called only from implementations of {@link PsiFile#isValid()}, only after they've been {@link PsiFileEx#markInvalidated()}, + * and only to check if they can be made valid again. + * Synchronized by read-write action. Calls from several threads in read action for the same virtual file are allowed. + * @return if the view provider is still valid + */ + public boolean evaluateValidity(@NotNull AbstractFileViewProvider viewProvider) { + ApplicationManager.getApplication().assertReadAccessAllowed(); + + VirtualFile file = viewProvider.getVirtualFile(); + if (getRawCachedViewProvider(file) != viewProvider) { + return false; + } + + if (viewProvider.getUserData(IN_COMA) == null) { + return true; + } + + if (shouldResurrect(viewProvider, file)) { + viewProvider.putUserData(IN_COMA, null); + LOG.assertTrue(getRawCachedViewProvider(file) == viewProvider); + + for (PsiFile psiFile : viewProvider.getCachedPsiFiles()) { + // update "myPossiblyInvalidated" fields in files + // that will call us recursively again, but since we're not IN_COMA now, we'll exit earlier and avoid SOE + LOG.assertTrue(psiFile.isValid()); + } + return true; + } + + getVFileToViewProviderMap().remove(file, viewProvider); + file.replace(myPsiHardRefKey, viewProvider, null); + viewProvider.putUserData(IN_COMA, null); + + return false; + } + + private boolean shouldResurrect(FileViewProvider viewProvider, VirtualFile file) { + if (!file.isValid()) return false; + + Map tempProviders = myTempProviders.get(); + LOG.assertTrue(!tempProviders.containsKey(file), "isValid leads to endless recursion"); + tempProviders.put(file, null); + try { + FileViewProvider recreated = createFileViewProvider(file, true); + tempProviders.put(file, recreated); + return areViewProvidersEquivalent(viewProvider, recreated) && + ((AbstractFileViewProvider)viewProvider).getCachedPsiFiles().stream().noneMatch(f -> hasInvalidOriginal(f)); + } + finally { + FileViewProvider temp = tempProviders.remove(file); + if (temp != null) { + DebugUtil.performPsiModification("invalidate temp view provider", () -> ((AbstractFileViewProvider)temp).markInvalidated()); + } + } + } + + private static boolean hasInvalidOriginal(PsiFile file) { + PsiFile original = file.getOriginalFile(); + return original != file && !original.isValid(); + } + } diff --git a/platform/core-impl/src/com/intellij/psi/impl/source/PsiFileImpl.java b/platform/core-impl/src/com/intellij/psi/impl/source/PsiFileImpl.java index 546c209bc68f..d5b6213599b6 100644 --- a/platform/core-impl/src/com/intellij/psi/impl/source/PsiFileImpl.java +++ b/platform/core-impl/src/com/intellij/psi/impl/source/PsiFileImpl.java @@ -63,10 +63,9 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF private long myModificationStamp; protected PsiFile myOriginalFile; - private final FileViewProvider myViewProvider; + private final AbstractFileViewProvider myViewProvider; private volatile FileTrees myTrees = FileTrees.noStub(null, this); - private boolean myInvalidated; - @SuppressWarnings("FieldAccessedSynchronizedAndUnsynchronized") + private volatile boolean myPossiblyInvalidated; protected final PsiManagerEx myManager; public static final Key BUILDING_STUB = new Key<>("Don't use stubs mark!"); private final PsiLock myPsiLock; @@ -78,8 +77,8 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF protected PsiFileImpl(@NotNull FileViewProvider provider ) { myManager = (PsiManagerEx)provider.getManager(); - myViewProvider = provider; - myPsiLock = ((AbstractFileViewProvider) provider).getFilePsiLock(); + myViewProvider = (AbstractFileViewProvider)provider; + myPsiLock = myViewProvider.getFilePsiLock(); } public void setContentElementType(final IElementType contentElementType) { @@ -146,12 +145,22 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF // but some VFS listeners receive the same events before that and ask PsiFile.isValid return false; } - return !myInvalidated; + + if (!myPossiblyInvalidated) return true; + + // synchronized by read-write action + if (((FileManagerImpl)myManager.getFileManager()).evaluateValidity(myViewProvider)) { + myPossiblyInvalidated = false; + PsiInvalidElementAccessException.setInvalidationTrace(this, null); + return true; + } + System.out.println("fail " + getName() + Thread.currentThread()); + return false; } @Override - public void markInvalidated() { - myInvalidated = true; + public final void markInvalidated() { + myPossiblyInvalidated = true; DebugUtil.onInvalidated(this); } diff --git a/platform/lang-impl/src/com/intellij/psi/impl/file/impl/PsiVFSListener.java b/platform/lang-impl/src/com/intellij/psi/impl/file/impl/PsiVFSListener.java index 1f25cd2e2198..26f2274a0fb9 100644 --- a/platform/lang-impl/src/com/intellij/psi/impl/file/impl/PsiVFSListener.java +++ b/platform/lang-impl/src/com/intellij/psi/impl/file/impl/PsiVFSListener.java @@ -585,7 +585,7 @@ public class PsiVFSListener implements VirtualFileListener, BulkFileListener { assert depthCounter >= 0 : depthCounter; if (depthCounter > 0) return; - DebugUtil.performPsiModification(null, () -> myFileManager.invalidateAllPsi()); + DebugUtil.performPsiModification(null, () -> myFileManager.possiblyInvalidatePhysicalPsi()); PsiTreeChangeEventImpl treeEvent = new PsiTreeChangeEventImpl(myManager); treeEvent.setPropertyName(PsiTreeChangeEvent.PROP_ROOTS);