Skip to content

Add org.eclipse.tm4e.core.benchmarks#389

Closed
sebthom wants to merge 1 commit intoeclipse-tm4e:masterfrom
sebthom:jmh
Closed

Add org.eclipse.tm4e.core.benchmarks#389
sebthom wants to merge 1 commit intoeclipse-tm4e:masterfrom
sebthom:jmh

Conversation

@sebthom
Copy link
Member

@sebthom sebthom commented May 9, 2022

This PR adds a test-plugin containing a JMH based benchmark of Grammar#tokenizeLine.
The benchmark is not executed during the regular build but can be launched using the provided run-benchmark.sh / run-benchmark.cmd scripts.

image

@sebthom sebthom requested a review from mickaelistria May 9, 2022 15:14
@sebthom
Copy link
Member Author

sebthom commented May 9, 2022

@mickaelistria the license check failed because jmh ist not recognized. I don't know how to fix this. Also since we are not bundling/publishing jmh I am not sure how relevant this is.

@mickaelistria
Copy link
Contributor

That's very interesting. I'm not much familiar with JMH and benchmarks. Can you please add a README.md file or something like that in the module to describe how to run it and how to interpret the results and whatever info that could help?

@mickaelistria
Copy link
Contributor

I'm afraid JMH is GPL so we not may be able to license this bundle under EPL if we want to use JMH. We'd need the permission to publish this code under GPL as well.

@sebthom
Copy link
Member Author

sebthom commented May 9, 2022

Oh, I overlooked that :-/

I will try to come up with an alternative, I still think it is useful to have some standard benchmark to better understand the implications of certain changes.

@jonahgraham
Copy link

I'm afraid JMH is GPL so we not may be able to license this bundle under EPL if we want to use JMH. We'd need the permission to publish this code under GPL as well.

I believe this conclusion was too conservative here, jmh has been approved for use with numerous projects. iplab search results

@mickaelistria
Copy link
Contributor

Approved for use, OK; but here it's different, what is proposed is to host code that uses JMH APIs.
But JMH APIs (such as @benchmark) are licensed under GPLv2
"""

  • This code is free software; you can redistribute it and/or modify it
  • under the terms of the GNU General Public License version 2 only, as
  • published by the Free Software Foundation. Oracle designates this
  • particular file as subject to the "Classpath" exception as provided
  • by Oracle in the LICENSE file that accompanied this code.
    """
    So that means that the code provided in this PR should be GPLv2 too. The license must be fixed before we can consider moving into the repo (the bundle should probably be GPLv2, and then when it's done, we can ask EMO whether we would be allowed to have 1 bundle being GPL in the project code)

@jonahgraham
Copy link

I had cross-referenced here because @sebthom had cited this PR as a reason not to accept a similar PR in LSP4J. I think that the LSP4J (and this PR) are the same as how RDF uses it.

licensed under GPLv2

But, not just normal GPLv2, but the classpath exception

Benchmark.java (for example) is explicitly in the classpath exception as designated by the JMH LICENSE. The licensing of JMH is the same as the JDK's standard library (e.g. String.java + JDK license).

Have I missed something above? If you still think this isn't ok I will raise it with Eclipse Foundation and block merging eclipse-lsp4j/lsp4j#929

@mickaelistria
Copy link
Contributor

Have I missed something above?

Apparently not at all; you've demonstrated I was the one who was missing something (the classpath exception).
Under this fresh (to me) piece of news. It looks like my concerns are indeed invalid and the proposed code could be licensed and published under EPLv2 without exception.
@sebthom Would you like to reopen this PR?

@jonahgraham
Copy link

Thanks @mickaelistria for checking again. Much appreciated and we will proceed with adding the equivalent to LSP4J in eclipse-lsp4j/lsp4j#929

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants