-
Notifications
You must be signed in to change notification settings - Fork 392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve order of operations when importing multi-module Maven projects. #3150
Conversation
@snjeza could you pls explain the root cause of this issue? |
There are several issues:
|
test this please |
From testing this out, I don't see a major improvement in terms of build time or memory : On quarkus-langchain4j project : We can still look at the changes here on their own based on whether they are an improvement (logically) but it's probably best if the commenters on MacOS can verify whether the change does anything for them in the issue. |
@rgrunber Could you try to restart the server after it has been successfully built? @rgrunber You can try to restart the keycloak with and without the PR too - redhat-developer/vscode-java#3639 |
@rgrunber I have updated the steps to reproduce. See steps 6 and 7 - #3150 (comment) |
@rgrunber The workspace is always rebuilt when Note: quarkus-langchain4j and keycloak have been built properly.
|
I'm pretty confident this will improve things. In particular (prior to this PR), If I attempted to reload the workspace, I would see : Basically a bunch of jobs to update individual projects. With your PR, this no longer happens and the language server never gets stuck or runs out of memory. I did notice there's some extra build jobs in the case of lombok support being enabled but at least the builds were not getting stuck. I think all that's left for me is to go through each of the changes. |
Was the use of of Lombok the trigger or how? |
@maxandersen There are several issues. Please see #3150 (comment) |
@rgrunber Please see #3150 (comment) issue 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions and comments.
...lipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/StandardProjectsManager.java
Outdated
Show resolved
Hide resolved
...lipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/StandardProjectsManager.java
Outdated
Show resolved
Hide resolved
...clipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/WorkspaceEventsHandler.java
Show resolved
Hide resolved
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/InitHandler.java
Show resolved
Hide resolved
@@ -577,7 +580,7 @@ public void didChangeConfiguration(DidChangeConfigurationParams params) { | |||
try { | |||
boolean isAutobuildEnabled = preferenceManager.getPreferences().isAutobuildEnabled(); | |||
boolean autoBuildChanged = ProjectsManager.setAutoBuilding(isAutobuildEnabled); | |||
if (jvmChanged || nullAnalysisOptionsUpdated && isAutobuildEnabled) { | |||
if ((jvmChanged || nullAnalysisOptionsUpdated) && isAutobuildEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! So if we had any old issues of builds being triggered on a jvm change / null analsysis options update despite it being off, this would fix that also ?
boolean hasMavenProjects = false; | ||
for (IProject project : ProjectUtils.getAllProjects()) { | ||
if (ProjectUtils.isMavenProject(project)) { | ||
hasMavenProjects = true; | ||
break; | ||
} | ||
} | ||
projectManager.setShouldUpdateProjects(hasMavenProjects); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the part in the else
always need to run because update(Preferences)
is guaranteed to be called from BaseInitHandler.handleInitializationOptions(..)
? That runs before the build is finished so this is definitely always triggered.
If so then I would think you don't need the shouldUpdateProjects
API, but let me know if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it. I want to delay updating the projects after the initialization of Java LS.
See
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/IProjectsManager.java
Outdated
Show resolved
Hide resolved
@rgrunber I have updated the PR. |
test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only other change I would make is to the commit message. Rename this to something like :
Improve order of operations when importing multi-module Maven projects.
- Reduce unnecessary calls to updating projects
- Reduce unnecessary calls to updating projects Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
@rgrubner I have done it. |
Fixes redhat-developer/vscode-java#3637
Steps to reproduce:
Test vsix