search

eclipse-jdtls / eclipse-jdt-core-incubator

Eclipse Public License 2.0
8 stars 1 forks source link

OutOfMemoryError when opening eclipse.jdt.ls with Javac bits #755

Open testforstephen opened 2 weeks ago

testforstephen commented 2 weeks ago 
!MESSAGE Failed to resolve binding
!STACK 0
java.lang.IllegalStateException: java.lang.OutOfMemoryError: Java heap space
    at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.analyze(JavacTaskImpl.java:383)
    at org.eclipse.jdt.core.dom.JavacBindingResolver.resolve(JavacBindingResolver.java:404)
    at org.eclipse.jdt.core.dom.JavacBindingResolver.resolvePackage(JavacBindingResolver.java:1080)
    at org.eclipse.jdt.core.dom.PackageDeclaration.resolveBinding(PackageDeclaration.java:323)
    at org.eclipse.jdt.core.dom.JavacCompilationUnitResolver.resolveBindings(JavacCompilationUnitResolver.java:408)
    at org.eclipse.jdt.core.dom.JavacCompilationUnitResolver.resolveBindings(JavacCompilationUnitResolver.java:402)
    at org.eclipse.jdt.core.dom.JavacCompilationUnitResolver.toCompilationUnit(JavacCompilationUnitResolver.java:461)
    at org.eclipse.jdt.core.dom.ASTParser.internalCreateASTCached(ASTParser.java:1263)
    at org.eclipse.jdt.core.dom.ASTParser.lambda$0(ASTParser.java:1142)
    at org.eclipse.jdt.internal.core.JavaModelManager.cacheZipFiles(JavaModelManager.java:5770)
    at org.eclipse.jdt.core.dom.ASTParser.internalCreateAST(ASTParser.java:1142)
    at org.eclipse.jdt.core.dom.ASTParser.createAST(ASTParser.java:882)
    at org.eclipse.jdt.core.manipulation.CoreASTProvider$1.run(CoreASTProvider.java:294)
    at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
    at org.eclipse.jdt.core.manipulation.CoreASTProvider.createAST(CoreASTProvider.java:286)
    at org.eclipse.jdt.core.manipulation.CoreASTProvider.getAST(CoreASTProvider.java:199)
    at org.eclipse.jdt.ls.core.internal.handlers.CodeActionHandler.getASTRoot(CodeActionHandler.java:384)
    at org.eclipse.jdt.ls.core.internal.handlers.CodeActionHandler.getCodeActionCommands(CodeActionHandler.java:156)
    at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$14(JDTLanguageServer.java:755)
    at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:87)
    at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:690)
    at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:527)
    at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:507)
    at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1458)
    at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:2034)
    at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:189)
Caused by: java.lang.OutOfMemoryError: Java heap space

The heap dump file is large, cannot share it directly. Here is the suspect report from Eclipse Memory Analyzer.

https://github.com/user-attachments/assets/72d92c7f-d335-4692-affe-8922f037ad53

mickaelistria commented 1 week ago

I've appended a couple of commits that might significantly improve performance (saving some useless processing here and there, and also improving synchronization to not have same work repeated uselessly across multiple threads). Can you please give it a new try and report whether those have led to improvements regarding this issue?

testforstephen commented 1 week ago

sure, will try and let's see if it happens again.

testforstephen commented 1 week ago

Still observe OOM when refreshing the test explorer.

