On Thu, 23 Mar 2023 18:45:29 GMT, Gilles Duboscq <gdub at openjdk.org> wrote:
>> 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.
>
> Gilles Duboscq has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
>
> Allow forcing the use of compiler hints using a system property
>
> 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.
I have more comments, apologies :)
jmh-core/src/main/java/org/openjdk/jmh/runner/CompilerHints.java line 108:
> 106: FORCE_ON("Forced on"),
> 107: FORCE_OFF("Forced off"),
> 108: AUTO("Automatically selected");
I am thinking it would be easier if we separate `AUTO_ON` and `AUTO_OFF`? This would eliminate `compilerHintsEnabled`, because you can just use `compilerHintsSelect` for everything, and treat its `null` is uninitialized. Might want to introduce a few helper methods in this enum, like `isAuto()` and `isEnabled()`.
jmh-core/src/main/java/org/openjdk/jmh/runner/CompilerHints.java line 134:
> 132: */
> 133: private static boolean compilerHintsEnabled() {
> 134: if (compilerHintsEnabled != null) {
Where do we set `compilerHintsEnabled`? I think it needs to be set before every `return` from this method.
jmh-core/src/main/java/org/openjdk/jmh/runner/CompilerHints.java line 232:
> 230: public static void addCompilerHints(List<String> command) {
> 231: if (!compilerHintsEnabled()) {
> 232: if (compilerHintsSelect() == CompilerHintsSelect.AUTO) {
Collapsible `if`-s. I'd say:
if (compilerHintsSelect() == CompilerHintsSelect.AUTO_OFF) {
...
}
jmh-core/src/main/java/org/openjdk/jmh/runner/CompilerHints.java line 235:
> 233: System.err.println("WARNING: Not a HotSpot compiler command compatible VM (\""
> 234: + System.getProperty("java.vm.name") + "-" + System.getProperty("java.version")
> 235: + "\"), compilerHints are disabled.");
While you are at it, please change "compilerHints" -> "compiler hints".
jmh-core/src/main/java/org/openjdk/jmh/runner/CompilerHints.java line 437:
> 435: public static void printHints(PrintStream out) {
> 436: out.print("# Compiler hints: " + (compilerHintsEnabled() ? "enabled" : "disabled") + " (" + compilerHintsSelect().desc() + ")");
> 437: out.println();
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...
-------------
Changes requested by shade (Committer).
PR Review: https://git.openjdk.org/jmh/pull/96#pullrequestreview-1356241031
PR Review Comment: https://git.openjdk.org/jmh/pull/96#discussion_r1147285403
PR Review Comment: https://git.openjdk.org/jmh/pull/96#discussion_r1147277919
PR Review Comment: https://git.openjdk.org/jmh/pull/96#discussion_r1147281285
PR Review Comment: https://git.openjdk.org/jmh/pull/96#discussion_r1147290287
PR Review Comment: https://git.openjdk.org/jmh/pull/96#discussion_r1147279761