Multi-Release-Jar with initial support for JDK 9 and JDK 10 NIO API

[mkarg]

Turning Plexus Utils into a Multi-Release-Jar to allow using the best API on each platform version available without increasing the minimum platform version.

This PR effectively replaces PRs #112 and #113, as per using NIO API there is no need for these separate PRs (they simply intended to use NIO API).

Note: Due to the way MRJARs are built, the actual content of the outcoming JAR is limited by the actual JDK release used to build. When using JDK 9, no JDK 10 APIs will be contained, etc..

[michael-o]

This is a great example I could even use in my project. @rfscholte is the expert here.

[rfscholte]

Please also confirm that every accessible method of touched by a test. The most tricky part with MRJAR is to keep all these method signatures in sync.
And of all methods, how many are adjusted and many still have the same implementation?

[mkarg]

Please also confirm that every accessible method of touched by a test.

I wonder if somebody knows a simple way to let Maven (here: on a JDK 10+ runtime) run tests multiple times using several release levels, so it effectively "fakes" being a JDK 8, 9, 10... VM?

The most tricky part with MRJAR is to keep all these method signatures in sync.
And of all methods, how many are adjusted and many still have the same implementation?

I think the best way here would not be to keep them in sync, but instead let the static facade call a private inner class that only contains the variant part, but all common part is in an abstract super class. Sounds complex but is rather easy and safe, and would effectively have us end up with a perfectly clear class structure holding only the modified methods. WDYT?

[rfscholte]

Right, if the amount of copied code is rather large, it is better to introduce a class just for the specific versions-implementations.

Regarding running the tests: https://maven.apache.org/plugins/maven-compiler-plugin/multirelease.html contains all kinds of strategies, none is perfect. So far this setup + a CI server works best for our projects.

[mkarg]

Right, if the amount of copied code is rather large, it is better to introduce a class just for the specific versions-implementations.

I will refactor the code of this PR in that way, so we have only the actually modified methods in the version-specific source folders.

Regarding running the tests: https://maven.apache.org/plugins/maven-compiler-plugin/multirelease.html contains all kinds of strategies, none is perfect. So far this setup + a CI server works best for our projects.

As the test matrix is already in place, the sole problem we have with this PR is that we need to split the stages for build from the stages for test. In the end, build stage must happen on JDK 10 and higher only, and that artifact is then to be tested on 8...16. I think that should work. Any better idea?

[mkarg]

Refactored code in bc5b0ab. No more complete duplicates of IOUtil but only lean overwrites of the actual version-specific methods. WDYT?

[rfscholte]

I think we can simplify without using a singleton.
Have one class, e.g. IOCopyUtil having the copy-methods, but static.

[mkarg]

Refactored in e16dc59. I prefer keeping the name VersionSpecifics so we can later put more methods from other classes into the same one, so the name should neither be bound to actually the copy methods or the IOUtils class.

[mkarg]

I have modified the Github CI script in cecd035 so it will build with JDK 15 always but just tests with the full matrix, hence the JDK 8 and 9 test is green again now! :-)