!MESSAGE Failed to resolve binding
!STACK 0
java.lang.IllegalStateException: java.lang.OutOfMemoryError: Java heap space
    at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.analyze(JavacTaskImpl.java:383)
    at org.eclipse.jdt.core.dom.JavacBindingResolver.resolve(JavacBindingResolver.java:416)
    at org.eclipse.jdt.core.dom.JavacBindingResolver.resolvePackage(JavacBindingResolver.java:1105)
    at org.eclipse.jdt.core.dom.PackageDeclaration.resolveBinding(PackageDeclaration.java:323)
    at org.eclipse.jdt.core.dom.JavacCompilationUnitResolver.resolveBindings(JavacCompilationUnitResolver.java:409)
    at org.eclipse.jdt.core.dom.JavacCompilationUnitResolver.resolveBindings(JavacCompilationUnitResolver.java:403)
    at org.eclipse.jdt.core.dom.JavacCompilationUnitResolver.toCompilationUnit(JavacCompilationUnitResolver.java:461)
    at org.eclipse.jdt.core.dom.ASTParser.internalCreateASTCached(ASTParser.java:1263)
    at org.eclipse.jdt.core.dom.ASTParser.lambda$0(ASTParser.java:1142)
    at org.eclipse.jdt.internal.core.JavaModelManager.cacheZipFiles(JavaModelManager.java:5770)
    at org.eclipse.jdt.core.dom.ASTParser.internalCreateAST(ASTParser.java:1142)
    at org.eclipse.jdt.core.dom.ASTParser.createAST(ASTParser.java:882)
    at org.eclipse.jdt.core.manipulation.CoreASTProvider$1.run(CoreASTProvider.java:294)
    at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
    at org.eclipse.jdt.core.manipulation.CoreASTProvider.createAST(CoreASTProvider.java:286)
    at org.eclipse.jdt.core.manipulation.CoreASTProvider.getAST(CoreASTProvider.java:199)
    at com.microsoft.java.test.plugin.util.TestSearchUtils.parseToAst(TestSearchUtils.java:639)
    at com.microsoft.java.test.plugin.util.TestSearchUtils.findDirectTestChildrenForClass(TestSearchUtils.java:263)
    at com.microsoft.java.test.plugin.handler.TestDelegateCommandHandler.executeCommand(TestDelegateCommandHandler.java:63)
    at org.eclipse.jdt.ls.core.internal.handlers.WorkspaceExecuteCommandHandler$1.run(WorkspaceExecuteCommandHandler.java:230)
    at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
    at org.eclipse.jdt.ls.core.internal.handlers.WorkspaceExecuteCommandHandler.executeCommand(WorkspaceExecuteCommandHandler.java:220)
    at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$4(JDTLanguageServer.java:606)
    at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:87)
    at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:690)
    at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:527)
    at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:507)
    at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1458)
    at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:2034)
    at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:189)
Caused by: java.lang.OutOfMemoryError: Java heap space

I have two questions regarding the implementation:

mickaelistria commented 1 week ago

Do we support cancel when parsing AST using javac?

We do not need to cancel unless the content of the file or the project configuration has changed.

Since the client side codeActionHandler will keep triggering parsing AST when you move cursor in the editor. That results in lots of AST parsing job if the javac parser cannot respond the cancel operation.

There is no need to reparse the AST when moving the cursor. Can't it be cached so it's just reused?

When creating DOM AST from javac, the DOM AST seems retaining lots of Javac models in memory. If the AST is not released in time, those Javac models will not be released as well.

When parsing only (without JavacBindingResolver being used), we should be able to just drop the whole Java context before returning the DOM. I think it should already be happening for this case as IIRC there is no reference left from AST to Context. When keeping the binding resolver, then we need to keep the Context which is necessary in order to manipulate symbols. There might be some subparts of the context we can trim out, but I don't think this part is the priority (we should focus on retaining max 1 context per open file before trying to reduce the context size).

testforstephen commented 1 week ago

Here are 3 suspect leaks.

image

image

image

- image

image

For problem 1, the language server could respond client operation in parallel and parsing AST in parallel, so that caching lots of Javac models in memory. For problem 2, the CodeActionHandler supports lazy resolving the textEdit of proposals and will cache all proposals at the first trigger. Since the proposals have reference to the AST, and will retain the AST and its backend Javac models as well. For problem 3, they are the dependency jars.

Since we will create a new Context for each AST parse operation, that seems to rebuild the type system (Javac models) again and cause more memory than ECJ. Another project I got the OOM is https://github.com/eclipse-sirius/sirius-web, this project is a multi-module maven projects and contain many dependencies as well. Caching the Javac type models seems memory expensive for such complex project. Maybe it's worth a look into how to reuse the Javac type models for the AST parsing job in the same project.

mickaelistria commented 1 week ago

