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

Igor Ignatyev igor.ignatyev at oracle.com
Thu Dec 25 12:19:13 UTC 2014 

looks good to me.

Thanks,
Igor

On 12/25/2014 03:19 PM, Dmitrij Pochepko wrote:
> Hi,
>
> new webrev:
> http://cr.openjdk.java.net/~iignatyev/dpochepk/8059625/webrev.03
> I've added @bug tag
> and moved process guard to DtraceRunner::dtraceAvailable
>
> Thanks,
> Dmitrij
>> 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