Improve maven multithreaded execution detection.

[asbachb]

As mvnd is multi threaded by default we need to disable the event spy which is normally disabled when using multiple threads. This fixes #5479 .

The PR also contains a performance fix which saves some time on every maven command execution

[mbien]

awesome. We have a red master situation atm, have to resolve that first before starting the tests here. Going to be back later I promise :)

[mbien]

looks good to me - thanks for fixing this.

good thinking regarding the optimization, this is certainly quicker than checking jar contents.

just a question, is this email address correct?
https://github.com/apache/netbeans/actions/runs/4175903071/jobs/7231902202#step:4:24
it looks a bit project specific :)

[asbachb]

just a question, is this email address correct? https://github.com/apache/netbeans/actions/runs/4175903071/jobs/7231902202#step:4:24 it looks a bit project specific :)

Yeah that's fine. Normally I create an e-mail address per purpose.

[mbien]

@asbachb I believe we should think about this a bit more :)

One difference between mvnd and maven is that mvnd uses different default settings. It uses the "smart builder" and uses more threads.

The maven standard builder can be set via -1 or --serial and thread count can be also set to 1 with -T 1.

So the question is, while using mvnd:

• what happens if -T 1 is set and isMultiThreaded is updated to return false, can event spy be used in multi module projects?
• does the builder type in use influence anything (probably not)
• is there an easy way for MavenCommandLineExecutor to detect if the project is single module?

It might be better to base the decision whether event spy should be used based on the computed settings, not based on whether the executable is mvnd or maven.

[asbachb]

@mbien I know the code is and was not optimal. The multi threaded detection is not very robust. On the other hand that PR makes the standard use case (don't configure any threading manually) somehow workable.

So from my point of view for now it's more important that multi modules give some output at all than having a nice output for single module projects.

what happens if -T 1 is set and isMultiThreaded is updated to return false, can event spy be used in multi module projects?

I don't know tbh. Based on the previous code I'd say: no. As it was disabled on purpose.

does the builder type in use influence anything (probably not)

What do you mean by "builder type"?

is there an easy way for MavenCommandLineExecutor to detect if the project is single module?

From my understanding the pom.xml is parsed and put into maven api objects. When we could access that pom.xml representation from MavenCommandLineExecutor and evaluate if it contains modules.

Final note: The output when running mvnd with multiple threads in NetBeans is not really helpful as it's mixed all over the place. When executing mvnd from console the output updates and after compilation consolidated log is printed (if I remember correctly it's also sorted by module).

[mbien]

So from my point of view for now it's more important that multi modules give some output at all than having a nice output for single module projects.

I do agree that this is an important fix. But we should also check if there is a way to not cause regressions in single module projects which could potentially use the event spy. There is no need to rush this in yet, lets wait a bit - we are early in the NB 18 cycle.

I don't know tbh. Based on the previous code I'd say: no. As it was disabled on purpose.

the previous code expected that -T X meant that someone enabled MT, but someone could set -T 1 for mvnd which would make it ST. We could even consider making -T 1 the default setting for maven and mvnd if it causes the least problems in the console and allows event spy usage. NB already sets -ntp by default since #4954.

What do you mean by "builder type"?

explained here: https://peter.palaga.org/2021/01/11/mvnd-parallel-builds.html, mvnd also uses a different builder than maven.

Final note: The output when running mvnd with multiple threads in NetBeans is not really helpful

yes this will be hard to fix if at all possible with the current way the console works

[asbachb]

Maybe it makes sense to split the PR. So we at least could merge the performance optimizations.

[asbachb]

We could even consider making -T 1 the default setting for maven and mvnd if it causes the least problems in the console and allows event spy usage.

I don't think that would be a good idea for mvnd. I'd say the default settings for mvnd to use multiple threads and a smart builder type are one of the main selling points for mvnd if we remove them by default mvnd would not bring much benefit.

[mbien]

I don't think that would be a good idea for mvnd. I'd say the default settings for mvnd to use multiple threads and a smart builder type are one of the main selling points for mvnd if we remove them by default mvnd would not bring much benefit.

it is still a warmed up daemon even when single threaded.

[asbachb]

I'd rather have fast builds then nice console output. But maybe it's just a personal preference.

[asbachb]

if (mvnd && (list.contains("-T 1") || list.contains("--threads 1"))) {

I'm not sure it's that easy. This would also trigger -T 10 and -T 1C

[asbachb]

Let me prepare a proposal for the logic. I'm working on something including unit tests.

[mbien]

I'm not sure it's that easy. This would also trigger -T 10 and -T 1C

I know. This is just a test setup so that we can experiment with this a bit better since the old impl of the method would return true too often.

@asbachb we could potentially merge this as is and update things later in follow ups. But again: there is no hurry, lets play with this a bit a week or two and then decide what to do.

[asbachb]

@mbien I pushed a change to my branch maybe you'd like to have a look: 4f58f54

So the base idea was to extract the functionality somehow in order to be able to do some unit testing. Maybe it's a little bit overkill to have two separated classes but I thought it might be more clear that there is a different behavior between default maven and maven daemon.

Love to hear your thoughts.

Edit: There are still some bugs. Just as proposal how we could handle the check.

[mbien]

I like it overall. Comments inline for you to consider. (only a quick review)

[mbien]

@asbachb you ok with me pushing a commit to this PR? Would like to try to get this in for NB18.

[asbachb]

Sure. Feel free to force push and reorganize as you like.

[mbien]

rebased to latest master since we had some more maven changes in the meantime, squashed everything, CI will produce a dev-build to play around with.

changes:

• inlined everything to two small methods
• they use the command line params directly, instead of building a String first and scanning it as it used to be

let me know what you think if you have time. Its probably worth to take another look at maven command line flags to check if there are more we should add.

[asbachb]

General remark: From my understanding mvnd could also be configured via configuration file. So just relying on the command line parameters might result in non working configurations as well.

[mbien]

@asbachb yeah true, lets ignore this for now. mvnd itself is a moving target atm. There might be even a way to talk to the daemon directly for IDEs some time in future, without having to pretend to be a shell.

[mbien]

added a check for -Dmvnd.threads=1, planning to merge once CI is green again.