Would it help to change CodeActionHandler to synchronize getASTRoot, eg

    public static CompilationUnit getASTRoot(ICompilationUnit unit, IProgressMonitor monitor) {
                if (unit == null) {
                   return null;
                }
                synchronized (unit) {
                return CoreASTProvider.getInstance().getAST(unit, CoreASTProvider.WAIT_YES, monitor);
               }
    }

? That should prevent from loading multiple times the AST for the same unit.

rgrunber commented 1 week ago

Would it help to change CodeActionHandler to synchronize getASTRoot, eg

  public static CompilationUnit getASTRoot(ICompilationUnit unit, IProgressMonitor monitor) {
                if (unit == null) {
                   return null;
                }
                synchronized (unit) {
              return CoreASTProvider.getInstance().getAST(unit, CoreASTProvider.WAIT_YES, monitor);
               }
  }

? That should prevent from loading multiple times the AST for the same unit.

Now that I think of it, I think there is an issue I'm aware of related to this. The shared ast mechanism (CoreASTProvider) works for retrieving a shared instance of the AST. At least when I look inside the proposals list, any references to an AST (usually coming from an ASTRewriteCorrectionProposal, or CodeActionProposal) use the same "reference". The problem is that we only cache 1 single AST at a time : https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/8b9636fdb7b639f4c8dae9e1f0bcf8db22e51965/org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/core/manipulation/CoreASTProvider.java#L374 .

So with JDT-LS (and likely Eclipse), if you open a file, type a few things (reconciling triggers caching), switch to another file and only move the cursor/trigger quick-fix/assist/code actions (no editing/saving), then the source file in the active editor is re-parsed every single time to get the AST. The previous file's AST remains cached. For ECJ, parsing is a pretty fast operation, and @datho7561 mentioned he had no reason to believe it'd be slow even for javac, but if you're re-parsing the AST every time and using each copy that might cause memory issues if the references aren't properly disposed.

mickaelistria commented 3 days ago

It seems like "com.microsoft.java.test.plugin.util.TestSearchUtils.parseToAst(TestSearchUtils.java:639)" is involved in this task. It might be that object retaining more ASTs than necessary. I think you should be able to avoid creating an AST here: it's somehow already present; you can either better reuse existing AST, or use the Java IType which should contain the necessary information (methods and annotations).

testforstephen commented 2 days ago

com.microsoft.java.test.plugin.util.TestSearchUtils.parseToAst(TestSearchUtils.java:639)

It uses CoreASTProvider.getInstance().getAST(unit, CoreASTProvider.WAIT_YES, monitor) for AST. And actually CoreASTProvider.getInstance().getAST(unit, CoreASTProvider.WAIT_YES, monitor) is the recommended way to reuse AST within the current file. Since most language features need to rely on the AST of current file for code analysis and manipulation, it's suggested to use CoreASTProvider to get the AST. If the file remains unchanged, CoreASTProvider will reuse the previously cached AST.

The question is why retaining more ASTs is so expensive when comparing Javac with ECJ. Since there is an ecosystem building tools based on JDT language server, it's difficult to expect downstream projects to optimize for this issue. If we can find a way to address this at the infrastructure level, it would be far more beneficial.

mickaelistria commented 2 days ago

Would you be able to debug with Javac vs with ECJ and inspect the number of AST instances that are still referenced in both cases?

mickaelistria commented 1 day ago

I have various ideas of potential causes. However, I still didn't manage to reproduce the issue in Eclipse IDE. Do you think you could write a test case for JDT-LS that reproduces the OOM programatically?

testforstephen commented 1 day ago

A quick question: What size of project do you try to reproduce the issue?

testforstephen commented 1 day ago

And one thing I forgot to mention is that vscode-java adds -Xmx1G -Xms100m to the startup CLI, limiting the max heap to 1G by default.

mickaelistria commented 1 day ago

I have multiple modules of JDT Core, JDT LS, m2e, LSP4E, sprint-petclinic open together in my workspace. The main difference seems to be that in Eclipse IDE with JDT-UI, we don't extensively use codeminings and don't use the CodeLensHandler at all. I sometimes put a breakpoint on creation of new AST, and then ask the debugger for "instance count", but even after extensive usage; I don't see this number explode, it remains usually about 2 * number of open files, and when I close files, the "instance count" decreases.