Hi Alex,
On 28/08/2020 6:29 am, Alex Menkov wrote:
Hi David,
On 08/26/2020 21:49, David Holmes wrote:
Hi Alex,
On 21/08/2020 6:54 am, Alex Menkov wrote:
Hi Igor,
On 08/20/2020 09:23, Igor Ignatyev wrote:
HI Alex,
one minor nit: according to usual java coding conventions,
isJVMTIIncluded should be spelled as isJvmtiIncluded. otherwise the
fix looks good to me.
I tried to be consistent with other methods like
isCDSIncludedInVmBuild, isJFRIncludedInVmBuild, isGCSupported,
isGCSelected, etc.
Yes - when a name includes an acronym the use of camel-case is a
secondary consideration.
Maybe this should be isJVMTIIncludedInVmBuild..
Yes that seems better and I would also prefer to see it implemented in
the same style as:
Sorry, the fix was pushed several days ago.
Sorry I was away and didn't notice the date. The review still seemed
open as you seemed to be posing a query to Igor on the naming.
Do you want to change the name to isJVMTIIncludedInVmBuild by follow-up?
I think we should go for consistency in naming for these type of feature
tests, and also consistency in the implementation - as I said this
doesn't need a runtime check at all (though I wonder if the C++ compiler
is smart enough to elide it?).
That said I would have preferred the existing checks to not have
"InVmBuild" in the name as it seems redundant/unnecessary to me. But a
RFE to get consistency here would be good thing IMO.
Thanks,
David
-----
WB_ENTRY(jboolean, WB_IsJFRIncludedInVmBuild(JNIEnv* env))
#if INCLUDE_JFR
return true;
#else
return false;
#endif // INCLUDE_JFR
WB_END
to avoid implicit booleans and avoid the runtime condition check.
I don't think true/false are correct here. This is jboolean, not bool.
INCLUDE_JVMTI ? JNI_TRUE : JNI_FALSE
#if INCLUDE_JVMTI
return JNI_TRUE;
#else
return JNI_FALSE;
#endif
looks better imo.
--alex
Thanks,
David
-----
Other tests will be updated in the follow-ups.
have you already identified all the tests which need this @requires?
filed bugs/RFEs for them?
Not yet.
I had problem with running all hotspot tests with minimal build (for
some reason jtreg was not able to complete it), so I decided start
from the tests mentioned in the jira issue and then test
area-by-area, file and fix the tests in batches.
--alex
Cheers,
-- Igor
On Aug 19, 2020, at 6:02 PM, Alex Menkov <[email protected]>
wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8251384
webrev:
http://cr.openjdk.java.net/~amenkov/jdk16/minimal_jvmti/webrev/
The fix introduces new @requires option "vm.jvmti":
test/lib/sun/hotspot/WhiteBox.java
test/jtreg-ext/requires/VMProps.java
src/hotspot/share/prims/whitebox.cpp
test/hotspot/jtreg/TEST.ROOT
and updates tests in test/hotspot/jtreg/serviceability/jvmti (the
only change in all tests is added "@requires vm.jvmti")
Other tests will be updated in the follow-ups.
The
