[MRESOLVER-7] download poms in parallel #178

caiwei-ebay · 2022-05-16T11:35:42Z

This PR is for parallel POM downloading, the idea is a bit different than https://github.com/ibabiankou/maven-resolver/pull/2/files:

only parallelize the VersionRange and ArtifactDesciptor resolution part (poms.xml & maven-metadata.xml downloading in parallel) as the poms downloading or maven-metadata.xml is the most slow part.
Pure dependency resolution logic and the skip logic are still run in sequence.

caiwei-ebay · 2022-05-16T11:56:50Z

@michael-o @cstamas @ibabiankou

I think the code is already ready for review, I would like to run more tests before converting the PR from Draft to final.
Please help review and share your comments with this PR.

michael-o

Please move the PremanagedDepenency change into a separate PR. This will make it eaiser to review

caiwei-ebay · 2022-05-17T00:06:46Z

@michael-o

Created separate PR for moving the PremanagedDependency class.
#179

caiwei-ebay · 2022-05-17T09:34:42Z

Separated the PR for PremanagedDependency refactoring and rebased the code.

michael-o · 2022-05-17T17:20:52Z

Much simpler to read now...

cstamas · 2022-06-06T17:24:10Z

Something is wrong here, unsure what. But, I tried to build maven master using maven-3.9.x + this PR + empty local repo and it fails:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:testCompile (default-testCompile) on project maven-model-builder: Compilation failure: Compilation failure: 
[ERROR] /home/cstamas/Worx/apache-maven/maven/maven-model-builder/src/test/java/org/apache/maven/model/interpolation/StringSearchModelInterpolatorTest.java:[36,27] package org.hamcrest does not exist
[ERROR] /home/cstamas/Worx/apache-maven/maven/maven-model-builder/src/test/java/org/apache/maven/model/interpolation/StringSearchModelInterpolatorTest.java:[36,1] static import only from classes and interfaces
[ERROR] /home/cstamas/Worx/apache-maven/maven/maven-model-builder/src/test/java/org/apache/maven/model/interpolation/StringSearchModelInterpolatorTest.java:[37,27] package org.hamcrest does not exist
[ERROR] /home/cstamas/Worx/apache-maven/maven/maven-model-builder/src/test/java/org/apache/maven/model/interpolation/StringSearchModelInterpolatorTest.java:[37,1] static import only from classes and interfaces
[ERROR] -> [Help 1]

The cmd used to test:

[cstamas@infinity maven (maven-3.9.x)]$ ~/bin/maven/apache-maven-3.9.0-SNAPSHOT/bin/mvn clean install -Dmaven.repo.local=local-repo -Drat.skip -DskipTests -Daether.collector.impl=bf

caiwei-ebay · 2022-06-09T06:25:19Z

@cstamas
I can reproduce. Let me do some research.

caiwei-ebay · 2022-06-23T11:43:35Z

@michael-o @cstamas

I have converted the PR from draft to ready. Please help review.

The basic idea of this PR is:

Given dependency A's children B, C, D
Iterate B, C, D one by one, take B as example below.
Should B be filtered? If yes, won't add to the queue. Logic here
If B should not be filtered, then figure out the managed version of B, ex B1, resolve B1's descriptor in a thread, then add B1 to the queue. Logic here

In such way, poms and metadata.xml will be downloaded in parallel.

The issue @cstamas raised has been fixed. The failure was caused incorrect filtering (should filter with original dependency, B instead of B1 as described in above flow)

Also runs the ITs with both DF and BF (code based on this PR) mode, same test result below:
2 tests that are not related with this PR failed in both modes. Not sure if this is as expected.

[ERROR] Failures: [ERROR] MavenITmng5576CdFriendlyVersions>AbstractMavenIntegrationTestCase.runTest:260->testContinuousDeliveryFriendlyVersionsAreWarningFreeWithBuildConsumer:98 [WARNING] [ERROR] MavenITmng5576CdFriendlyVersions>AbstractMavenIntegrationTestCase.runTest:260->testContinuousDeliveryFriendlyVersionsAreWarningFreeWithoutBuildConsumer:67 [WARNING] [INFO] [ERROR] Tests run: 912, Failures: 2, Errors: 0, Skipped: 0

Also dry-run 1000+ apps in our company by comparing the maven dependency tree with DF and BF mode (code based on this PR), no issues found so far. Test method like this:

