From 9919d640b8e229f7e02050844a2943fdffc06b05 Mon Sep 17 00:00:00 2001 From: peter Date: Thu, 14 Sep 2017 10:04:22 +0200 Subject: [PATCH] cache PsiFileImpl#hasUnbindableCachedPsi (GO-4344) --- .../java/psi/StubAstSwitchTest.groovy | 32 ++++++++- .../extapi/psi/StubBasedPsiElementBase.java | 4 +- .../psi/impl/source/AstPathPsiMap.java | 68 ++++++++++++------- .../intellij/psi/impl/source/PsiFileImpl.java | 39 +++++------ 4 files changed, 94 insertions(+), 49 deletions(-) diff --git a/java/java-tests/testSrc/com/intellij/java/psi/StubAstSwitchTest.groovy b/java/java-tests/testSrc/com/intellij/java/psi/StubAstSwitchTest.groovy index 5362c9ee3063..0c7d2fdc5519 100644 --- a/java/java-tests/testSrc/com/intellij/java/psi/StubAstSwitchTest.groovy +++ b/java/java-tests/testSrc/com/intellij/java/psi/StubAstSwitchTest.groovy @@ -34,14 +34,15 @@ import com.intellij.psi.stubs.StubTree import com.intellij.psi.util.PsiTreeUtil import com.intellij.reference.SoftReference import com.intellij.testFramework.LeakHunter +import com.intellij.testFramework.PlatformTestUtil import com.intellij.testFramework.SkipSlowTestLocally import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase import com.intellij.util.ref.GCUtil +import groovy.transform.CompileStatic import java.util.concurrent.Callable import java.util.concurrent.CountDownLatch import java.util.concurrent.Future - /** * @author peter */ @@ -68,7 +69,7 @@ class StubAstSwitchTest extends LightCodeInsightFixtureTestCase { void "test reachable psi classes remain valid when nothing changes"() { int count = 1000 - List> classList = (0..> classList = (0..(myFixture.addClass("class Foo$it {}")) } System.gc() System.gc() System.gc() @@ -333,4 +334,31 @@ class B { assert stubTree.plainList.find { it.stubType == JavaStubElementTypes.ANONYMOUS_CLASS } } + + @CompileStatic + void "test getStub performance with cached PSI"() { + def text = "class Foo { " + "void bar(int a, int b, int c, int d, int e) { int x = null; }\n" * 1000 + "}" + def file = myFixture.addFileToProject "a.java", text + + PsiMethod[] methods = ((PsiJavaFile) file).classes[0].methods + def params = methods.collect { PsiMethod method -> method.parameterList.parameters } + def literal = file.findElementAt(text.indexOf('null')).parent as PsiLiteralExpression // the only cached PSI without stubIndex + + GCUtil.tryGcSoftlyReachableObjects() + + def fileImpl = (PsiFileImpl)file + assert !fileImpl.treeElement + assert !fileImpl.stub + + PlatformTestUtil.startPerformanceTest('getStub performance', 100, { + 10_000.times { + if (fileImpl.stub != null) { + throw new IllegalStateException("has stub") + } + } + }).assertTiming() + + assert params + assert literal + } } diff --git a/platform/core-impl/src/com/intellij/extapi/psi/StubBasedPsiElementBase.java b/platform/core-impl/src/com/intellij/extapi/psi/StubBasedPsiElementBase.java index 11339ecae671..eb91ea950877 100644 --- a/platform/core-impl/src/com/intellij/extapi/psi/StubBasedPsiElementBase.java +++ b/platform/core-impl/src/com/intellij/extapi/psi/StubBasedPsiElementBase.java @@ -230,7 +230,7 @@ public class StubBasedPsiElementBase extends ASTDelegateP * Don't invoke this method, it's public for implementation reasons. */ public final void setNode(@NotNull ASTNode node) { - mySubstrateRef = SubstrateRef.createAstStrongRef(node); + setSubstrateRef(SubstrateRef.createAstStrongRef(node)); } /** @@ -533,7 +533,7 @@ public class StubBasedPsiElementBase extends ASTDelegateP @Override protected Object clone() { final StubBasedPsiElementBase copy = (StubBasedPsiElementBase)super.clone(); - copy.mySubstrateRef = SubstrateRef.createAstStrongRef(getNode()); + copy.setSubstrateRef(SubstrateRef.createAstStrongRef(getNode())); return copy; } } diff --git a/platform/core-impl/src/com/intellij/psi/impl/source/AstPathPsiMap.java b/platform/core-impl/src/com/intellij/psi/impl/source/AstPathPsiMap.java index 60ffc8663a94..87e300b5a03b 100644 --- a/platform/core-impl/src/com/intellij/psi/impl/source/AstPathPsiMap.java +++ b/platform/core-impl/src/com/intellij/psi/impl/source/AstPathPsiMap.java @@ -28,14 +28,19 @@ import com.intellij.util.containers.ContainerUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.lang.ref.Reference; import java.lang.ref.ReferenceQueue; import java.lang.ref.WeakReference; -import java.util.Collections; -import java.util.List; +import java.util.Objects; import java.util.concurrent.ConcurrentMap; +import java.util.stream.Stream; /** * A weak cache for all instantiated stub-based PSI to allow {@link CompositeElement#getPsi()} return it when AST is reloaded.

