Hi Jorn,
looks reasonable to me, and I can sponsor. A review from one of the
build maintainers would be appreciated.
/Claes
On 2020-06-03 22:01, Jorn Vernee wrote:
Hi,
Please sponsor the following patch that correctly passes
java.library.path when running micro benchmarks [1].
As a little bit of background; previously the java.library.path was
appended to $1_MICRO_JAVA_OPTIONS and this was passed to the java VM
running the JMH jar. But, the problem with this is that the path is not
passed on to the forks that are created by JMH, so it only works when
running without forks.
As a fix for this, the java.library.path was instead appended to
JMH_JVM_ARGS which is ultimately passed to JMH using the -jvmArgs
option, which will make sure it is passed to the forks created by JMH as
well.
Unfortunately, it was moved into the same if block that checks whether
either MICRO_VM_OPTIONS or MICRO_JAVA_OPTIONS was set on the command
line, before appending their values to JMH_JVM_ARGS as well. This means
that if MICRO_VM_OPTIONS or MICRO_JAVA_OPTIONS are not set,
java.library.path will not be passed to JMH either. This was not a
problem in our use case since we needed to set VM_OPTIONS for the
benchmark any ways. But it should still be fixed to make benchmarks that
don't use VM_OPTIONS, but _do_ use native libraries work, and to avoid a
confusing linkage error if the VM_OPTIONS argument is forgotten.
As a fix, I now unconditionally set $1_JMH_JVM_ARGS to include
java.library.path, and then conditionally append MICRO_VM_OPTIONS or
MICRO_JAVA_OPTIONS if they were set. Note that I also added the '$1_'
prefix to JMH_JVM_ARGS to make it local to the current macro expansion
(it seems like this was forgotten before).
Testing: verifying that with and without setting VM_OPTIONS,
java.library.path is set when running the benchmarks.
Thanks,
Jorn
[1] :
diff --git a/make/RunTests.gmk b/make/RunTests.gmk
index ccd9632ad4f..805de4dd785 100644
--- a/make/RunTests.gmk
+++ b/make/RunTests.gmk
@@ -687,13 +687,15 @@ define SetupRunMicroTestBody
$1_MICRO_BASIC_OPTIONS += -rff
$$($1_TEST_RESULTS_DIR)/jmh-result.$(MICRO_RESULTS_FORMAT)
endif
+ # Set library path for native dependencies
+ $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native
+
ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), )
- JMH_JVM_ARGS := $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
- # Set library path for native dependencies
- JMH_JVM_ARGS += -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native
- $1_MICRO_VM_OPTIONS := -jvmArgs $(call ShellQuote,$$(JMH_JVM_ARGS))
+ $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
endif
+ $1_MICRO_VM_OPTIONS := -jvmArgs $(call ShellQuote,$$($1_JMH_JVM_ARGS))
+
ifneq ($$(MICRO_ITER), )
$1_MICRO_ITER := -i $$(MICRO_ITER)
endif
