Remove usages of getPsi() #2901
Conversation
|
Thanks for your contributions. So far, I have just skimmed your changes. I will do a detailed review later. To not block you, I have approved the workflow so that the pipeline will be trigger when you add changes.
What lead you to analyzing/resolving this problem? Are you resolving problems with Ktlint CLI, ktlint-intellij-plugin, or with any API Consumer? Of course, all will benefit from improved performance. But I am just curious to understand where you are coming from. |
|
I am running ktlint primarily as an API consumer inside of a gradle plugin. I build a large project with hundreds of modules and execute ktlint concurrently on thousands of kotlin files and I have observed frequently my gradle builds seem to pause for long times (1-30 seconds) on ktlint execution. These pauses seem unnaturally long. I don't see pauses like this during regular kotlin compilation or during detekt. So I thought something different might be happening in ktlint. Upon CPU profiling, the flame graph looked like this: 
What this told me is that it wasn't one rule, but rather something dispersed throughout ktlint. Then I took a closer look, and that is when I saw the image I shared at the top. The
This result tells a compelling story: 
|
|
My strategy so far has been:
My biggest concern is that even though I ensure tests pass, some of my changes are likely to cause unforseable bugs. I feel bad introducing these bugs, but I feel its neccesary to migrate the code to more lightweight APIs. I hope you agree. Also, I still have a lot to learn about these APIs and could be making mistakes. One idea I have is that if this gets merged, the first release after this merge is a beta release allowing a time for people to report bugs, since the tests probably aren't covering every corner case. I don't think these changes can be released as a stable version right away. I am looking forward to your feedback. |
|
The other main concern that I have is that the ASTNode API doesn't do as much logic for you, so its harder to maintain and develop. It seems with a bit of dedication we could build an ASTNode-based library of extension functions to mimic a lot of the logic of psi, but I have mixed feelings about that. |
|
There are 13 more Rule classes that have usages of This could technically be merged as is. The remaining work is just to remove more usages of |
|
Also I am curious, would ktlint possibly migrate to the analysis API in the future? I don't know much about this, but I wonder if newer APIs like that resolve the problem, like maybe they are both lightweight and also could offer more support with the logic like psi does? Just a thought. |
|
Looking into it more, apparently the Analysis API is overkill for a formatter and has lots of overhead. This leads me to think maybe creating a small lightweight ASTNode-based support library, largely just copying a lot of the logic from Psi classes, will be helpful. |
|
Basically expanding |
mgroth0
changed the title
Interesting to know this. Personally I use ktlint CLI only on small projects, so I have never noticed this.
Ktlint uses both the ASTNode interface as well as Psi. I expect that it dependended on the knowdledge/experience of rule developers which approach was used. In most cases I prefer the ASTNode, as it is way more easy to comprehend. On the other hand Psi ofter gives access to helper functions, but I find them typically hard to find. But I have never (until this issue) seen a reason to migrate away from PSI fully.
I guess that the cancellation checking especially makes sense when running within the context of IntelliJ IDEA. For ktlint (including the ktlint-intellij-plugin) this does not seem relevant though.
Sounds reasonable.
I have no problem with this. I am quite confident about the test converage of most rules. Those tests cover both linting and formatting. But I hope that I can count on your support in case such errors will occur.
After each merge, a SNAPSHOT version of ktlint is released. The ktlint build pipeline does not support BETA builds as far as I know. Also, that would not make a lot of sense to me, as users of ktlint do not seem to be interested/invested in testing new versions before the actual release. That is the main reason that after major/minor release usually one patch version is needed a couple of weeks after the major/minor release.
This is indeed a concern. But there is no need to be stressed about it too much. We don't need to replace 100% of Psi with AST. First we can pick the low hanging fruit. By doing so, we learn more about what works or doesn't work. Those learnings will help with future decision making.
Agree. Let's first work on wrapping/merging this part.
I see no reason to migrate to the analysis API. Expanding ASTNodeExtension is fine. Let's see how this works out. Thanks for your efforts sofar. It is nice to have input from a totally different perspective. I will start detailed review. |
I cannot make a strong commitment to make fixes in a timely manner due to my other priorities, but please feel free to flag me on any bug report that involves my changes and I will help when I can. |
There was a problem hiding this comment.
This is really a promising initative!
Please don't be dishartened by the amount of review remarks. Most of them are relatively minor because you are not fully acquainted with the code base.
I do have some concerns about replacing all Psi code with own AstNode although the readability is improved on lots of places. Sofar, I have seen no changes that should not have been ported from Psi.
| @@ -6,6 +6,7 @@ public final class com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtensionKt | |||
| public static final fun findCompositeParentElementOfType (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode; | |||
| public static final fun firstChildLeafOrSelf (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode; | |||
| public static final fun getColumn (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)I | |||
| public static final fun getPrevSiblingIgnoringWhitespaceAndComments (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode; | |||
There was a problem hiding this comment.
Whenever a function is added to the public API, we get a responsibility to maintain it at least until the next major release. As of that we should be careful with:
- Naming of the function
- Avoiding duplication
For each public method that is added to the ASTNodeExtension the following is required:
- API documentation
- Unit tests in
ASTNodeExtensionTest. Typically the methods need multiple units tests, which should be collected in an inner class.
| @@ -183,11 +184,17 @@ public fun ASTNode.parent( | |||
| return null | |||
| } | |||
|
|
|||
| public fun ASTNode.isPartOf(tokenSet: TokenSet): Boolean = parent(predicate = { tokenSet.contains(it.elementType) }, strict = false) != null | |||
There was a problem hiding this comment.
I prefer trailing lambda style when possible:
public fun ASTNode.isPartOf(tokenSet: TokenSet): Boolean = parent(strict = false) { tokenSet.contains(it.elementType) } != null
| @Deprecated( | ||
| "psi is a performance issue, see https://github.com/pinterest/ktlint/pull/2901", | ||
| replaceWith = ReplaceWith("ASTNode.isPartOf(elementType: IElementType) or ASTNode.isPartOf(tokenSet: TokenSet)"), | ||
| ) |
There was a problem hiding this comment.
Replace with:
@Deprecated(
"Marked for removal in Ktlint 2.x. Replace with ASTNode.isPartOf(elementType: IElementType) or ASTNode.isPartOf(tokenSet: TokenSet). " +
"This method might cause performance issues, see https://github.com/pinterest/ktlint/pull/2901",
replaceWith = ReplaceWith("this.isPartOf(elementTypeOrTokenSet)"),
)
Note the replaceWith. IDEA interprets the replaceWith and provides an action that does the suggested refactoring. Not only is this helpful for refactoring rules in Ktlint, but also for external rule providers.
or
and after clicking
| @@ -223,10 +230,21 @@ public fun ASTNode.isLeaf(): Boolean = firstChildNode == null | |||
| */ | |||
| public fun ASTNode.isCodeLeaf(): Boolean = isLeaf() && !isWhiteSpace() && !isPartOfComment() | |||
|
|
|||
| public fun ASTNode.isPartOfComment(): Boolean = parent(strict = false) { it.psi is PsiComment } != null | |||
| public fun ASTNode.isPartOfComment(): Boolean = isPartOf(TokenSets.COMMENTS) | |||
There was a problem hiding this comment.
Nice, this improve readability.
| children().forEach { | ||
| yield(it) | ||
| yieldAll(it.recursiveChildren()) | ||
| } |
There was a problem hiding this comment.
Replace with:
children().forEach { yieldAll(it.recursiveChildren(includeSelf = true)) }
| ?.getPrevSiblingIgnoringWhitespaceAndComments(withItself = false) | ||
| ?.takeIf { it is KtDeclaration } | ||
| ?.getPrevSiblingIgnoringWhitespaceAndComments() | ||
| ?.takeIf { KtTokenSets.DECLARATION_TYPES.contains(it.elementType) } |
There was a problem hiding this comment.
- Replace
containswithin - KtTokensSets is still declared in Psi (Java) library. It might be better to copy to our
TokensSets.kt.
| val addNewLine = | ||
| leafBeforeArrowOrNull | ||
| ?.let { !(leafBeforeArrowOrNull is PsiWhiteSpace && leafBeforeArrowOrNull.textContains('\n')) } | ||
| ?.let { | ||
| !( | ||
| leafBeforeArrowOrNull.elementType == ElementType.WHITE_SPACE && | ||
| leafBeforeArrowOrNull.textContains('\n') | ||
| ) | ||
| } | ||
| ?: false |
There was a problem hiding this comment.
This can be simplified to:
val addNewLine = !(leafBeforeArrowOrNull?.isWhiteSpaceWithNewline() ?: true)
but it requires another change (see next review comment) to resolve compilation errors.
| if (leafBeforeArrowOrNull.elementType == ElementType.WHITE_SPACE) { | ||
| (leafBeforeArrowOrNull.psi as LeafPsiElement).rawReplaceWithText(indent) |
There was a problem hiding this comment.
Replace with:
if (leafBeforeArrowOrNull.isWhiteSpace()) {
(leafBeforeArrowOrNull?.psi as LeafPsiElement).rawReplaceWithText(indent)
| @@ -341,7 +343,7 @@ public class TrailingCommaOnDeclarationSiteRule : | |||
|
|
|||
| if (inspectNode.treeParent.elementType == ElementType.ENUM_ENTRY) { | |||
| val parentIndent = | |||
| (prevNode.psi.parent.prevLeaf() as? PsiWhiteSpace)?.text | |||
| (prevNode.treeParent.prevLeaf()?.takeIf { it.elementType == ElementType.WHITE_SPACE })?.text | |||
There was a problem hiding this comment.
Replace with:
(prevNode.treeParent.prevLeaf()?.takeIf { it.isWhiteSpace() })?.text
| (psi as KtFunctionLiteral) | ||
| .arrow | ||
| ?.prevLeaf() | ||
| when (elementType) { |
There was a problem hiding this comment.
Replace when statement with:
takeIf { it.elementType == WHEN_ENTRY || it.elementType == FUNCTION_LITERAL }
?.findChildByType(ARROW)
?.prevLeaf()
Description
I observed a performance bottleneck in
com.pinterest.ktlint.rule.engine.core.api.isPartOfComment. CPU profiling showed a significant amount of time being wasted doing stuff related to checking for cancellation, like IntelliJ GUI stuff.Checklist
Before submitting the PR, please check following (checks which are not relevant may be ignored):
Closes #<xxx>orFixes #<xxx>(replace<xxx>with issue number)Documentation is updated. See difference between snapshot and release documentation
This PR doesn't include new tests, doesn't reference an issue, and doesn't change documentaiton. It is a performance optimization.
According to IntelliJ's Usages, there are about 65 other usages of
ASTNode.getPsi()in the ktlint codebase. I'm not sure if all of them could be removed, but maybe we could try to remove as many of them as possible? If this PR looks good, I suggest we follow up by doing that.