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.

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

Add maven dependency for JMH framework. #182

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@C0rWin
Copy link
Contributor

C0rWin commented Aug 13, 2016

In order to provide patch for LANG-1110, required dependcy on JMH lib.
Current commit add benchmark profile and ability to run JMH based benchmark by
executing "mvn test -P benchmark" command, moreover it's also possible to
specify exact benchmark name by running "mvn test -P benchmark
-Dbenchmark=benchmark.full.class.name".

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 13, 2016

Coverage Status

Coverage decreased (-0.007%) to 93.43% when pulling 9f4c0ec on C0rWin:LANG-1256 into 648eebb on apache:master.

@C0rWin

This comment has been minimized.

Copy link
Contributor Author

C0rWin commented Aug 13, 2016

No Java code was involved in this PR, how that possible coverage reduced?

@PascalSchumacher

This comment has been minimized.

Copy link
Contributor

PascalSchumacher commented Aug 20, 2016

@C0rWin The OpenJDK 7 Build did not finish: https://travis-ci.org/apache/commons-lang/jobs/151969189 maybe that caused the change. Or there is a random element in the existing test?

@C0rWin

This comment has been minimized.

Copy link
Contributor Author

C0rWin commented Aug 20, 2016

@PascalSchumacher As far as I know coverage could be reduced only if I've added new code which is not tested or not covered by existing tests.

pom.xml Outdated
@@ -563,6 +578,10 @@
<commons.site.path>lang</commons.site.path>
<commons.scmPubUrl>https://svn.apache.org/repos/infra/websites/production/commons/content/proper/commons-lang</commons.scmPubUrl>
<commons.scmPubCheckoutDirectory>site-content</commons.scmPubCheckoutDirectory>

<!-- JMH Benchmark related properties, version, target compiler and name of the benchmarking uber jar. -->

This comment has been minimized.

Copy link
@PascalSchumacher

PascalSchumacher Oct 20, 2016

Contributor

Either the comment is wrong or there are properties missing?

@kinow

This comment has been minimized.

Copy link
Member

kinow commented Feb 12, 2017

Hi @C0rWin

Some time ago I found this pull request and had to learn what was JMH :-) added a comment on LANG-1110, and today had time to revisit the issue and take a look at the replies.

So Commons CSV and Commons RNG use JMH too. Both projects apply a similar approach, with the main difference being that Commons RNG - maybe for being a multi-module project - contains a jmh Maven module.

However, the dependencies and plug-in execution are part of a benchmark profile. Following the suggestions in LANG-1110, would be nice if we:

I wanted to check with you before I opened a separate pull request. Would you be willing to update this pull request again with these changes? If so, just ping me and I'll reply here as soon as possible. Have plenty of time to work on Open Source projects in the next two months :-)

Cheers
Bruno

@C0rWin

This comment has been minimized.

Copy link
Contributor Author

C0rWin commented Feb 12, 2017

Add maven dependency for JMH framework.
In order to provide patch for LANG-1110, required dependcy on JMH lib.
Current commit add benchmark profile and ability to run JMH based benchmark by
executing "mvn test -P benchmark" command, moreover it's also possible to
specify exact benchmark name by running "mvn test -P benchmark
-Dbenchmark=benchmark.full.class.name".

@C0rWin C0rWin force-pushed the C0rWin:LANG-1256 branch from 9f4c0ec to d38aea4 Feb 12, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 12, 2017

Coverage Status

Coverage remained the same at 94.536% when pulling d38aea4 on C0rWin:LANG-1256 into b715d18 on apache:master.

@PascalSchumacher

This comment has been minimized.

Copy link
Contributor

PascalSchumacher commented Feb 21, 2017

I guess this is ready to merge?

@kinow

This comment has been minimized.

Copy link
Member

kinow commented Feb 21, 2017

I was waiting for @C0rWin to update the pull request, to have an approach similar to what we have in other commons projects. If he has updated it, then +1 to merging (or maybe check with Emmanuel Bourg if it looks good too?)

@PascalSchumacher

This comment has been minimized.

Copy link
Contributor

PascalSchumacher commented Feb 21, 2017

As far as I can tell @C0rWin updated the pull request 9 days ago (after your conversation).

By the way merging this only makes sense if commons lang actually uses JHM. Do you (or somebody else) plan to work on this?

@kinow

This comment has been minimized.

Copy link
Member

kinow commented Feb 21, 2017

Oh, missed that one, will take a look today to compare with [csv].

I've never seen it failing in Jenkins or Travis CI, but I suspect it is run manually, whenever someone needs to work on the classes involved in the benchmark tests for new features or debugging issues in different environments.

@kinow

This comment has been minimized.

Copy link
Member

kinow commented Feb 22, 2017

Tried running the following to test the pull request:

$ mvn clean install
$ mvn test -Pbenchmark -e -X

And it kept failing with:

[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 9.718 s
[INFO] Finished at: 2017-02-22T20:00:57+13:00
[INFO] Final Memory: 32M/486M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.5.0:exec (benchmark) on project commons-lang3: Command execution failed. Process exited with an error: 1 (Exit value: 1) -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.5.0:exec (benchmark) on project commons-lang3: Command execution failed.
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:212)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:153)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:145)
	at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:116)
	at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:80)
	at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:51)
	at org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:128)
	at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:307)
	at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:193)
	at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:106)
	at org.apache.maven.cli.MavenCli.execute(MavenCli.java:863)
	at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:288)
	at org.apache.maven.cli.MavenCli.main(MavenCli.java:199)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:289)
	at org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:229)
	at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:415)
	at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:356)
Caused by: org.apache.maven.plugin.MojoExecutionException: Command execution failed.
	at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:302)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:207)
	... 20 more
Caused by: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1)
	at org.apache.commons.exec.DefaultExecutor.executeInternal(DefaultExecutor.java:404)
	at org.apache.commons.exec.DefaultExecutor.execute(DefaultExecutor.java:166)
	at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:764)
	at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:711)
	at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:289)
	... 22 more

Tried playing with dependency versions and settings, using Commons CSV as example (which works for me, as long as manually install one maven dependency as per pom.xml comment) with no luck.

Will give it another try tomorrow at work.

$ mvn -v
Apache Maven 3.3.9 (bb52d8502b132ec0a5a3f4c09453c07478323dc5; 2015-11-11T05:41:47+13:00)
Maven home: /opt/maven
Java version: 1.8.0_121, vendor: Oracle Corporation
Java home: /usr/lib/jvm/java-8-oracle/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "4.4.0-62-generic", arch: "amd64", family: "unix"
@C0rWin

This comment has been minimized.

Copy link
Contributor Author

C0rWin commented Feb 22, 2017

I'd guess it fails since there is no JMH tests available in the project, so it fails to execute them.

@C0rWin

This comment has been minimized.

Copy link
Contributor Author

C0rWin commented Feb 22, 2017

Actually once this one will be in, I have a few tests based on JMH which I can push.

@kinow

This comment has been minimized.

Copy link
Member

kinow commented Feb 22, 2017

@C0rWin what about trying to add the test from LANG-1110 to the pull request? Look at the attachments section.

I noticed the title of the pull request, but maybe we could change it to be a complete patch for LANG-1110. What do you think?

@C0rWin

This comment has been minimized.

Copy link
Contributor Author

C0rWin commented Feb 22, 2017

I'm fine with it, will add the test from LANG-1110.

@asfgit asfgit closed this in 111fd3f Apr 28, 2017

@PascalSchumacher

This comment has been minimized.

Copy link
Contributor

PascalSchumacher commented Apr 28, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.