If we need to proof that we can build (not just test) on a full matrix (excluding builds on 8 and 9 as certainly these won't be able to compile Java 10 source code), then I could modify maven.yml to contain two separate jobs with different matrixes. Is that wanted / needed?

[rfscholte]

No, this is not good. You can only verify it works with JDK8 by building and testing it with JDK8.
And I see the issue: you're not using JDK activated profiles. See github.com/apache/maven-compiler-plugin/blob/master/src/it/multirelease-patterns/singleproject-runtime/pom.xml to see how it should work.

[mkarg]

Why can't I see if the JAR works on JDK 8 when I build it on 15 but test it with 8? For what is the test good then...?

That makes no sense to me since the result of that would be that on JDK 9 you end up with a JAR that would contain the variants for 8 and 9 but not for 10. So if we do not build with 10+ always, you would effectively get different content in the JAR. Why should somebody want that?

[rfscholte]

How do you test it with JDK8?

[mkarg]

I don't understand the question. As you can see in https://github.com/codehaus-plexus/plexus-utils/pull/120/checks?check_run_id=1640215941#step:7:12, the current CI matrix tests on Java 8, but it uses the JAR built using JDK 15 for the tests (see https://github.com/codehaus-plexus/plexus-utils/pull/120/checks?check_run_id=1640215941#step:5:10). This works pretty well as long as what you want to test is whether a JDK 15 built JAR works (i. e. finds the default implementation) on JDK 8.

[mkarg]

@rfscholte WDYT?

[rfscholte]

please just use JDK activated profiles. This will also make it easier to test locally.

[mkarg]

@rfscholte According to your wish, I have undone the Gitlab CI change, so now you can locally build using JDK 8 or 9 to get different multi-release JARs for those platforms (while still the general idea is to build on JDK 10+ to get the sole one to be published on Maven Central). Is it now in the way that you prefer?

[olamy]

What??? what has been done regarding #120 (comment) ??? If some jdk level is required we must enforce it via some enforcer rules to provide easy human readable message.

[olamy]

And seriously such change must be squashed into a single commit when merged in master branch.

[olamy]

I'm definitely -1 on such changes!!!

[olamy]

can you please fix that!!

[michael-o]

Merging dirty commits is a no go.

[olamy]

@michael-o and it's too late now...

[michael-o]

@michael-o and it's too late now...

True, I don't understand that

[mkarg]

Sorry if I did something wrong, but nobody was responding since five days, even after explicitly asking for comments, so I thought you are all fine (just as Robert was, who explicitly answered earlier). I do not see that an enforcer is needed, as no particular JDK level is actually needed. The solution works well with any JDK level, and the description of the PR clearly explains how it works. No harm is done actually. Whether or not you squash changes is a question of likes and dislikes. Neither did somebody request that, not did I find a written rule about that, and in many projects "dirty" commits for accepted in SNAPSHOTs. So if there is such a rule, and if you get asked by a new committer about comments, then you maybe should refer to it and request a change when asked for. I did not merge "hastily", I asked days ago. Sorry guys, but staying silent for five days was simply misunderstood.

[mkarg]

Guys, I am taking your comment very seriously so I checked the points you mentioned today. In fact, to me comment #120 (comment) was resolved by me changing the PR description and nobody responded to that, so I thought you are fine. The comments, besides two, are independent, and not dirty. For me, supporting Java 9, 10, MJARs, and changing things later due to Robert's request, are neither intermediate nor dirty commits, so actually I thought it is good to have them separate (some OS projects even prevent squashing but insist on having the full history of each merge branch). "dirty" is only the support for JDK 15, and yes, if you would have told me earlier (you had time since Janary 16 and was asked explicitly five days ago) I certainly would have followed your request.

So how to go on from here? Do you want me change change something now then please exactly tell me what. If there is a document that I shall read to prevent future "surprises" then please directly point me to that link.

Thanks guys, and sorry again.

[michael-o]

The problem ist that this topic is complex, I am not qualified to judge here. Most aren't. Five days sounds a lot, but it isn't. Most need weeks to find time for this. I am slow.

[mkarg]

I totally understand that, but the truth is, the PR is open since January 1st and the discussions werde finished in January 26th, so this is much more than five days. The five days were just a final reminder. Robert is the defintive expert, and he was fine with the solution provides by January 26th already.

[michael-o]

If @rfscholte was fine with that, I am too. But a cleanup is imperative.

[olamy]

Sorry if I did something wrong, but nobody was responding since five days, even after explicitly asking for comments, so I thought you are all fine (just as Robert was, who explicitly answered earlier). I do not see that an enforcer is needed, as no particular JDK level is actually needed. The solution works well with any JDK level, and the description of the PR clearly explains how it works. No harm is done actually. Whether or not you squash changes is a question of likes and dislikes. Neither did somebody request that, not did I find a written rule about that, and in many projects "dirty" commits for accepted in SNAPSHOTs. So if there is such a rule, and if you get asked by a new committer about comments, then you maybe should refer to it and request a change when asked for. I did not merge "hastily", I asked days ago. Sorry guys, but staying silent for five days was simply misunderstood.

Sorry but you are wrong! Enforcer rule is definitely needed as asked here: #120 (review)

If I build the project with java11 or java8 or java9 the produced jar will have different content.
with java8, everything related to java9 or java10 profile are not included.
As we do not enforce the java version, we are not sure about what will be released it will depends on the jdk used by the developer releasing the project.
Sorry but it's totally wrong!

jdk 11 build jar content:

unzip -l target/plexus-utils-3.4.0-SNAPSHOT.jar | grep version
        0  01-20-2020 18:52   META-INF/versions/
        0  01-20-2020 18:52   META-INF/versions/10/
        0  01-20-2020 18:52   META-INF/versions/10/org/
        0  01-20-2020 18:52   META-INF/versions/10/org/codehaus/
        0  01-20-2020 18:52   META-INF/versions/10/org/codehaus/plexus/
        0  01-20-2020 18:52   META-INF/versions/10/org/codehaus/plexus/util/
        0  01-20-2020 18:52   META-INF/versions/9/
        0  01-20-2020 18:52   META-INF/versions/9/org/
        0  01-20-2020 18:52   META-INF/versions/9/org/codehaus/
        0  01-20-2020 18:52   META-INF/versions/9/org/codehaus/plexus/
        0  01-20-2020 18:52   META-INF/versions/9/org/codehaus/plexus/util/
      843  01-20-2020 18:52   META-INF/versions/10/org/codehaus/plexus/util/BaseIOUtil.class
      942  01-20-2020 18:52   META-INF/versions/9/org/codehaus/plexus/util/BaseIOUtil.class

jdk8 build jar content:

unzip -l target/plexus-utils-3.4.0-SNAPSHOT.jar | grep version
mbp-olamy-2:plexus-utils olamy$ 

Your comment on no rule written on squash etc... Well it's really a common sense as this pr just have different commits with non related comments...

5 days. what is 5 days in a world with global pandemic, people working on different timezone and with easter weekend (school holidays which can change a lot the availability of some contributors)

[mkarg]

@olamy Sorry but you are wrong in some points, and I would kindly ask you to check my original commit for MJAR support instead of just looking at the final PR.

• It was not just five days, it was months. See the actual discussion above and my explanation from yesterday Multi-Release-Jar with initial support for JDK 9 and JDK 10 NIO API #120 (comment).
• Originally I proposed using JDK 15 to be mandatory (that's why I originally added it here cecd035 and my intention was to enforce it).
• It was Robert who explicitly asked to not enforce JDK 15, but to in fact make the build work with JDK 8. That's why I have undone that commit. This is what you call "dirty" but it is needed to understand the history and to prevent the current discussion.
• The PR must keep most of its commits standalone, as support for NIO 9 and NIO 10 won't work without support for MJARs, but actually are standalone ideas (that's why there originally were separate PRs for both), and support for MJARs make no sense without support for NIO 9 at least. The sole thing I could squash would be the history that proofs that I originally had enforced JDK 15, but then Robert wanted support for JDK 8. It seems, you didn't follow the PR's discussion, but in fact, it clearly explains why the PR is in the current state and why I merged it, and that it was not my idea to keep JDK 8, and that I explicitly warned Robert about the fact that in the end the changes he wanted lead to the fact that depending und the instaled JDK the result will be different. This all was his idea, not mine. Please read the full thread!

I deliberately did not squash it and deliberately waited for months for you to be able to follow exactly that situation.

So until you find an agreement with Robert, I simply cannot fix this, because what he wanted me to do is exactly the opposite of what you want me to do, and he was very eager about this discussion, while you had time since January 26th to respond but just didn't.

[rfscholte]

To ensure the the Java 8 classes still work with Java 8, you must run it with JDK8. That's why the CI server is important, you cannot verify this with a single Maven build on your local system.
However, for releasing you MUST use the highest required JDK to build the code. To do that you must add this as an enforcer rule to the release profile.

[mkarg]

So where to go from here, just add a commit ontop which adds the enforcer rule on the release profile, or squashing everything (i. e. breaking the branch's history) or just squashing the changes you personally requested (which still breaks history)? I need clear instructions how it is definitively to be done, since (as I said) this is not common sense and done differently in other open source projects. Thanks.

[olamy]

@olamy Sorry but you are wrong in some points, and I would kindly ask you to check my original commit for MJAR support instead of just looking at the final PR.

• It was not just five days, it was months. See the actual discussion above and my explanation from yesterday #120 (comment).
• Originally I proposed using JDK 15 to be mandatory (that's why I originally added it here cecd035 and my intention was to enforce it).

java15 as a minimun? really? not sure the IT industry has reached such level.

• It was Robert who explicitly asked to not enforce JDK 15, but to in fact make the build work with JDK 8. That's why I have undone that commit. This is what you call "dirty" but it is needed to understand the history and to prevent the current discussion.

who cares about having this in a code history. We are not here to store some fingers pointing. It's already part of the PR discussion no really need to add more.

• The PR must keep most of its commits standalone, as support for NIO 9 and NIO 10 won't work without support for MJARs, but actually are standalone ideas (that's why there originally were separate PRs for both), and support for MJARs make no sense without support for NIO 9 at least. The sole thing I could squash would be the history that proofs that I originally had enforced JDK 15, but then Robert wanted support for JDK 8. It seems, you didn't follow the PR's discussion, but in fact, it clearly explains why the PR is in the current state and why I merged it, and that it was not my idea to keep JDK 8, and that I explicitly warned Robert about the fact that in the end the changes he wanted lead to the fact that depending und the instaled JDK the result will be different. This all was his idea, not mine. Please read the full thread!

As I read this comment, some commits cannot work separately so better to have a single one.
More generally it's just better to have a new feature including in a single commit to be able to easily revert it if needed.

I deliberately did not squash it and deliberately waited for months for you to be able to follow exactly that situation.

So until you find an agreement with Robert, I simply cannot fix this, because what he wanted me to do is exactly the opposite of what you want me to do, and he was very eager about this discussion, while you had time since January 26th to respond but just didn't.

Perso I don't see adding this build complexity (debugging) as a real step forward. Does it really worth? Is it really so much performant?
Anyway java9 and java10 are dead release. I would prefer to have only java11.

[mkarg]

@olamy JDK 15 was initially proposed as the enforced build JDK as it was the latest GA release available on Github, so it would allow to add support for more version folders in the MJAR without changing the POM; anyways I just told you that to proof that I did have the idea of enforcing the JDK version used for the build (just as you do request it). Hence I have no problem with reducing the enforcement to just JDK 11 at all, I'm just waiting for instructions as you see in the PR comments. Whether something is a dead horse plays no role for MJARs: These are the minimum levels the API changes will work upon, so in fact, we do currently support 8, 9 and 10, but actually 8, 9, 10, 11, 12, 13, 14, 15, 16, 17-EA with this very same solution. "Just 11" would not work as @rfscholte insisted on having support for 8, too (see the PR history). And yes, it is worth the complexity, and we should concentrate our discussion on going forward not backwards.

@rfscholte As I do not want to do a second failure in your unwritten processes, I need clear instructions whether I may directly rewrite the history (as apparently targeted by @olamy IIUC his complaint) or whether I may push the needed commit for the enforcer just ontop, or whether I shall provide a new PR with just that single commit. Or whatever else this community wants me to do to stop the discussion about the past and go in the right direction for the future.

See, now that I learned that nobody will point a finger on anybody (which I really appreciate) I really want to help fixing the "mess" as best as I can, but *I need to know which way the majority wants me to go. Thanks.

[rfscholte]

This is the first time I hit this situation, so I'd follow Oliviers advice here.

[olamy]

please provide a PR. rewriting master branch history is just worst.
can you please provide some jmh tests to prove it really worths such complexity?

[mkarg]

please provide a PR. rewriting master branch history is just worst.
can you please provide some jmh tests to prove it really worths such complexity?

I will provide a PR for the enforcer.

Regarding JMH I will look into something on my Youtube channel regarding performance in general.

[olamy]

please provide a PR. rewriting master branch history is just worst.
can you please provide some jmh tests to prove it really worths such complexity?

I will provide a PR for the enforcer.

Regarding JMH I will look into something on my Youtube channel regarding performance in general.

take it easy buddy we do not need some TV show but just few jmh tests....

[mkarg]

Regarding JMH I will look into something on my Youtube channel regarding performance in general.
take it easy buddy we do not need some TV show but just few jmh tests....

comparing old-school code with NIO 10 code sounds like a good topic for a shootout video, and I am always looking for interesting topics for my channel anyways

[mkarg]

can you please provide some jmh tests to prove it really worths such complexity?

@olamy It took some time as I was rather busy, but meanwhile I found the time to provide the JMH benchmark.

Please find https://gitlab.com/mkarg/jaxrs-done-right/-/blob/1a85b315470af7bd5702dd6762d912c0f053f5ca/src/main/java/eu/sandbox/Sandbox.java.

Ignoring all the out-commented benchmarks, it effectively just compares what Plexus-Utils did before (using NIO 7) and after (using NIO 9 and NIO 10) my merge of the MRJAR PR.

I have performed this benchmark on both, Linux and Windows, and while certainly the difference currently is not dramatic yet, I personally would say the complexity (which I, in fact, would not say is really big) is worth the improved throughput, in particular as the rather complex implementation of the transferTo() methods clearly could be further optimized, while the explicit custom code that one had to write in lieu of transferTo() simply already reached the peak optimization that is possible outside of the JRE.

The actual numbers of my Windows Laptop are:

Sandbox.java07_Files_newBufferedReader        1000  thrpt   25  3257,480 ▒  75,403  ops/s
Sandbox.java07_Files_newBufferedReader     1000000  thrpt   25   518,090 ▒  14,041  ops/s
Sandbox.java07_Files_newBufferedReader  1000000000  thrpt   25     0,472 ▒   0,008  ops/s
Sandbox.java07_Files_newInputStream           1000  thrpt   25  3446,345 ▒ 197,931  ops/s
Sandbox.java07_Files_newInputStream        1000000  thrpt   25  1136,500 ▒  24,773  ops/s
Sandbox.java07_Files_newInputStream     1000000000  thrpt   25     1,137 ▒   0,023  ops/s
Sandbox.java09_InputStream_transferTo         1000  thrpt   25  3332,082 ▒  72,743  ops/s
Sandbox.java09_InputStream_transferTo      1000000  thrpt   25   865,717 ▒  24,389  ops/s
Sandbox.java09_InputStream_transferTo   1000000000  thrpt   25     0,733 ▒   0,003  ops/s
Sandbox.java10_Reader_transferTo              1000  thrpt   25  3141,430 ▒  71,134  ops/s
Sandbox.java10_Reader_transferTo           1000000  thrpt   25   545,857 ▒  15,744  ops/s
Sandbox.java10_Reader_transferTo        1000000000  thrpt   25     0,489 ▒   0,004  ops/s

Some actual numbers of a Linux cloud container:

Sandbox.java07_Files_newBufferedReader        1000  thrpt    5  31405.640 ±  3084.986  ops/s
Sandbox.java07_Files_newBufferedReader     1000000  thrpt    5    475.655 ±   107.712  ops/s
Sandbox.java07_Files_newBufferedReader  1000000000  thrpt    5      0.432 ±     0.017  ops/s
Sandbox.java07_Files_newInputStream           1000  thrpt    5  63765.430 ± 15673.334  ops/s
Sandbox.java07_Files_newInputStream        1000000  thrpt    5   1549.640 ±   150.097  ops/s
Sandbox.java07_Files_newInputStream     1000000000  thrpt    5      1.087 ±     0.110  ops/s
Sandbox.java09_InputStream_transferTo         1000  thrpt    5  75235.225 ±  9918.075  ops/s
Sandbox.java09_InputStream_transferTo      1000000  thrpt    5   1344.171 ±    91.492  ops/s
Sandbox.java09_InputStream_transferTo   1000000000  thrpt    5      0.996 ±     0.031  ops/s
Sandbox.java10_Reader_transferTo              1000  thrpt    5  36727.881 ±   684.608  ops/s
Sandbox.java10_Reader_transferTo           1000000  thrpt    5    492.899 ±    24.666  ops/s
Sandbox.java10_Reader_transferTo        1000000000  thrpt    5      0.442 ±     0.026  ops/s

What these numbers do tell us is that the differences between NIO 7, 9 and 10 are different on Windows and Linux (besides the fact that one cannot compare a Windows Laptop to a Linux cloud container, certainly), but also, and this is much more important, Reader.transferTo(Writer) of NIO 10 outperforms custom array-copy-loops of NIO 7 (while being more comfortable). And yes, I also panicked when I noticed that InputStream.transfertTo(OutputStream) of NIO 9 actually is much slower than custom array-copy-loops of NIO 7! As it happens always, and as it happens on Windows Laptop and Linux cloud container, my next step is debugging into the OpenJDK source code what the heck is going on there and possibly proposing a bug fix there.

If that means that you want to undo NIO 9 support until then, feel free... but I personaly think we should not workaround specific performance bugs of particular JDK implementations but instead fix them, as we do not know if they even exist in JDK 17 or on ARM or on Mac...

[mkarg]

@olamy More performance news. Looked into the JRE source and did some more benchmarks. Actually the performance drop of InputStream.transferTo reported above is not bug at all. It is simply the reflection of the fact that the original Plexus code used a slightly bigger buffer when copying files, effectively making the Plexus code being optimized for larger files than the optimization chosen inside of the JRE itself which is for good for smaller files. It is doubtful what the better optimization goal is. Anyways, I will discuss with the OpenJDK team how we can make InputStream.transferTo perform better, or even use more offloading (I did a benchmark with an offloading prototype effectively outperforming any other algorithm, including the original Plexus code, unambigously. So I would say, supporting InputStream.transferTo might be a step back for some file sizes temporarily, but has the potential to outperform in future, and I am pretty sure the OpenJDK team will adopt a feasible optimization. Stay tuned.

[mkarg]

@olamy Performance update: Filed enhancement proposal meanwhile, now waiting for OpenJDK's answer. If they accept it, InputStream.transferTo will be the fastest possible solution at all, as the proposal uses NIO channels and buffers which turned out to actually completely offload to the OS, and outperformed all other methods in my JMH benchmark. Exciting! :-)