melix / jmh-gradle-plugin

Integrates the JMH benchmarking framework with Gradle
Apache License 2.0
666 stars 88 forks source link

Build an über jar using the standard jar plugin. #154

Closed jake-at-work closed 5 years ago

jake-at-work commented 5 years ago
coveralls commented 5 years ago

Coverage Status

Coverage decreased (-0.01%) to 51.737% when pulling 6482f72a459ff7bfc54b767bc33226bb3831e5af on pivotal-jbarrett:feature/uber-jar into 3f5003df4ef40f565173ee4339a7cfa6452cbbdc on melix:master.

melix commented 5 years ago

Thanks for the PR. However I think it contradicts the intent of this task, which is to avoid the creation of an uber jar unless you use the shadow plugin. In particular not all projects need an uber jar, they can keep their dependencies separate.

Actually I would prefer if we had an option to use either the shadow jar or your code, but keep the option to use separate jars. Does it make sense?

jake-at-work commented 5 years ago

Thanks for the feedback. The reason I changed this jar is because what was produced didn’t make sense to me. The jar contained the main and jmh classes and then jars from runtimeOnly (as jars embedded in this jar). I couldn’t find any real use for this jar. I assumed it was mean to produce something like the shadow jar but had been limited by older version of Gradle.

So what is the intended use case for this jar?

melix commented 5 years ago

I agree with you that the current jar doesn't make sense. I'm not quite sure why it was implemented this way, possibly as a workaround for some JMH classloading issue.

I think ideally the normal jar should remain the normal jar, nothing embedded. Then there should be something that generates a runnable JMH "app", and last there should be an option to create a fatjar.

jake-at-work commented 5 years ago

I agree with you that the current jar doesn't make sense. I'm not quite sure why it was implemented this way, possibly as a workaround for some JMH classloading issue.

Well if there isn't value in the current implementation we should probably change it. Is there a strong need to keep an unknown thing like this around?

I think ideally the normal jar should remain the normal jar, nothing embedded. Then there should be something that generates a runnable JMH "app", and last there should be an option to create a fatjar.

What do you see as a difference between a runnable JMH jar and a fat jar?

What I see is that we have two things, either you are running your JMH via Gradle or not.

If you run them from Gradle then you don't need the jar at all. You set the class path for the forked worker and it would execute the JHM runner, just the same as it would from the JMH app jar, but without a need to generate and include that jar.

If you run it outside of Gradle then you likely want to do it the JMH way, which is the app/fat/uber jar that is documented in JMH. In this case I don't think we really need to be using shadow anymore since Gradle's jar task can do all that now too.

I don't see much value in producing any other jar artifacts since it isn't common, in the JMH way of things, to exec java with a long line of jars in the class path including one that has the generated benchmark byte codes. (let Gradle do that ugliness for you)

Long message short, I see really only a need for the uber/fat/app jar that contains everything necessary to execute the benchmark, call this jmhJar, and a jmh task to execute JMH but from Gradle class paths and not dependent at all on the jmhJar. In another PR I will be opening is a POC of using Gradle task options to enhance the command line for jmh task, like ./gradlew jmh --includes=FooBenchmark --threads=8. Doing so makes needing the 'jmhJar` task pretty useless unless you want to script out some more advanced benchmarking or have existing tooling expecting the benchmark uber jar.

melix commented 5 years ago

Well if there isn't value in the current implementation we should probably change it. Is there a strong need to keep an unknown thing like this around?

No. To be honest I'm very unsatisfied by the implementation of this plugin. It should be simpler and easier to maintain. Only I don't have the bandwidth to do it.

What do you see as a difference between a runnable JMH jar and a fat jar?

I'm not particularly talking about a runnable jar. There should be different things:

Then we can provide something that can be executed from the CLI: java -cp bench.jar:jmh.jar Main

And what I'm saying is that their could be a convenience, just like the application plugin, to have something that runs it. Running from within Gradle as a task is already a convenience but if you do serious benchmarking you shouldn't really run from Gradle.

Last, there's the fatjar version. This version can be standalone, I don't really care.

If you run it outside of Gradle then you likely want to do it the JMH way, which is the app/fat/uber jar that is documented in JMH. In this case I don't think we really need to be using shadow anymore since Gradle's jar task can do all that now too.

Yes and no. Gradle can do it, but shadow has way more configuration. Merging jars is not a trivial thing. There are descriptors, for example, which may need to be merged (think of Groovy extension class descriptors).

I don't see much value in producing any other jar artifacts since it isn't common, in the JMH way of things, to exec java with a long line of jars in the class path including one that has the generated benchmark byte codes. (let Gradle do that ugliness for you)

There's one value in that it keeps the structure simple and maintainable. Each step is separate. Generate classes, compile, package in a jar, generate uber jar, ... It also makes it possible to integrate with other plugins if need be.

melix commented 5 years ago

So given this PR improves the situation I decided to merge it. I still think we should redesign a bit the architecture of this plugin which is too messy but at least what it does now is consistent. I will issue an rc2 soon.

melix commented 5 years ago

And 0.5.0-rc-2 is out with your changes.