mvn clean install -DskipTests -Daether.collector.impl=bf -Dmaven.repo.local=bf
mvn dependency:tree -DoutputFile=bf-tree.txt -Daether.collector.impl=bf -Dmaven.repo.local=bf
mvn clean install -DskipTests -Dmaven.repo.local=df
mvn dependency:tree -DoutputFile=df-tree.txt -Dmaven.repo.local=df
compare bf-tree.txt df-tree.txt

Please help review and feel free to let me know if anything I can help.

caiwei-ebay · 2022-07-06T13:53:38Z

@michael-o @cstamas
Folks, please help review the PR and let me know if anything needs from me.
This PR can make poms/metadata.xml download much faster.

cstamas · 2022-07-06T14:20:17Z

Will do around weekend, busy few days till then.

michael-o · 2022-07-06T18:33:40Z

Will likely happen either this Friday or in 10 days. I think this should not be in Maven 3.9.0 just to avoid any breakings.

...er-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java

michael-o · 2022-07-23T18:37:23Z

...er-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java

@@ -365,6 +477,64 @@ else if ( descriptorResult == DataPool.NO_DESCRIPTOR )
        return descriptorResult;
    }

+    static class ParallelDescriptorResolver
+    {
+        final ExecutorService executorService;


Can this resolver leverage the same executor service as for artifacts with aether.connector.basic.threads? At the end it does not matter what the connector transports, no?

See: org.eclipse.aether.connector.basic.BasicRepositoryConnector

@michael-o
Thanks for the advice. Agreed we should reuse the executor service.

The BfDependencyCollector calls DefaultArtifactsResolver and the latter then calls (Basic)RepositoryConnector.

Do you mean we should make BfDependencyCollector calls RepositoryConnector's get(artifactDownloads, metadataDownloads) to download poms? or simply make the getExecutor method as public and then make BfDependencyCollector call the getExcutor to reuse the executor service. Please help clarify.

I mean the former, fully utilize get(List, List) with all the GetTask, PeekTask. Downloading POMs in parallel shouldn't be any different than before sequentially.

Sorry for the late reply. Busy working these days.

Uploaded a diagram to explain how the dependency resolution works.

In a Maven build:

DependencyCollector focus on resolve dependency graph by downloading and parsing metadata.xml and pom.xmls which is running in sequence as of now.

MojoExecutor calls ensureDependencyResolved which download jars in parallel based on the dependency graph calculated by DependencyCollector.

Using the former (fully unitize get <List, List>) in DependencyCollector seems not possible, I think we need to use a separate thread pool for dependency resolution in DependencyCollector which includes 2 steps:

Resolve version range (which resort to VersionRangeResolver -> MetadataResolver -> RepositoryConnector)

Resolve descriptor (download or pick local poms and then parse poms, which resort to ArtifactDescriptorReader -> ArtifactResolver -> RepositoryConnector)

By running the above 2 steps in a separate thread pool can achieve better parallelism.
Like the MetadataResolver, it is also using a separate ThreadPool to download metadata.xml one by one from multiple repositories.

May I know any concerns that we should not use another thread pool for dependency resolution here?

@michael-o @cstamas Please share your thoughts about using a separate pool here. Or please give some design suggestions on how to make the code to meet the merge criteria.

the executor exists in connector, this is impl module, connector does not depend on impl nor impl depend on connector, see https://maven.apache.org/resolver/index.html

Now, not only that, but also it is quite possible that execution does not "even get to" connector (ie. all is present locally), so this would require major undertake, probably to move executor providing component to SPI (as util is not quite the place for this), then change both uses (so BF collector and connector), but in connector module we'd need to provide some "stub" implementation (as again, it does not depend on impl), and finally that would become one whole. But even writing this down is long, and seems totally unrelated to this PR.

Am currently totally not concerned about sharing executor, but more that executor is created per invocation of collect merhd...

For a multiple module project, collect method will be called when building each module, and when run in parallel, collect method will be called by multiple threads. If we consider parallel build case, creates multiple executors seems more reasonable.

@cstamas Any suggestions about whether we should use separate executors for the Maven parallel build?

For now I'd follow very same pattern as in connector, it creates executor for each remote repos (not good, but not bad either). So creating executor per resolve invocation is also ok. Later, we could do something around this, but it requires larger scale changes, so leave it for now IMHO

Totally agreed. Could refactor the executor part in a separate PR so all of the download related logic could share.
Another issue you've pointed out is fixed. Thanks for the review. Please let me know if anything else I can help.

Eskibear · 2022-08-09T02:49:39Z

Any updates on this PR? Can we expect it to be merged soon?

I'm a contributor of JDT Language Server, and one of our core component m2e is depending on maven-resolver. We've been observing that downloading poms one by one takes a lot of time when importing Maven projects. To be honest, it's one of our performance bottleneck and we can hardly improve it on our own, in downstream... So it would be great if this improvement can be shipped in coming releases.

caiwei-ebay · 2022-08-09T08:28:31Z

Any updates on this PR? Can we expect it to be merged soon?

I'm a contributor of JDT Language Server, and one of our core component m2e is depending on maven-resolver. We've been observing that downloading poms one by one takes a lot of time when importing Maven projects. To be honest, it's one of our performance bottleneck and we can hardly improve it on our own, in downstream... So it would be great if this improvement can be shipped in coming releases.

Friends, the PR is under review. Feel free to participate into the contribution. :)

