Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[MNG-7615] Multithreaded model builder #893

Merged
merged 4 commits into from
Nov 7, 2023
Merged

[MNG-7615] Multithreaded model builder #893

merged 4 commits into from
Nov 7, 2023

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Dec 2, 2022 •
edited
Loading

JIRA issue: https://issues.apache.org/jira/browse/MNG-7615
Same PR as #803 but reformatted.

@gnodet gnodet added this to the 4.0.0-alpha-3 milestone Dec 2, 2022
@gnodet gnodet mentioned this pull request Dec 2, 2022
Closed
@gnodet gnodet marked this pull request as draft December 3, 2022 15:27
cstamas
cstamas reviewed Dec 3, 2022
View reviewed changes
maven-model-builder/src/main/java/org/apache/maven/model/superpom/DefaultSuperPomProvider.java Outdated
@@ -52,8 +53,8 @@ public DefaultSuperPomProvider(ModelProcessor modelProcessor) {

@Override
public Model getSuperModel(String version) {
if (superModel == null) {
String resource = "/org/apache/maven/model/pom-" + version + ".xml";
return SUPER_MODELS.computeIfAbsent(version, v -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that for example maven 4.0.1 could return super POM of Maven 3.8.6 (ie. newer could have all super POMs of older Mavens?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, though the only place it's called is with 4.0.0 afaik, and that's since ages. I found the code does not reflect the javadoc and that the version parameter is not used, so I fixed it.
If that's not intended, then we should remove the parameter completely.
The version was originally hardcoded, see dd635ca
The commit added a version parameter, but it's only used the first time and subsequent calls return the cached version.

As for having multiple superpoms, I think the problem is that the version has never been updated when the pom changed. So not sure what the future will be for the supermom...

@gnodet gnodet modified the milestones: 4.0.0-alpha-3, 4.0.0-alpha-4 Dec 3, 2022
@cstamas
Copy link
Member

cstamas commented Dec 3, 2022 •
edited
Loading

Tried to test drive it, first run on Java 11 was ok, but 2nd run seems deadlocked (console stopped at line 12):
https://gist.github.com/cstamas/d54715ffc24d7513e735353d17d8f78b

@cstamas
Copy link
Member

cstamas commented Dec 3, 2022 •
edited
Loading

Deadlock seems consistent, Ctrl-C after 6 minutes previous build, re-run (but this time added -V) and again:
https://gist.github.com/cstamas/62818af78abb293088cbb9383a278285

@cstamas
Copy link
Member

cstamas commented Dec 3, 2022

Those two runs above were Java 11. Switched to Java 17, first run was OK, but then it deadlocked again:
https://gist.github.com/cstamas/c44475bc3231363bc159d1a1d52f27c0

@gnodet gnodet force-pushed the mt-model-builder branch 2 times, most recently from 7d7e278 to a46f8b3 Compare December 7, 2022 13:46
@gnodet
Copy link
Contributor Author

gnodet commented Dec 9, 2022

So the PR works with 1 one thread here, but may deadlock irrespective on the number of threads.
The deadlock happen in the following scenario : 3 projects P1, P2, P3, P3 depends on P2, P2 depends on P1. The three projects are submitted to the FJ pool, which is able to steal work, so that jobs may be executed without any particular order.
So P2 is being executed, which requires P1. P1 is being joined by the FJ, but it decides to execute P3. P3 is being executed in the same thread and then waits for P2 being finished which can't happen.

@gnodet gnodet force-pushed the mt-model-builder branch 3 times, most recently from afec4a2 to e0d9d91 Compare December 30, 2022 16:35
@gnodet gnodet modified the milestones: 4.0.0-alpha-4, 4.0.0-alpha-5 Jan 27, 2023
@gnodet gnodet modified the milestones: 4.0.0-alpha-5, 4.0.0-alpha-6 Mar 7, 2023
@gnodet gnodet modified the milestones: 4.0.0-alpha-6, 4.0.0-alpha-7 May 23, 2023
@gnodet gnodet modified the milestones: 4.0.0-alpha-8, 4.0.0 Sep 15, 2023
@gnodet gnodet force-pushed the mt-model-builder branch 4 times, most recently from 6d9fb28 to a1d11a0 Compare September 21, 2023 08:50
@gnodet gnodet force-pushed the mt-model-builder branch 2 times, most recently from b4529bb to d6fb14f Compare September 22, 2023 11:15
@cstamas
Copy link
Member

cstamas commented Oct 17, 2023

Ran ITs of this PR + resolver 2.0.0 (both combined) and ITs are OK

@gnodet gnodet mentioned this pull request Nov 6, 2023
Merged
@gnodet gnodet marked this pull request as ready for review November 7, 2023 06:26
@gnodet gnodet merged commit 7fcdd32 into apache:master Nov 7, 2023
18 checks passed
@gnodet gnodet modified the milestones: 4.0.0, 4.0.0-alpha-9 Nov 7, 2023
@gnodet gnodet deleted the mt-model-builder branch November 10, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cstamas cstamas cstamas approved these changes

@mthmulders mthmulders Awaiting requested review from mthmulders

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.0-alpha-9
Development

Successfully merging this pull request may close these issues.
2 participants
@gnodet @cstamas