RFR: 8059625 - JEP-JDK-8043304: Test task: DTrace- tests for segmented codecache feature
Dmitrij Pochepko
dmitrij.pochepko at oracle.com
Thu Dec 25 12:19:06 UTC 2014
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