michael-o · 2022-08-09T08:30:08Z

Any updates on this PR? Can we expect it to be merged soon?

I'm a contributor of JDT Language Server, and one of our core component m2e is depending on maven-resolver. We've been observing that downloading poms one by one takes a lot of time when importing Maven projects. To be honest, it's one of our performance bottleneck and we can hardly improve it on our own, in downstream... So it would be great if this improvement can be shipped in coming releases.

Proper evaluation I'd mandatory, we already had severe regressions due to the previous approach.

Eskibear · 2022-09-05T05:55:28Z

Proper evaluation I'd mandatory

I understand and appreciate all the work you guys have done. If I remember clearly, some integration tests and details were discussed in a private ASF slack channel which I had no access...

Just want to know if there's any ETA (already one month since last update)? Anyway, please let me know if there's anything I can help (if that would help to proceed this).

michael-o · 2022-09-05T10:18:57Z

Proper evaluation I'd mandatory

I understand and appreciate all the work you guys have done. If I remember clearly, some integration tests and details were discussed in a private ASF slack channel which I had no access...

Just want to know if there's any ETA (already one month since last update)? Anyway, please let me know if there's anything I can help (if that would help to proceed this).

@cstamas we need to discuss this sometime this month.

caiwei-ebay · 2022-09-28T06:44:43Z

Proper evaluation I'd mandatory

I understand and appreciate all the work you guys have done. If I remember clearly, some integration tests and details were discussed in a private ASF slack channel which I had no access...
Just want to know if there's any ETA (already one month since last update)? Anyway, please let me know if there's anything I can help (if that would help to proceed this).

@cstamas we need to discuss this sometime this month.

@michael-o @cstamas

May I know the latest status of discussion? Please never hesitate to let me know if anything I can help or the code I should revise.

cstamas · 2022-09-28T12:33:35Z

@caiwei-ebay will get to it soon, sorry for delay....

michael-o · 2022-09-28T12:37:55Z

For me, this won't happen before October.

...er-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java

caiwei-ebay2 · 2022-10-13T14:41:55Z

@michael-o
How's everything? Got a chance to give more comments to this PR? And may I know what's the remaining steps to get this PR merged?

cstamas · 2022-10-19T10:15:54Z

@michael-o can you please specify what changes are requested with your "change request" PR review? It is unclear to me. As for me, this PR is good to merge (and probably improve it later re shared executor, but is def out of scope for this PR)

cstamas · 2022-10-26T06:32:56Z

...er-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java

