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.spit...@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
