[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8059625 - JEP-JDK-8043304: Test task: DTrace- tests for segmented codecache feature
From: Filipp Zhinkin <filipp.zhinkin () oracle ! com>
Date: 2014-12-25 13:05:12
Message-ID: 549C0B88.9080408 () oracle ! com
[Download RAW message or body]
Looks good.
Thanks,
Filipp.
On 12/25/2014 05:04 PM, Dmitrij Pochepko wrote:
> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic