update default vmargs of jdtls to resolve dependencies in parallel

[Eskibear]

it requires eclipse-jdtls/eclipse.jdt.ls#2563

[Eskibear]

Thank you @snjeza for reviewing the code.

Now eclipse-jdtls/eclipse.jdt.ls#2563 is merged. @rgrunber @testforstephen Let me know if there's any other concern or objection. If not, please help to merge it, thanks!

[rgrunber]

Adding the flag seems fine to me. Just 2 questions :

1. According to apache/maven-resolver@5ce7312 , it is the df collector that was given the ability to download poms in parallel. Looking at https://github.com/apache/maven-resolver/blob/master/src/site/markdown/configuration.md

The name of the dependency collector implementation to use: depth-first (original) named df, and
breadth-first (new in 1.8.0) named bf. Both collectors produce equivalent results, but they may differ
performance wise, depending on project being applied to. Our experience shows that existing df is
well suited for smaller to medium size projects, while bf may perform better on huge projects with
many dependencies. Experiment (and come back to us!) to figure out which one suits you the better.

So are we benefiting from parallel downloading of poms or the new (bf) strategy ? Or both ?

2. Would it be better to insert the flag somewhere at

   vscode-java/src/javaServerStarter.ts

   Line 126 in e7f2d0c

   if (!isSyntaxServer) {

   much like we do for shared indexing ? The idea here is to protect the flag from being touched by users who wish to configure the vmargs.

[Eskibear]

For 1).
spring-petclinic is a small sample project, and based on my previous tests, bf is much more performant than the default df impl in downloading dependencies. I can test it again and provide some details. Meanwhile, you can give it a try on your own. Using:

mvn clean package -Daether.dependencyCollector.impl=bf -Dmaven.repo.local=../m2-bf

mvn clean package -Daether.dependencyCollector.impl=df -Dmaven.repo.local=../m2-df

For 2).
I was also thinking about it. Totally agree.

[Eskibear]

The conclusion is:
In 3.9.1, both bf and df have better performance than 3.8.7. And bf is way better in all the cases I've tested.

sample project created from maven-archetype-quickstart (smallest I would say)

# maven 3.9.1 + bf
> mvn clean package -Daether.dependencyCollector.impl=bf -Dmaven.repo.local=../m2-bf
[INFO] Total time:  11.330 s

# maven 3.9.1 + df (default)
> mvn clean package -Daether.dependencyCollector.impl=df -Dmaven.repo.local=../m2-df
[INFO] Total time:  18.672 s

# 3.8.7 (default and only impl is df-like)
> mvn clean package -Dmaven.repo.local=../m2-387
[INFO] Total time:  31.795 s

spring-petclinic project (small, or medium at most)

# maven 3.9.1 + bf
> mvn clean package -Daether.dependencyCollector.impl=bf -Dmaven.repo.local=../m2-bf
[INFO] Total time:  04:32 min

# maven 3.9.1 + df
> mvn clean package -Daether.dependencyCollector.impl=df -Dmaven.repo.local=../m2-df
[INFO] Total time:  10:14 min

# 3.8.7 (default and only impl is df-like)
> mvn clean package -Dmaven.repo.local=../m2-387
[INFO] Total time:  11:05 min

For even larger projects with more dependencies, I believe bf would also be more performant.

[rgrunber]

I'm seeing the same with 3.9.1. eclipse/lemminx (wrapper update) went from ~47s (df) to ~24s (bf). For eclipse/eclipse.jdt.ls (wrapper update) it went from ~4m19s (df) to ~4m4s (bf). Definitely a lot less benefit there, but maybe something to do with the fact that it's using Tycho as well.

[Eskibear]

From our experience so far, it's reasonable to use bf by default for better performance. I just updated the PR to pin the flag as suggested.

[testforstephen]

According to the original PR download pom in parallel, the parallel capability is based on bf mode. It makes sense for me to switch to bf mode for better performance.

[fbricon]

Checked this PR with petclinic on an empty maven repo, build takes 3min with DF, 2min with BF, so definitely a win.
Merging. Thanks guys!

[mickaelistria]

@Eskibear That's really good. Do you think you could influence Eclipse m2e or even Maven in making this strategy the default? that would be 1 less thing to manage on vscode-java and a benefit for more clients at once.

[Eskibear]

you could influence Eclipse m2e or even Maven in making this strategy the default?

Yes, I think so. As long as maven-resolver makes bf as default strategy, everyone in downstream benefits, automatically.

In fact, it's already tracked in MRESOLVER-324. But AFAIK years ago maven-resolver maintainers prefer stability over performance, so I don't expect much that would happen soon.

My initial strategy was:

• we adopt bf strategy in vscode-java by default, monitoring the performance boost and confirm there's no regression or reliablity issue.
• with above as detailed proof, we might debate in MRESOLVER-324 to push it forward.

If you want m2e related clients get dogfood the performance boost first, we can create an issue in m2e to track this, and make it happen.

[rgrunber]

I would be in favour of seeing this done by default in m2e-core. If we can't make it the default there, maybe we can get some kind of API so we can set it as the default in JDT-LS (or just set the system property really early in JDT-LS startup ?).

[mickaelistria]

@Eskibear Thanks for the link to MRESOLVER-324 and for sharing your plan of action, I also found eclipse-m2e/m2e-core#1169 . I think the metrics you've reported here would weigh a lot in allowing the default to change. And even if they don't, they're extremely valuabledata to share.
I can't tell the hope for default to change in Maven, I also experienced that the maintainers are relatively conservative.
For m2e...

If you want m2e related clients get dogfood the performance boost first, we can create an issue in m2e to track this, and make it happen.

That would be great, I think m2e is more pragmatic and open to adopt more performant constructs unless they're know to be fragile (which doesn't really seem to be the case here)

[rgrunber]

I think I've found a regression here, though I suspect not a lot of people are running into it. Still worth investigating.

Make sure you have development version of vscode-java (or pre-release), and vscode-pde.

Simply import eclipse/eclipse.jdt.ls .

The first time, when certain dependencies need to be update / workspace refresh, it will work as expected. In particular you'll notice the diagnostics get published (icons on bottom left), and there's a period where the build status icon is still spinning as it finishes off the build. It will eventually complete, in ~1min for me.

Reload the window / close + re-open the child instance from the extension host, and while it seems to get past ServiceReady (the import notification disappears), the diagnostics never get published, and the build status icon is left spinning. When checking the client/server communication, there isn't much being reported, which should be the case if it were just taking longer to build. As far as I can tell it just hangs in this incomplete state.

Build status shows :

923c305a JBang Discovery [Done]
7b7afbf1 Importing Maven project(s) [Done]
b14f0b35 Refreshing workspace [Done]
8dd960fa Synchronizing projects [Done]
c868fe83 Synchronizing projects [Done]
2f0bcfb5 Synchronizing projects [Done]
6e1276af Synchronizing projects [Done]
1c70112e Background task: 7% Cleaning output folder for org.eclipse.jdt.ls.core [65/1000]

If I switch (back) to df in the client, the problem goes way, and multiple reloads finish 😐

diff --git a/src/javaServerStarter.ts b/src/javaServerStarter.ts
index 34ba23d..e86166d 100644
--- a/src/javaServerStarter.ts
+++ b/src/javaServerStarter.ts
@@ -32,7 +32,7 @@ export const HEAP_DUMP = '-XX:+HeapDumpOnOutOfMemoryError';
  * See: https://github.com/apache/maven-resolver/blob/maven-resolver-1.9.7/src/site/markdown/configuration.md
  */
 const DEPENDENCY_COLLECTOR_IMPL= '-Daether.dependencyCollector.impl=';
-const DEPENDENCY_COLLECTOR_IMPL_BF= 'bf';
+const DEPENDENCY_COLLECTOR_IMPL_BF= 'df';
 
 export function prepareExecutable(requirements: RequirementsData, workspacePath, javaConfig, context: ExtensionContext, isSyntaxServer: boolean): Executable {
        const executable: Executable = Object.create(null);

[Eskibear]

I tried a couple of times (including on a new machine), but could not reproduce it.

1c70112e Background task: 7% Cleaning output folder for org.eclipse.jdt.ls.core [65/1000]

It seems to be unrelated at a glance. I do observe that for an arbitrary maven project occasionally it gets stuck on a background task and never finishs, but that's usually back to normal after a reloading. Not sure if that's related.

@rgrunber What if you switch back to bf, does the issue come back again?

@testforstephen @jdneo @CsCherrYY Are you observing the same issue given you are working on jdtls project everyday?

[testforstephen]

1c70112e Background task: 7% Cleaning output folder for org.eclipse.jdt.ls.core [65/1000]

Recently I encountered this issue once, and reload window fixed it for me.