-
Notifications
You must be signed in to change notification settings - Fork 297
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
7903448: JMH: Override the use of compiler hints using a system property #96
7903448: JMH: Override the use of compiler hints using a system property #96
Conversation
👋 Welcome back gdub! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I understand the use case. But I also think that accepting a boolean as jmh.compilerhints.mode
is ambiguous, especially if we ask questions about defaults :) It would be cleaner to do it like we do it for Blackholes:
public enum CompilerHintsMode {
FORCE_ON("Forced on"),
FORCE_OFF("Forced off"),
AUTO("Automatically selected"),
}
Then parse that enum off the jmh.compilerhints.mode
, default to AUTO
.
Good idea. I'll do |
Also, go to https://github.com/gilles-duboscq/jmh/actions and press a large green button to enable PR testing. |
Using `-Djmh.compilerhints.mode=FORCE_ON` will force the use of compiler hints. `-Djmh.compilerhints.mode=FORCE_OFF` will prevent the use of compiler hints. When the system property is not set, or when it is set to `AUTO`, the current beahviour based on vm name and version will be used.
8996a68
to
5e550e4
Compare
@gilles-duboscq Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
I have changed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have more comments, apologies :)
/** | ||
* FIXME (low priority): check if supplied JVM is hint compatible. This test is applied to the Runner VM, | ||
* not the Forked and may therefore be wrong if the forked VM is not the same JVM | ||
*/ | ||
private static boolean isHintCompatibleVM() { | ||
private static boolean compilerHintsEnabled() { | ||
if (compilerHintsEnabled != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we set compilerHintsEnabled
? I think it needs to be set before every return
from this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
out.print("# Compiler hints: " + (compilerHintsEnabled() ? "enabled" : "disabled") + " (" + compilerHintsSelect().desc() + ")"); | ||
out.println(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's print this only if !compilerHintsSelect().isAuto()
. The normal use case is having compiler hints transparently enabled. This is quite a bit different from Blackhole that does have a normal variance in settings...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
jmh-core/src/main/java/org/openjdk/jmh/runner/CompilerHints.java
Outdated
Show resolved
Hide resolved
jmh-core/src/main/java/org/openjdk/jmh/runner/CompilerHints.java
Outdated
Show resolved
Hide resolved
jmh-core/src/main/java/org/openjdk/jmh/runner/CompilerHints.java
Outdated
Show resolved
Hide resolved
Ping @gilles-duboscq -- do you want to complete this, or want me to take over? |
Other things have taken over but i was still planning to get back to this within the next 1-2 weeks. If somebody else needs that faster, feel free to take over. |
That's fine, there is no time pressure to complete this work. Looking forward to new version in a few weeks. I plan to have the next JMH release some time in May. |
Only print compiler hints status when it is forced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version looks good!
There are some flukes in GHA testing, but they all seem to be environmental.
@gilles-duboscq This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 4 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@shipilev) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
@gilles-duboscq |
/sponsor |
Going to push as commit c5c93a2.
Your commit was automatically rebased without conflicts. |
@shipilev @gilles-duboscq Pushed as commit c5c93a2. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Using
-Djmh.compilerhints.mode=true
will force the use of compiler hints.-Djmh.compilerhints.mode=false
will prevent the use of compiler hints. When the system property is not set, the current beahviour based on vm name and version will be used by default.This is especially useful when working with uncommon / rare JVMs or when adding compiler hints support to a new JVM.
I considered auto-detecting support for compiler hints but that didn't fit very well in the current code and required much more intrusive changes.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jmh.git pull/96/head:pull/96
$ git checkout pull/96
Update a local copy of the PR:
$ git checkout pull/96
$ git pull https://git.openjdk.org/jmh.git pull/96/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 96
View PR using the GUI difftool:
$ git pr show -t 96
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jmh/pull/96.diff
Webrev
Link to Webrev Comment