+ * + * All methods should be called under an external lock (view provider's PsiLock), except for + * ({@link #getCachedPsi(AstPath)} which can be called without lock. + * * @author peter */ class AstPathPsiMap { @@ -45,6 +50,7 @@ class AstPathPsiMap { * Otherwise the files end up retaining lots of maps with all-gc-ed stuff inside, but the maps are still very large. */ private final ConcurrentMap myMap = ContainerUtil.newConcurrentMap(); + private volatile Boolean myHasUnbindableCachedPsi = null; private static final Key STUB_PSI_REFS = Key.create("STUB_PSI_REFS"); private final MyReferenceQueue myQueue; @@ -56,27 +62,23 @@ class AstPathPsiMap { void invalidatePsi() { myQueue.cleanupStaleReferences(); - for (MyReference reference : myMap.values()) { - StubBasedPsiElementBase psi = SoftReference.dereference(reference); - if (psi != null) { - DebugUtil.onInvalidated(psi); - psi.setSubstrateRef(SubstrateRef.createInvalidRef(psi)); - } - } + getAllCachedPsi().forEach(psi -> { + DebugUtil.onInvalidated(psi); + psi.setSubstrateRef(SubstrateRef.createInvalidRef(psi)); + }); myMap.clear(); + myHasUnbindableCachedPsi = false; } void switchToStrongRefs() { myQueue.cleanupStaleReferences(); - for (MyReference reference : myMap.values()) { - StubBasedPsiElementBase psi = SoftReference.dereference(reference); - if (psi != null) { - CompositeElement node = (CompositeElement)psi.getNode(); - node.setPsi(psi); - psi.setSubstrateRef(SubstrateRef.createAstStrongRef(node)); - } - } + getAllCachedPsi().forEach(psi -> { + CompositeElement node = (CompositeElement)psi.getNode(); + node.setPsi(psi); + psi.setSubstrateRef(SubstrateRef.createAstStrongRef(node)); + }); myMap.clear(); + myHasUnbindableCachedPsi = false; } @Nullable @@ -91,18 +93,34 @@ class AstPathPsiMap { // otherwise another thread could invoke StubRef.getNode and fail since file's AST isn't set yet psi.setSubstrateRef(key); myMap.put(key, new MyReference(psi, key, myQueue)); + clearStubIndexCache(); return psi; } - List> getAllCachedPsi() { + Stream> getAllCachedPsi() { myQueue.cleanupStaleReferences(); - if (myMap.isEmpty()) return Collections.emptyList(); + if (myMap.isEmpty()) return Stream.empty(); - List> result = ContainerUtil.newArrayList(); - for (MyReference reference : myMap.values()) { - ContainerUtil.addIfNotNull(result, reference.get()); + return myMap.values().stream().map(Reference::get).filter(Objects::nonNull); + } + + boolean hasUnbindableCachedPsi() { + Boolean answer = myHasUnbindableCachedPsi; + if (answer == null) { + myHasUnbindableCachedPsi = answer = calcHasUnbindableCachedPsi(); } - return result; + return answer; + } + + private boolean calcHasUnbindableCachedPsi() { + myQueue.cleanupStaleReferences(); + if (myMap.isEmpty()) return false; + + return getAllCachedPsi().anyMatch(e -> e.getStubIndex() < 0); + } + + void clearStubIndexCache() { + myHasUnbindableCachedPsi = null; } private static class MyReference extends WeakReference> { @@ -125,7 +143,9 @@ class AstPathPsiMap { if (reference == null) break; AstPath key = reference.pathRef; - key.getContainingFile().getRefToPsi().myMap.remove(key, reference); + AstPathPsiMap refToPsi = key.getContainingFile().getRefToPsi(); + refToPsi.myMap.remove(key, reference); + refToPsi.clearStubIndexCache(); } } 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 9ae2f3ebda16..14817553e1e8 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 @@ -269,6 +269,7 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF myRefToPsi.cachePsi(path, psi); psi.setStubIndex(i + 1); } + myRefToPsi.clearStubIndexCache(); } private List> calcStubAstBindings(@NotNull FileElement root, FileTrees trees) { @@ -602,15 +603,17 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF DebugUtil.startPsiModification("onContentReload"); try { - myRefToPsi.invalidatePsi(); + synchronized (myPsiLock) { + myRefToPsi.invalidatePsi(); - FileElement treeElement = derefTreeElement(); - if (treeElement != null) { - treeElement.detachFromFile(); - DebugUtil.onInvalidated(treeElement); + FileElement treeElement = derefTreeElement(); + if (treeElement != null) { + treeElement.detachFromFile(); + DebugUtil.onInvalidated(treeElement); + } + updateTrees(myTrees.clearStub("onContentReload")); + setTreeElementPointer(null); } - updateTrees(myTrees.clearStub("onContentReload")); - setTreeElementPointer(null); } finally { DebugUtil.finishPsiModification(); @@ -712,6 +715,7 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF // Stub index might call getStubTree on main PSI file, but then use getPlainListFromAllRoots and return stubs from another file. // Even if that file already has AST, stub.getPsi() should be the same as in AST TreeUtil.bindStubsToTree(stubTree, fileElement); + eachPsiRoot.myRefToPsi.clearStubIndexCache(); } else { eachPsiRoot.bindStubsToCachedPsi(stubTree); bindings.put(eachPsiRoot, stubTree); @@ -732,16 +736,10 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF } private boolean hasUnbindableCachedPsi() { - for (PsiFile file : myViewProvider.getAllFiles()) { - if (file instanceof PsiFileImpl) { - for (StubBasedPsiElementBase psi : ((PsiFileImpl)file).myRefToPsi.getAllCachedPsi()) { - if (psi.getStubIndex() < 0) { - return true; - } - } - } + synchronized (myPsiLock) { + return ContainerUtil.exists(myViewProvider.getAllFiles(), + file -> file instanceof PsiFileImpl && ((PsiFileImpl)file).myRefToPsi.hasUnbindableCachedPsi()); } - return false; } @Nullable @@ -761,13 +759,13 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF } private void bindStubsToCachedPsi(StubTree stubTree) { - for (StubBasedPsiElementBase psi : myRefToPsi.getAllCachedPsi()) { + myRefToPsi.getAllCachedPsi().forEach(psi -> { int index = psi.getStubIndex(); if (index >= 0) { //noinspection unchecked ((StubBase)stubTree.getPlainList().get(index)).setPsi(psi); } - } + }); } protected PsiFileImpl cloneImpl(FileElement treeElementClone) { @@ -1029,8 +1027,6 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF return this == another; } - private final Object myStubFromTreeLock = new Object(); - /** * @return a stub tree object having {@link #getGreenStub()} as a root, or null if there's no green stub available */ @@ -1048,7 +1044,7 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF } assert myFileElementBeingLoaded.get() == null : "non-empty thread-local"; FileElement fileElement = calcTreeElement(); - synchronized (myStubFromTreeLock) { + synchronized (myPsiLock) { tree = derefStub(); if (tree == null) { @@ -1074,6 +1070,7 @@ public abstract class PsiFileImpl extends ElementBase implements PsiFileEx, PsiF tree.setDebugInfo("created in calcStubTree"); try { TreeUtil.bindStubsToTree(tree, fileElement); + myRefToPsi.clearStubIndexCache(); } catch (TreeUtil.StubBindingException e) { rebuildStub();