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

Igor Ignatyev igor.ignatyev at oracle.com
Thu Dec 25 12:22:52 UTC 2014 

On 12/25/2014 03:12 PM, Filipp Zhinkin wrote:
> Dmitrij,
>
> I believe that it should be "@bug 8015774".
agree
>
> Now DtraceRunner::runDtrace isn't checking exit code at all.
> I think that it should assert on zero exit code.
no, it shouldn't it should be a part of custom implementation of 
DtraceResultsAnalyzer::analyze.

Dima, could you please add exit code check into 
SegmentedCodeCacheDtraceTest.SegmentedCodeCacheDtraceResultsAnalyzer::analyze

>
> Thanks,
> Filipp.
>
> On 12/25/2014 04: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