[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8251384: [TESTBUG] jvmti tests should not be executed with minimal VM
From:       David Holmes <david.holmes () oracle ! com>
Date:       2020-08-28 1:30:08
Message-ID: 5a7bdc82-4a18-0a14-dbfe-c802c30e4815 () oracle ! com
[Download RAW message or body]

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 <alexey.menkov@oracle.com> 
>>>>> 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
>>>>
[prev in list] [next in list] [prev in thread] [next in thread]