8161605 : The '!UseSharedSpaces' check is not need in JvmtiManageCapabilities::recompute_always_capabilities

[Mikhailo Seledtsov]

Looks good to me,

Thank you,
Misha

On 2/5/18, 12:10 PM, Alex Menkov wrote:
> Thank you for the feedback.
>
> Updated fix:
> http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev.03/
>
> --alex
>
> On 02/02/2018 11:18, Mikhailo Seledtsov wrote:
>> Hi Alex,
>>
>>    Several test-related comments:
>>    - CanGenerateAllClassHook.java
>>       " 44  * So the test runs 2 java process": s/process/process*es*/
>>
>>    - CanGenerateAllClassHook.java
>>       A general comment - please consider using "TestCommon" class 
>> and "CDSTestUtils" where possible.
>>       There are useful methods for forming ProcessBuilder with 
>> parameters specific to AppCDS, and also for checking results.
>>       In some cases the test case is so custom that use of these 
>> utilities is not applicable.
>>       Please take a look at ClassFileLoadHookTest.java as an 
>> example.  It uses TestCommon.executeAndLog(),
>>       TestCommon.checkExec() and more. Other good examples are 
>> SharedArchiveFile.java, HelloExtTest.java or other tests under
>>       appcds directory.
>>
>>       Important to note a condition of "mapping failure". When using 
>> CDS/AppCDS
>>       with Xshare:on, the mapping of a CDS archive to memory may fail 
>> at random (due to ASLR or other conditions).
>>       TestCommon.checkExec() handles such conditions propertly, 
>> otherwise you will have random/intermittent test failures.
>>
>>    - CanGenerateAllClassHook.java
>>
>>      122     private static boolean isWin() {
>>
>>       I recommend using jdk.test.lib.Platform, the isWindows() 
>> method. The SQE guideline is to use test utilities where possible.
>>       FYI, the test utilities reside in: open/test/lib/jdk/test/lib
>>
>>
>> The rest of the tests look good,
>>
>> Thank you,
>> Misha
>>
>>
>> On 2/1/18, 4:11 PM, Alex Menkov wrote:
>>> Hi Serguei,
>>>
>>> Thanks for the feedback.
>>>
>>> Updated fix: 
>>> http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev.02/
>>>
>>> --alex
>>>
>>> On 02/01/2018 11:00, serguei.spitsyn at oracle.com wrote:
>>>> One more attempt to send it with the correct to-list...
>>>> Sorry for the noise.
>>>>
>>>>
>>>> Hi Alex,
>>>>
>>>> It looks good in general.
>>>> A couple of comments on the test.
>>>>
>>>> We had a convention to avoid having test sub-folders with bug numbers.
>>>> Could you, please, rename it to some symbolic name?
>>>>
>>>> Also we need the test to fail in the native agent when an error is 
>>>> reported with the reportError().
>>>>
>>>> It'd be great if some CDS/AppCDS experts from the Runtime team look 
>>>> at the fix and test
>>>> so I've included Jiangli and Misha into the to-list.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 1/25/18 16:11, Alex Menkov wrote:
>>>>> Hi all,
>>>>>
>>>>> Please take a look at a fix for
>>>>> JDK-8161605 : The '!UseSharedSpaces' check is not need in 
>>>>> JvmtiManageCapabilities::recompute_always_capabilities
>>>>>
>>>>> The fix moves can_generate_all_class_hook_events capability from 
>>>>> "onload_capabilities" to "always_capabilities" and cleans up 
>>>>> recompute_always_capabilities method.
>>>>>
>>>>> jira: https://bugs.openjdk.java.net/browse/JDK-8161605
>>>>> webrev: 
>>>>> http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev/
>>>>>
>>>>> --alex
>>>>