RFR: 8059625 - JEP-JDK-8043304: Test task: DTrace- tests for segmented codecache feature
Dmitrij Pochepko
dmitrij.pochepko at oracle.com
Thu Dec 25 13:04:52 UTC 2014
Hi,
please take a look at fixed version
http://cr.openjdk.java.net/~iignatyev/dpochepk/8059625/webrev.04
Thanks,
Dmitrij
>
> 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 hotspot-compiler-dev
mailing list