RFR: 8059625 - JEP-JDK-8043304: Test task: DTrace- tests for segmented codecache feature

Filipp Zhinkin filipp.zhinkin at oracle.com
Thu Dec 25 10:25:17 UTC 2014 

Hi Dmitrij,

DtraceRunner::runDtrace prints warning in case of non-zero exit code.
It means that the test will pass even if traced JVM crashed.

I can name several issues that were found accidentally, because tests
didn't care about a crash.

Are there any other sane reasons for DTrace to return non-zero exit code
except lack of privileges?
Igor's suggestion will prevent us from running DTrace without required
privileges, so I'd prefer to see the test failure instead of a warning in case
of non-zero exit code returned by DTrace.

Also, please add @bug to the test.

Thanks,
Filipp.

On 12/25/2014 02:05 PM, Igor Ignatyev wrote:
> I'd prefer to check privileges by running dtrace against dtrace;
> and do it in DtraceRunner::dtraceAvailable, smth like
>
>> result = false;
>> dtrace = getDtracePath();
>> if ($dtrace) {
>>   $dtrace $dtrace
>>   result = $? == 0;
>> }
>> return $result;
>
>
> Igor
>
> On 12/24/2014 11:09 PM, Dmitrij Pochepko wrote:
>> Hi,
>>
>> i've placed a guard which skips test in case dtrace exits with non-zero
>> code.
>>
>> http://cr.openjdk.java.net/~iignatyev/dpochepk/8059625/webrev.02/
>>
>> Thanks,
>> Dmitrij
>>> Hi,
>>>
>>> There is one more issues related to these tests(not directly).
>>> Currently, dtrace tests can't be run in jprt, because dtrace require
>>> additional previleges for launching user.
>>> Perhaps it's just user to be granted previleges in jprt solaris hosts...
>>> Not sure about some guard in tests... the only way i know so far is to
>>> just try launching dtrace and parse output
>>> saying about missing previleges, but it doesn't seem to be good solution.
>>> I think test should assume to be launched in correct environment and
>>> in case it's failed because of previleges, evaluating engineer
>>> starting to work on respective env. issue...
>>> Does anybody have some additional concerns?
>>>
>>> Thanks,
>>> Dmitrij
>>>
>>>> Hi Dmitrij,
>>>>
>>>>
>>>> There is still a typo in spelling (DESCTRUCTIVE => DESTRUCTIVE):
>>>>
>>>> test/testlibrary/com/oracle/java/testlibrary/dtrace/DtraceRunner.java
>>>>    42     public static final String 
>>>> PERMIT_DESCTRUCTIVE_ACTIONS_DTRACE_OPTION = "w";
>>>>
>>>> test/compiler/codecache/dtrace/SegmentedCodeCacheDtraceTest.java
>>>>   85 DtraceRunner.PERMIT_DESCTRUCTIVE_ACTIONS_DTRACE_OPTION,
>>>>
>>>>
>>>> Otherwise, looks good.
>>>> Please, consider it reviewed (no need to re-review).
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 12/23/14 3:36 AM, Dmitrij Pochepko wrote:
>>>>> Hi,
>>>>> please review updated webrev
>>>>>
>>>>> http://cr.openjdk.java.net/~iignatyev/dpochepk/8059625/webrev.01/
>>>>>
>>>>> Thanks,
>>>>> Dmitrij
>>>>>> On 12/23/14 12:37 AM, Dmitrij Pochepko wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Thank you for catching these issues.
>>>>>>> I have a question regarding last comment: does it make any
>>>>>>> difference to change "reading of static member 3 times" to
>>>>>>> "copying into static member of another class and then read it 3
>>>>>>> times"?
>>>>>>
>>>>>> Yes, it does (it is a minor issue though).
>>>>>> It is because the class names are pretty big.
>>>>>> It would simplify the code and improve readability, right?
>>>>>>
>>>>>> You already do it two times:
>>>>>>   179             List<Executable> tml
>>>>>>   180                     = 
>>>>>> SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
>>>>>>   ...
>>>>>>   235             List<Executable> mlist
>>>>>>   236                     = 
>>>>>> SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
>>>>>>
>>>>>> My suggestion is to do it once somewhere before the line 74:
>>>>>>    static final List<Executable> MLIST = 
>>>>>> SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
>>>>>> and then use the mlist in other places where it is needed:
>>>>>>
>>>>>>    75         String params = MLIST.stream()
>>>>>>   ...
>>>>>>   182                 d.put(MLIST.get(i).getName(), new 
>>>>>> MethodData(MLIST.get(i).getName(),
>>>>>>   ...
>>>>>>   239             for (int i = MLIST.size() - 1; i > -1; i--) {
>>>>>>   240 sb.append(MLIST.get(i).getName()).append(delimeter);
>>>>>> These lines will go away:
>>>>>>   179             List<Executable> tml
>>>>>>   180                     = 
>>>>>> SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
>>>>>>   ...
>>>>>>   235             List<Executable> mlist
>>>>>>   236                     = 
>>>>>> SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
>>>>>>
>>>>>>
>>>>>> Both private static classes are inner classes of the public one.
>>>>>> The MLIST will be in a context for the inner classes, and so, needs
>>>>>> no prefixing.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Dmitrij
>>>>>>>> Hi Dmitry,
>>>>>>>>
>>>>>>>>
>>>>>>>> It looks good in general.
>>>>>>>> Some minor comments are below.
>>>>>>>>
>>>>>>>> test/testlibrary/com/oracle/java/testlibrary/dtrace/DtraceRunner.java
>>>>>>>>
>>>>>>>>    42     public static final String 
>>>>>>>> PERMIT_DESCTUCTIVE_ACTIONS_DTRACE_OPTION = "w";
>>>>>>>>    A typo in the constant name: DESCTUCTIVE => DESTRUCTIVE
>>>>>>>>
>>>>>>>>
>>>>>>>> test/compiler/codecache/dtrace/SegmentedCodeCacheDtraceTest.java
>>>>>>>>
>>>>>>>>    84 DtraceRunner.PERMIT_DESCTUCTIVE_ACTIONS_DTRACE_OPTION,
>>>>>>>>    A typo in the constant name: DESCTUCTIVE => DESTRUCTIVE
>>>>>>>>
>>>>>>>>
>>>>>>>>    60     private static final String WORKER_CLASS_NAME
>>>>>>>>    61             = SegmentedCodeCacheDtraceTestWorker.class.getName();
>>>>>>>>   ...
>>>>>>>>    80 runner.runDtrace(JDKToolFinder.getTestJDKTool("java"), JAVA_OPTS,
>>>>>>>>    81 SegmentedCodeCacheDtraceTestWorker.class.getName(), params,
>>>>>>>>    The WORKER_CLASS_NAME can be used at 81.
>>>>>>>>
>>>>>>>>    75         String params = 
>>>>>>>> SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST.stream()
>>>>>>>>   ...
>>>>>>>>   179             List<Executable> tml
>>>>>>>>   180                     = 
>>>>>>>> SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
>>>>>>>>   ...
>>>>>>>>   235             List<Executable> mlist
>>>>>>>>   236                     = 
>>>>>>>> SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
>>>>>>>>     The TESTED_METHODS_LIST is used three times.
>>>>>>>>     It can be cached at the top and re-used.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/19/14 11:03 AM, Dmitrij Pochepko wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Please review changes for
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8059625 -
>>>>>>>>> JEP-JDK-8043304: Test task: DTrace- tests for segmented
>>>>>>>>> codecache feature
>>>>>>>>>
>>>>>>>>> Description: this fix introduce dtrace test, which verify that
>>>>>>>>> different combinations of available compile levels(and, in case
>>>>>>>>> compile levels allows it, different code heaps as result)
>>>>>>>>> doesn't affect callstack shown by dtrace. There is a control
>>>>>>>>> class SegmentedCodeCacheDtraceTest.java and class for running
>>>>>>>>> via dtrace SegmentedCodeCacheDtraceTestWorker.java. A dtrace d
>>>>>>>>> script is also present (SegmentedCodeCacheDtraceTestScript.d). A
>>>>>>>>> control class is using DtraceRunner.java to run dtrace and then
>>>>>>>>> analyzing results using class
>>>>>>>>> SegmentedCodeCacheDtraceResultsAnalyzer with
>>>>>>>>> DtraceResultsAnalyzer interface.
>>>>>>>>> There is also a small class CompilerUtils.java created for
>>>>>>>>> usefull common code.
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~iignatyev/dpochepk/8059625/webrev.00/
>>>>>>>>>
>>>>>>>>> Additional note: Please note that this path assumes that fix for
>>>>>>>>> JDK-8066440 - Various changes in testlibrary for JDK-8059613 is
>>>>>>>>> also applied.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Dmitrij
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>

More information about the serviceability-dev mailing list