+
+        private ExecutorService getExecutorService( RepositorySystemSession session )
+        {
+            int nThreads = ConfigUtils.getInteger( session, 5, "maven.artifact.threads" );


One more thing, but I will do this post merge: if you look at BasicRepositoryConnector, it uses CONFIG_PROP_THREADS and just as "secondary" property uses this "maven.artifact.threads" as a fallback. I will update this post merge, to make them behave siimilarly; have it's own prop but fall back to "maven.artifact.threas" as well

done in 7f56672

caiwei-ebay marked this pull request as draft May 16, 2022 11:36

michael-o requested changes May 16, 2022

View reviewed changes

caiwei-ebay force-pushed the master branch from 0773be2 to 3c9f683 Compare May 17, 2022 09:33

michael-o self-requested a review May 17, 2022 17:20

caiwei-ebay force-pushed the master branch 2 times, most recently from 295deb3 to 77a54f5 Compare June 15, 2022 10:34

download pom in parallel

5d32275

caiwei-ebay force-pushed the master branch 2 times, most recently from 7db6ae9 to 2614812 Compare June 17, 2022 12:23

fix relocation IT MNG-3769 failure with the parallel download patch

5cf45e8

caiwei-ebay force-pushed the master branch from 2614812 to 5cf45e8 Compare June 17, 2022 12:57

no need to handle the exception here, handle in future.get

2c805b5

caiwei-ebay marked this pull request as ready for review June 23, 2022 09:29

michael-o requested review from cstamas, mthmulders and hboutemy July 6, 2022 18:35

michael-o reviewed Jul 23, 2022

View reviewed changes

caiwei-ebay added 4 commits August 9, 2022 22:49

refactor a bit

6c067c8

fix using linkedHashMap in parallel stream

c3ae229

refactor

39da8e2

unused import

2f19ec2

cstamas reviewed Sep 29, 2022

View reviewed changes

...er-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java Outdated Show resolved Hide resolved

change method return value

b5cf751

cstamas approved these changes Oct 10, 2022

View reviewed changes

gnodet approved these changes Oct 22, 2022

View reviewed changes

cstamas reviewed Oct 26, 2022

View reviewed changes

cstamas merged commit 5ce7312 into apache:master Oct 26, 2022

Eskibear mentioned this pull request Jan 3, 2023

Support Maven 3.9.x to download dependency POMs in parallel eclipse-m2e/m2e-core#1169

Closed

testforstephen mentioned this pull request Mar 31, 2023

update default vmargs of jdtls to resolve dependencies in parallel redhat-developer/vscode-java#3030

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRESOLVER-7] download poms in parallel #178

[MRESOLVER-7] download poms in parallel #178

caiwei-ebay commented May 16, 2022 •

edited

Loading

caiwei-ebay commented May 16, 2022

michael-o left a comment

caiwei-ebay commented May 17, 2022

caiwei-ebay commented May 17, 2022

michael-o commented May 17, 2022

cstamas commented Jun 6, 2022

caiwei-ebay commented Jun 9, 2022

caiwei-ebay commented Jun 23, 2022 •

edited

Loading

caiwei-ebay commented Jul 6, 2022

cstamas commented Jul 6, 2022

michael-o commented Jul 6, 2022 •

edited

Loading

michael-o Jul 23, 2022

caiwei-ebay Jul 25, 2022

michael-o Jul 25, 2022

caiwei-ebay Aug 9, 2022 •

edited

Loading

caiwei-ebay Aug 9, 2022

cstamas Sep 29, 2022 •

edited

Loading

caiwei-ebay Sep 29, 2022

caiwei-ebay Oct 1, 2022

cstamas Oct 1, 2022

caiwei-ebay Oct 3, 2022

Eskibear commented Aug 9, 2022

caiwei-ebay commented Aug 9, 2022

michael-o commented Aug 9, 2022

Eskibear commented Sep 5, 2022

michael-o commented Sep 5, 2022

caiwei-ebay commented Sep 28, 2022

cstamas commented Sep 28, 2022

michael-o commented Sep 28, 2022

caiwei-ebay2 commented Oct 13, 2022

cstamas commented Oct 19, 2022

cstamas Oct 26, 2022

cstamas Oct 26, 2022

[MRESOLVER-7] download poms in parallel #178

[MRESOLVER-7] download poms in parallel #178

Conversation

caiwei-ebay commented May 16, 2022 • edited Loading

caiwei-ebay commented May 16, 2022

michael-o left a comment

Choose a reason for hiding this comment

caiwei-ebay commented May 17, 2022

caiwei-ebay commented May 17, 2022

michael-o commented May 17, 2022

cstamas commented Jun 6, 2022

caiwei-ebay commented Jun 9, 2022

caiwei-ebay commented Jun 23, 2022 • edited Loading

caiwei-ebay commented Jul 6, 2022

cstamas commented Jul 6, 2022

michael-o commented Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caiwei-ebay Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eskibear commented Aug 9, 2022

caiwei-ebay commented Aug 9, 2022

michael-o commented Aug 9, 2022

Eskibear commented Sep 5, 2022

michael-o commented Sep 5, 2022

caiwei-ebay commented Sep 28, 2022

cstamas commented Sep 28, 2022

michael-o commented Sep 28, 2022

caiwei-ebay2 commented Oct 13, 2022

cstamas commented Oct 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caiwei-ebay commented May 16, 2022 •

edited

Loading

caiwei-ebay commented Jun 23, 2022 •

edited

Loading

michael-o commented Jul 6, 2022 •

edited

Loading

caiwei-ebay Aug 9, 2022 •

edited

Loading

cstamas Sep 29, 2022 •

edited

Loading